(WIP) salt-api: Prevent SA impersonation

When using ServiceAccount, we were not checking if the provided Bearer
`token` actually belonged to the claimed `username`. This is now done,
and we add tests to prevent regressions.

NOTE: storage-operator username, from SaltAPI PoV, is now
'system:serviceaccount:kube-system:storage-operator'.

Fixes: #2634

salt/_auth/kubernetes_rbac.py

@@ -43,23 +43,6 @@ def wrapped(*args, **kwargs):
     return wrapped
 
 
-def _check_k8s_creds(kubeconfig, token):
-    """Check the provided credentials against /version."""
-    # Using the '/version/' endpoint which is unauthenticated by default but,
-    # when presented authentication data, will process this information and fail
-    # accordingly.
-    url = '{}/version/'.format(kubeconfig.host)
-    verify = kubeconfig.ssl_ca_cert if kubeconfig.verify_ssl else False
-    try:
-        response = requests.get(
-            url, headers={'Authorization': token}, verify=verify
-        )
-        return 200 <= response.status_code < 300
-    except:
-        log.exception('Error during request')
-        raise
-
-
 def _check_auth_args(on_fail=lambda: False):
     def wrapper(f):
         @wraps(f)
@@ -168,7 +151,34 @@ def _groups_basic(kubeconfig, username, password):
 
 @_log_exceptions
 def _auth_bearer(kubeconfig, username, token):
-    return _check_k8s_creds(kubeconfig, 'Bearer {}'.format(token))
+    """Check the provided bearer token using the TokenReview API."""
+    client = kubernetes.client.ApiClient(configuration=kubeconfig)
+    authn_api = kubernetes.client.AuthenticationV1Api(api_client=client)
+
+    token_review = authn_api.create_token_review(
+        body=kubernetes.client.V1TokenReview(
+            spec=kubernetes.client.V1TokenReviewSpec(token=token)
+        )
+    )
+
+    if token_review.status.error:
+        log.error('Failed to create TokenReview: %s',
+                  token_review.status.error)
+        return False
+
+    if token_review.status.authenticated:
+        if token_review.status.user.username != username:
+            log.error(
+                "Provided token belongs to '%s', does not match '%s'",
+                token_review.status.user.username,
+                username,
+            )
+            return False
+        else:
+            return True
+    else:
+        log.error('Provided token failed to authenticate')
+        return False
 
 AUTH_HANDLERS['bearer'] = {
     'auth': _auth_bearer,

salt/metalk8s/salt/master/files/master-99-metalk8s.conf.j2

@@ -42,7 +42,7 @@ external_auth:
       - '@wheel'
       - '@runner'
       - '@jobs'
-    storage-operator:
+    'system:serviceaccount:kube-system:storage-operator':
       - '*':
         - 'disk.dump'
         - 'state.sls'

storage-operator/pkg/controller/volume/volume_controller.go

@@ -2,7 +2,6 @@ package volume
 
 import (
 	"context"
-	b64 "encoding/base64"
 	"fmt"
 	"io/ioutil"
 	"time"
@@ -975,7 +974,10 @@ func getAuthCredential(config *rest.Config) *salt.Credential {
 	if config.BearerToken != "" {
 		log.Info("using ServiceAccount bearer token")
 		return salt.NewCredential(
-			"storage-operator", config.BearerToken, salt.Bearer,
+			// FIXME: this should depend on the actual SA used
+			"system:serviceaccount:kube-system:storage-operator",
+			config.BearerToken,
+			salt.Bearer,
 		)
 	} else if config.Username != "" && config.Password != "" {
 		log.Info("using Basic HTTP authentication")

storage-operator/pkg/controller/volume/volume_controller_test.go

@@ -18,7 +18,9 @@ func TestGetAuthCredential(t *testing.T) {
 		"ServiceAccount": {
 			token: "foo", username: "", password: "",
 			expected: salt.NewCredential(
-				"storage-operator", "foo", salt.Bearer,
+				"system:serviceaccount:kube-system:storage-operator",
+				"foo",
+				salt.Bearer,
 			),
 		},
 		"BasicAuth": {

tests/post/features/salt_api.feature

@@ -9,12 +9,22 @@ Feature: SaltAPI
         And we have '@runner' perms
         And we have '@jobs' perms
 
-    Scenario: Login to SaltAPI using a ServiceAccount
+    Scenario: Login to SaltAPI using the storage-operator ServiceAccount
         Given the Kubernetes API is available
-        When we login to SaltAPI with the ServiceAccount 'storage-operator'
+        When we login to SaltAPI with the ServiceAccount 'kube-system/storage-operator'
         Then we can invoke '["disk.dump", "state.sls"]' on '*'
         And we have '@jobs' perms
 
+    Scenario: Login to SaltAPI using any ServiceAccount
+        Given the Kubernetes API is available
+        When we login to SaltAPI with the ServiceAccount 'kube-system/default'
+        Then we have no permissions
+
+    Scenario: SaltAPI impersonation using a ServiceAccount
+        Given the Kubernetes API is available
+        When we impersonate user 'system:serviceaccount:kube-system:storage-operator' against SaltAPI using the ServiceAccount 'kube-system/default'
+        Then authentication fails
+
     Scenario: Login to SaltAPI using an incorrect password
         Given the Kubernetes API is available
         When we login to SaltAPI as 'admin' using password 'notadmin'

tests/post/steps/test_salt_api.py

@@ -16,8 +16,19 @@ def test_login_basic_auth_to_salt_api(host):
     pass
 
 @scenario('../features/salt_api.feature',
-          'Login to SaltAPI using a ServiceAccount')
-def test_login_bearer_auth_to_salt_api(host):
+          'Login to SaltAPI using the storage-operator ServiceAccount')
+def test_login_salt_api_storage_operator(host):
+    pass
+
+
+@scenario('../features/salt_api.feature',
+          'Login to SaltAPI using any ServiceAccount')
+def test_login_salt_api_service_account(host):
+    pass
+
+@scenario('../features/salt_api.feature',
+          'SaltAPI impersonation using a ServiceAccount')
+def test_salt_api_impersonation_with_bearer_auth(host):
     pass
 
 @scenario('../features/salt_api.feature', 'Login to SaltAPI using an incorrect password')
@@ -49,21 +60,43 @@ def login_salt_api_basic(host, username, password, version, context):
 
 
 @when(parsers.parse(
-    "we login to SaltAPI with the ServiceAccount '{account_name}'"))
-def login_salt_api_token(host, k8s_client, account_name, version, context):
+    "we login to SaltAPI with the ServiceAccount '{namespace}/{account_name}'"))
+def login_salt_api_token(
+    host, k8s_client, namespace, account_name, version, context
+):
     address = _get_salt_api_address(host, version)
     service_account = k8s_client.read_namespaced_service_account(
-        name=account_name, namespace='kube-system'
+        name=account_name, namespace=namespace
     )
+    username = 'system:serviceaccount:{}:{}'.format(namespace, account_name)
     secret = k8s_client.read_namespaced_secret(
-        name=service_account.secrets[0].name, namespace='kube-system'
+        name=service_account.secrets[0].name, namespace=namespace
     )
     token = base64.decodebytes(secret.data['token'].encode('utf-8'))
     context['salt-api'] = _salt_api_login(
-        address, username=account_name, token=token
+        address, username=username, token=token
     )
 
 
+@when(parsers.parse(
+    "we impersonate user '{username}' against SaltAPI "
+    "using the ServiceAccount '{namespace}/{account_name}'"
+))
+def login_salt_api_token_override_username(
+    host, k8s_client, namespace, account_name, username, version, context
+):
+    address = _get_salt_api_address(host, version)
+    service_account = k8s_client.read_namespaced_service_account(
+        name=account_name, namespace=namespace
+    )
+    secret = k8s_client.read_namespaced_secret(
+        name=service_account.secrets[0].name, namespace=namespace
+    )
+    token = base64.decodebytes(secret.data['token'].encode('utf-8'))
+    context['salt-api'] = _salt_api_login(
+        address, username=username, token=token
+    )
+
 # }}}
 # Then {{{
 
@@ -102,6 +135,9 @@ def invoke_module_on_target(host, context, modules, targets):
 def have_perms(host, context, perms):
     assert perms in context['salt-api']['perms']
 
+@then(parsers.parse("we have no permissions"))
+def have_no_perms(host, context):
+    assert context['salt-api']['perms'] == {}
 
 # }}}
 # Helpers {{{