Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

salt/auth: Add groups support for bearer auth #2068

Merged
merged 3 commits into from
Nov 29, 2019
Merged

salt/auth: Add groups support for bearer auth #2068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba68911
salt/auth: Add groups support for bearer auth
TeddyAndrieux Nov 19, 2019
c2d62ac
tests: Add Admin ServiceAccount fixture
TeddyAndrieux Nov 28, 2019
9d7e63d
tests: Add salt-api test for Bearer token of Admin SA
TeddyAndrieux Nov 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 55 additions & 21 deletions salt/_auth/kubernetes_rbac.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,40 @@ def _check_k8s_creds(kubeconfig, token):
raise


def _check_node_admin(kubeconfig):
client = kubernetes.client.ApiClient(configuration=kubeconfig)

authz_api = kubernetes.client.AuthorizationV1Api(api_client=client)

result = authz_api.create_self_subject_access_review(
body=kubernetes.client.V1SelfSubjectAccessReview(
spec=kubernetes.client.V1SelfSubjectAccessReviewSpec(
resource_attributes=kubernetes.client.V1ResourceAttributes(
resource='nodes',
verb='*',
),
),
),
)

return result.status.allowed


AVAILABLES_GROUPS = {
'node-admins': _check_node_admin
}


def _get_groups(kubeconfig):
groups = set()

for group, func in AVAILABLES_GROUPS.items():
if func(kubeconfig):
groups.add(group)

return list(groups)


@_log_exceptions
def _auth_basic(kubeconfig, username, token):
decoded = base64.decodestring(token)
Expand Down Expand Up @@ -86,42 +120,42 @@ def _groups_basic(kubeconfig, username, token):
kubeconfig.cert_file = None
kubeconfig.key_file = None

client = kubernetes.client.ApiClient(configuration=kubeconfig)

authz_api = kubernetes.client.AuthorizationV1Api(api_client=client)

groups = set()

result = authz_api.create_self_subject_access_review(
body=kubernetes.client.V1SelfSubjectAccessReview(
spec=kubernetes.client.V1SelfSubjectAccessReviewSpec(
resource_attributes=kubernetes.client.V1ResourceAttributes(
resource='nodes',
verb='*',
),
),
),
)

if result.status.allowed:
groups.add('node-admins')

return list(groups)
return _get_groups(kubeconfig)


AUTH_HANDLERS['basic'] = {
'auth': _auth_basic,
'groups': _groups_basic,
}


@_log_exceptions
def _auth_bearer(kubeconfig, username, token):
return _check_k8s_creds(kubeconfig, 'Bearer {}'.format(token))


@_log_exceptions
def _groups_bearer(kubeconfig, _username, token):
kubeconfig.api_key = {
'authorization': token,
}
kubeconfig.api_key_prefix = {
'authorization': 'Bearer',
}
kubeconfig.username = None
kubeconfig.password = None
kubeconfig.cert_file = None
kubeconfig.key_file = None

return _get_groups(kubeconfig)


AUTH_HANDLERS['bearer'] = {
'auth': _auth_bearer,
'groups': _groups_bearer
}


@_log_exceptions
def _load_kubeconfig(opts):
config = {
Expand Down
105 changes: 105 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,111 @@ def test_something(k8s_client):
return api_cls(api_client=k8s_apiclient)


@pytest.fixture
def admin_sa(k8s_client, k8s_apiclient):
"""Fixture to create a ServiceAccount which is bind to `cluster-admin`
ClusterRole and return the ServiceAccount name
"""
rbac_k8s_client = kubernetes.client.RbacAuthorizationV1Api(
api_client=k8s_apiclient
)
sa_name = 'test-admin'
sa_namespace = 'default'
sa_manifest = {
'apiVersion': 'v1',
'kind': 'ServiceAccount',
'metadata': {
'name': sa_name,
'namespace': sa_namespace
}
}
crb_manifest = {
'apiVersion': 'rbac.authorization.k8s.io/v1',
'kind': 'ClusterRoleBinding',
'metadata': {
'name': sa_name
},
'roleRef': {
'apiGroup': 'rbac.authorization.k8s.io',
'kind': 'ClusterRole',
'name': 'cluster-admin'
},
'subjects': [
{
'kind': 'ServiceAccount',
'name': sa_name,
'namespace': sa_namespace
}
]
}

k8s_client.create_namespaced_service_account(
body=sa_manifest,
namespace=sa_namespace
)
rbac_k8s_client.create_cluster_role_binding(body=crb_manifest)

def _check_crb_exists():
try:
rbac_k8s_client.read_cluster_role_binding(name=sa_name)
except kubernetes.client.rest.ApiException as err:
if err.status == 404:
raise AssertionError('ClusterRoleBinding not yet created')
raise

def _check_sa_exists():
try:
sa_obj = k8s_client.read_namespaced_service_account(
name=sa_name,
namespace=sa_namespace
)
except kubernetes.client.rest.ApiException as err:
if err.status == 404:
raise AssertionError('ServiceAccount not yet created')
raise

assert sa_obj.secrets
assert sa_obj.secrets[0].name

try:
secret_obj = k8s_client.read_namespaced_secret(
sa_obj.secrets[0].name,
sa_namespace
)
except kubernetes.client.rest.ApiException as err:
if err.status == 404:
raise AssertionError('Secret not yet created')
raise

assert secret_obj.data.get('token')

# Wait for ClusterRoleBinding to exists
utils.retry(_check_crb_exists, times=20, wait=3)

# Wait for ServiceAccount to exists
utils.retry(_check_sa_exists, times=20, wait=3)

yield (sa_name, sa_namespace)

try:
rbac_k8s_client.delete_cluster_role_binding(
name=sa_name,
body=kubernetes.client.V1DeleteOptions(
propagation_policy='Foreground'
)
)
except kubernetes.client.rest.ApiException:
pass

k8s_client.delete_namespaced_service_account(
name=sa_name,
namespace=sa_namespace,
body=kubernetes.client.V1DeleteOptions(
propagation_policy='Foreground'
)
)


@pytest.fixture(scope='module')
def bootstrap_config(host):
with host.sudo():
Expand Down
9 changes: 9 additions & 0 deletions tests/post/features/salt_api.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@ Feature: SaltAPI
And we have '@runner' perms
And we have '@jobs' perms

Scenario: Login to SaltAPI using an admin ServiceAccount
Given the Kubernetes API is available
When we login to SaltAPI with an admin ServiceAccount
Then we can ping all minions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, the fact that this test requires changes shows that there's an issue somewhere in the implementation. This test should remain as-is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw this test do not require change but since storage-opeator is able to do a * on nodes he also have this perms.

Basically storage-operator perms in this PR are:

        "perms": [
            {
                "*": [
                    "disk.dump", 
                    "state.sls"
                ]
            }, 
            "@jobs", 
            {
                "*": [
                    ".*"
                ]
            }, 
            "@wheel", 
            "@runner", 
            "@jobs"
        ],

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it shouldn't have those perms?!? https://github.com/scality/metalk8s/blob/development/2.4/storage-operator/deploy/role.yaml

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the cluster role say that he can do * on nodes the check we do for groups is approved so we add node-admin groups thats why but if we dont want that the storage operator can do this then we need to either change the check from groups function or changes the rule of his cluster role

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I read the Role too fast. Then that would be a bug I guess, we shouldn't give that many permissions to storage-operator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this issue, it seems we should not have kept the default ClusterRole that gets created when the project is initialized with operator-sdk new. Since operator-sdk generate k8s doesn't generate this file, it would make sense to update it to match our needs and commit it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2084

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it's not part of this PR, because we may want to fix it also in 2.4

And we can invoke '[".*"]' on '*'
And we have '@wheel' perms
And we have '@runner' perms
And we have '@jobs' perms

Scenario: Login to SaltAPI using a ServiceAccount
Given the Kubernetes API is available
When we login to SaltAPI with the ServiceAccount 'storage-operator'
Expand Down
43 changes: 33 additions & 10 deletions tests/post/steps/test_salt_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,15 @@
def test_login_basic_auth_to_salt_api(host):
pass


@scenario('../features/salt_api.feature',
'Login to SaltAPI using an admin ServiceAccount')
@scenario('../features/salt_api.feature',
'Login to SaltAPI using a ServiceAccount')
def test_login_bearer_auth_to_salt_api(host):
pass


@scenario('../features/salt_api.feature', 'Login to SaltAPI using an incorrect password')
def test_login_to_salt_api_using_an_incorrect_password(host, request):
pass
Expand Down Expand Up @@ -49,19 +53,25 @@ def login_salt_api_basic(host, username, password, version, context):
context['salt-api'] = _salt_api_login(address, username, token, 'Basic')


@when("we login to SaltAPI with an admin ServiceAccount")
def login_salt_api_admin_sa(host, k8s_client, admin_sa, version, context):
sa_name, sa_namespace = admin_sa
address = _get_salt_api_address(host, version)

context['salt-api'] = _login_salt_api_sa(
address, k8s_client,
sa_name, sa_namespace
)


@when(parsers.parse(
"we login to SaltAPI with the ServiceAccount '{account_name}'"))
def login_salt_api_token(host, k8s_client, account_name, version, context):
def login_salt_api_system_sa(host, k8s_client, 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'
)
secret = k8s_client.read_namespaced_secret(
name=service_account.secrets[0].name, namespace='kube-system'
)
token = base64.decodebytes(secret.data['token'].encode('utf-8'))
context['salt-api'] = _salt_api_login(
address, account_name, token, 'Bearer'

context['salt-api'] = _login_salt_api_sa(
address, k8s_client,
account_name, 'kube-system'
)


Expand Down Expand Up @@ -108,6 +118,19 @@ def have_perms(host, context, perms):
# Helpers {{{


def _login_salt_api_sa(address, k8s_client, sa_name, sa_namespace):
service_account = k8s_client.read_namespaced_service_account(
name=sa_name, namespace=sa_namespace
)
secret = k8s_client.read_namespaced_secret(
name=service_account.secrets[0].name, namespace=sa_namespace
)
token = base64.decodebytes(secret.data['token'].encode('utf-8'))
return _salt_api_login(
address, sa_name, token, 'Bearer'
)


def _get_salt_api_address(host, version):
SALT_API_PORT = 4507
cmd_cidr = ' '.join([
Expand Down