Storage Operator can use SaltAPI without RBAC in 2.5 #2634
Comments
gdemonet
added
state:question
|
First observation: in |
|
There's no difference in the eauth code between 2.5 and 2.6, so it's odd as to why it worked in 2.5 and not 2.6. |
|
Actually not sure there's something wrong here. When looking at This So everything works as designed (except that maybe the |
This is indeed a bug and needs to be fixed. |
|
Seems introduced in 56eda6d @slaperche-scality. Giving the |
|
Actually, in 2.6 the Now, |
Yup, see #2084 |
That's really a weird side-effect, I've totally missed it :/ |
I can't agree more. Could we imagine building another RBAC model for Salt in K8s? I'm thinking, just as an example, we could create dummy objects (empty ConfigMaps) in some An example apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
namespace: metalk8s-salt
name: salt-admin
rules:
- apiGroups:
- ""
resources:
- configmaps
resourceNames:
- salt-rbac-admin
verbs:
- getIf we really wanted to (not sure though), we could even use the contents of these ConfigMaps as a source for templating the RBAC rules in the Salt master configuration. |
This checks that if not allowed in Salt eauth perms, an authenticated user (using 'storage-operator' as a practical example) cannot run any module (using 'test.ping' as an example). See: #2634
This checks that if not allowed in Salt eauth perms, an authenticated user (using 'storage-operator' as a practical example) cannot run any module (using 'test.ping' as an example). See: #2634
This checks that if not allowed in Salt eauth perms, an authenticated user (using 'storage-operator' as a practical example) cannot run any module (using 'test.ping' as an example). See: #2634
Another idea: when using the |
We can't, I believe, and shouldn't:
|
|
Mmh right, ServiceAccounts only have |
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
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
We only assessed if an authentication attempt failed, while tried to check the actual permissions for an authn assumed successful. To make errors more explicit, we add a `then` check that the "authentication succeeds". See: #2634
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
We only assessed if an authentication attempt failed, while tried to check the actual permissions for an authn assumed successful. To make errors more explicit, we add a `then` check that the "authentication succeeds". See: #2634
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
We only assessed if an authentication attempt failed, while tried to check the actual permissions for an authn assumed successful. To make errors more explicit, we add a `then` check that the "authentication succeeds". See: #2634
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
We only assessed if an authentication attempt failed, while tried to check the actual permissions for an authn assumed successful. To make errors more explicit, we add a `then` check that the "authentication succeeds". See: #2634
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
We only assessed if an authentication attempt failed, while tried to check the actual permissions for an authn assumed successful. To make errors more explicit, we add a `then` check that the "authentication succeeds". See: #2634
Since our eauth module for Salt doesn't accept Basic auth anymore, we remove this logic from the storage-operator entirely. Since we don't plan on adding this support back, nor on adding other authentication methods (everything should work with Bearer tokens), we also remove the multi-method handling from our eauth module. See: #2634 See: #2642
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
We only assessed if an authentication attempt failed, while tried to check the actual permissions for an authn assumed successful. To make errors more explicit, we add a `then` check that the "authentication succeeds". See: #2634
We only assessed if an authentication attempt failed, while tried to check the actual permissions for an authn assumed successful. To make errors more explicit, we add a `then` check that the "authentication succeeds". See: #2634
Since our eauth module for Salt doesn't accept Basic auth anymore, we remove this logic from the storage-operator entirely. Since we don't plan on adding this support back, nor on adding other authentication methods (everything should work with Bearer tokens), we also remove the multi-method handling from our eauth module. See: #2634 See: #2642
The fact that any authenticated user can use the SelfSubjectAccessReview API was not self-evident. We add a comment to clarify this. See: #2634
We only need this token when interacting with K8s API, and it's already stored in LocalStorage. We don't need to save it in the Redux store. See: #2634
Component: salt
What happened:
When implementing #2625, we found that forgetting the permission for a newly added Salt execution function (namely,
metalk8s_volumes.device_name) didn't preventstorage-operatorfrom using it, which is a serious security hole.This appeared in 2.5.1-dev, whereas the RBAC was properly enforced in 2.6.0-dev.
What was expected:
Without the relevant permissions, one shouldn't be able to use SaltAPI freely.
If there is a hole in our setup in 2.5 around RBAC with Salt(API), we need it fixed ASAP.
Steps to reproduce: TBD
Resolution proposal (optional): TBD (we need a reproducing example, and tests to ensure this behaviour is fixed)
The text was updated successfully, but these errors were encountered: