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.

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

Handle secret read clusterroles #8871

Merged
merged 3 commits into from
Mar 4, 2025
Merged

Handle secret read clusterroles #8871

merged 3 commits into from
Mar 4, 2025

Conversation

mikkeloscar
Copy link
Contributor

@mikkeloscar mikkeloscar commented Feb 4, 2025

Improves handling of secret read cluster roles in EKS clusters

The implementation is described as follows:

  1. Extend admission-controller with a clusterrole admitter which detects clusterroles that defined secret read permissions ({ApiGroups: [""], Resources:["secrets"],Verbs:["get|list|watch"]}). If detected, it extracts the permissions from the role and moves them to an annotation: clusterroles.admission-controller.zalando.org/secret-read-verbs: "get,list,watch". This achieves two things 1) Users can't deploy clusterroles with secret read permissions because those permissions will be removed during admission. 2) The role gets annotated such that it can be detected if the users intended to have read secret permissions. This is implemented in admission-controller: Update to version master-244 #8987
  2. Extend the role-sync-controller to look for clusterroles withs the annotation: clusterroles.admission-controller.zalando.org/secret-read-verbs: "get,list,watch" and resolve the clusterrolebindings binding to those roles. With the bindings we get a list of subjects that want read secret permissions at cluster level and those subjects can be added to the per namespace secrets role bindings. This is implemented in role-sync-controller: Update to version main-7 #8986
  3. Allow read secret permissions at the cluster level for poweruser/CDP. This effectively allows users to apply a clusterrole with read secret permissions, but because of 1 and 2, the clusterrole is not applied in that way but the subject(s) binding to the clusterrole will instead bind to the per namespace secret reader roles and the users get effectively the same permissions as in non-EKS, without changing their manifests.
  4. Lastly, to prevent powerusers from reading from kube-system we remove secret read permissions from the cluster-role and only attach those permissions to the cdp/deployment-service service accounts. This means a poweruser, user, e.g. Manual access, can't create a cluster-role with read secret permissions. This is however still possible via CDP.

TODO

  • Add e2e covering this feature.

@mikkeloscar mikkeloscar added the bugfix Bug fixes and patches, e.g. fixing of a production issue that is affecting the customer experience. label Feb 4, 2025
@mikkeloscar mikkeloscar force-pushed the eks-secret-read-fixes branch 10 times, most recently from 8e73869 to 03e4229 Compare February 26, 2025 16:05
@mikkeloscar mikkeloscar changed the title [WIP] Handle secret read clusterroles Handle secret read clusterroles Feb 26, 2025
@mikkeloscar mikkeloscar reopened this Feb 26, 2025
@mikkeloscar mikkeloscar force-pushed the eks-secret-read-fixes branch 2 times, most recently from 569001c to 07c1c80 Compare February 28, 2025 10:38
@demonCoder95
Copy link
Member

Add e2e covering this feature

Don't the admission-controller unit tests cover this already? What kind of coverage are you thinking about for e2e tests? 🤔

@mikkeloscar mikkeloscar force-pushed the eks-secret-read-fixes branch from 5716875 to 52bb6e5 Compare February 28, 2025 15:11
@mikkeloscar
Copy link
Contributor Author

mikkeloscar commented Feb 28, 2025

Add e2e covering this feature

Don't the admission-controller unit tests cover this already? What kind of coverage are you thinking about for e2e tests? 🤔

Nvm, the role-sync-controller was not working correctly making some e2e fail. This was fixed in #9005

There is one thing which is not covered by e2e, will track it in an internal issue:

With this change the cdp and deployment-service can read secrets from kube-system because they need this access to allow users to deploy read secret cluster roles. This can not be defined as a generic test as it would fail on non-eks where it's not allowed.

Signed-off-by: Mikkel Oscar Lyderik Larsen <[email protected]>
Signed-off-by: Mikkel Oscar Lyderik Larsen <[email protected]>
@mikkeloscar mikkeloscar force-pushed the eks-secret-read-fixes branch from 52bb6e5 to b592364 Compare March 3, 2025 19:30
@mikkeloscar
Copy link
Contributor Author

👍

@linki
Copy link
Member

linki commented Mar 4, 2025

👍

1 similar comment
@mikkeloscar
Copy link
Contributor Author

👍

@mikkeloscar mikkeloscar merged commit b624802 into eks Mar 4, 2025
10 checks passed
@mikkeloscar mikkeloscar deleted the eks-secret-read-fixes branch March 4, 2025 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fixes and patches, e.g. fixing of a production issue that is affecting the customer experience. internal/merges-tagged merged/alpha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants