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

TeddyAndrieux · 2019-11-22T15:39:12Z

Component:

'salt', 'tests'

Context:

Currently we have no permissions when using bearer token auth

Summary:

add a groups function to authenticate bearer auth using the same approach as
the basic auth for the moment
Also update test accordingly

bert-e · 2019-11-22T15:39:14Z

Hello teddyandrieux,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Status report is not available.

bert-e · 2019-11-22T15:39:18Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

tests/post/features/salt_api.feature

salt/_auth/kubernetes_rbac.py

bert-e · 2019-11-22T17:14:53Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following reviewers are expecting changes from the author, or must review again:

@gdemonet

salt/_auth/kubernetes_rbac.py

NicolasT · 2019-11-25T19:33:39Z

tests/post/features/salt_api.feature

@@ -12,7 +12,10 @@ Feature: SaltAPI
    Scenario: Login to SaltAPI using a ServiceAccount
        Given the Kubernetes API is available
        When we login to SaltAPI with the ServiceAccount 'storage-operator'
-        Then we can invoke '["disk.dump", "state.sls"]' on '*'
+        Then we can ping all minions


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

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" ],

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

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

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.

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.

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

salt/_auth/kubernetes_rbac.py

bert-e · 2019-11-27T16:32:58Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

Currently we have no permissions when using bearer token auth, add a groups function to authenticate bearer auth using the same approach as the basic auth for the moment

Add a fixture to create an Admin ServiceAccount for test purpose. This fixture create a ServiceAccount and a ClusterRoleBinding to bind this new ServiceAccount to `cluster-admin` ClusterRole

Add test that a ServiceAccount bind to `cluster-admin` ClusterRole is able to everything with salt-api

TeddyAndrieux · 2019-11-29T08:20:18Z

/approve

bert-e · 2019-11-29T08:20:23Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
one peer

Peer approvals must include at least 1 approval from the following list:

The following options are set: approve

bert-e · 2019-11-29T08:20:49Z

In the queue

The changeset has received all authorizations and has been added to the
relevant queue(s). The queue(s) will be merged in the target development
branch(es) as soon as builds have passed.

The changeset will be merged in:

✔️ development/2.5

The following branches will NOT be impacted:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4

There is no action required on your side. You will be notified here once
the changeset has been merged. In the unlikely event that the changeset
fails permanently on the queue, a member of the admin team will
contact you to help resolve the matter.

IMPORTANT

Please do not attempt to modify this pull request.

Any commit you add on the source branch will trigger a new cycle after the
current queue is merged.
Any commit you add on one of the integration branches will be lost.

If you need this pull request to be removed from the queue, please contact a
member of the admin team now.

The following options are set: approve

bert-e · 2019-11-29T08:20:54Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/2.5

The following branches have NOT changed:

development/1.0
development/1.1
development/1.2
development/1.3
development/2.0
development/2.1
development/2.2
development/2.3
development/2.4

Please check the status of the associated issue None.

Goodbye teddyandrieux.

TeddyAndrieux requested a review from a team November 22, 2019 15:39

gdemonet suggested changes Nov 22, 2019

View reviewed changes

tests/post/features/salt_api.feature Outdated Show resolved Hide resolved

salt/_auth/kubernetes_rbac.py Outdated Show resolved Hide resolved

NicolasT reviewed Nov 25, 2019

View reviewed changes

salt/_auth/kubernetes_rbac.py Outdated Show resolved Hide resolved

NicolasT reviewed Nov 25, 2019

View reviewed changes

TeddyAndrieux force-pushed the improvement/add-groups-support-for-bearer-auth-saltapi branch from 1d4b15a to 4859253 Compare November 27, 2019 13:54

gdemonet previously approved these changes Nov 27, 2019

View reviewed changes

salt/_auth/kubernetes_rbac.py Outdated Show resolved Hide resolved

salt/auth: Add groups support for bearer auth

ba68911

Currently we have no permissions when using bearer token auth, add a groups function to authenticate bearer auth using the same approach as the basic auth for the moment

TeddyAndrieux dismissed gdemonet’s stale review via ba68911 November 27, 2019 17:48

TeddyAndrieux force-pushed the improvement/add-groups-support-for-bearer-auth-saltapi branch 6 times, most recently from d90f001 to 9d7e63d Compare November 28, 2019 15:01

TeddyAndrieux added 2 commits November 28, 2019 16:01

tests: Add Admin ServiceAccount fixture

c2d62ac

Add a fixture to create an Admin ServiceAccount for test purpose. This fixture create a ServiceAccount and a ClusterRoleBinding to bind this new ServiceAccount to `cluster-admin` ClusterRole

tests: Add salt-api test for Bearer token of Admin SA

9d7e63d

Add test that a ServiceAccount bind to `cluster-admin` ClusterRole is able to everything with salt-api

TeddyAndrieux requested review from NicolasT, gdemonet and slaperche-scality November 28, 2019 15:51

carlito-scality approved these changes Nov 29, 2019

View reviewed changes

bert-e merged commit 9d7e63d into development/2.5 Nov 29, 2019

bert-e deleted the improvement/add-groups-support-for-bearer-auth-saltapi branch November 29, 2019 08:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

TeddyAndrieux commented Nov 22, 2019

bert-e commented Nov 22, 2019

bert-e commented Nov 22, 2019

bert-e commented Nov 22, 2019

NicolasT Nov 25, 2019

TeddyAndrieux Nov 27, 2019

gdemonet Nov 27, 2019

TeddyAndrieux Nov 27, 2019

gdemonet Nov 27, 2019

gdemonet Nov 27, 2019

gdemonet Nov 27, 2019

TeddyAndrieux Nov 28, 2019

bert-e commented Nov 27, 2019

TeddyAndrieux commented Nov 29, 2019

bert-e commented Nov 29, 2019

bert-e commented Nov 29, 2019

bert-e commented Nov 29, 2019

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

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

Conversation

TeddyAndrieux commented Nov 22, 2019

bert-e commented Nov 22, 2019

Hello teddyandrieux,

bert-e commented Nov 22, 2019

Waiting for approval

bert-e commented Nov 22, 2019

Waiting for approval

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bert-e commented Nov 27, 2019

Waiting for approval

TeddyAndrieux commented Nov 29, 2019

bert-e commented Nov 29, 2019

Waiting for approval

bert-e commented Nov 29, 2019

In the queue

bert-e commented Nov 29, 2019