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

Chore: Tighten RBAC permissions #1954

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Chore: Tighten RBAC permissions #1954

merged 1 commit into from
Jun 21, 2022

Conversation

ellistarn
Copy link
Contributor

@ellistarn ellistarn commented Jun 17, 2022

Fixes #973

Description

Tighten RBAC permissions

How was this change tested?

  • make test
  • make apply
  • checked controller/webhook logs

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

Release Note

None

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ellistarn ellistarn requested a review from a team as a code owner June 17, 2022 19:31
@ellistarn ellistarn requested a review from spring1843 June 17, 2022 19:31
@netlify
Copy link

netlify bot commented Jun 17, 2022

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit bdc293f
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/62b1fc4f33b02600095e8f8d

- apiGroups: ["admissionregistration.k8s.io"]
resources: ["validatingwebhookconfigurations", "mutatingwebhookconfigurations"]
verbs: ["get", "watch", "list"]
# Write
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I like that we're separating out the Read and Write permissions!

Comment on lines 17 to 19
- apiGroups: [""]
resources: ["namespaces"]
resources: ["configmaps", "namespaces", "secrets"]
verbs: ["get", "list", "watch"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we scope this down to the specific ConfigMaps (logging, batchingConfig) and Secrets that we need via resourceNames ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe you can't restrict list requests by resource name, but we need list/watch for the informer. Since these are read permissions, I think this is acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can't restrict list, can we atleast restrict get and watch?

Copy link
Contributor Author

@ellistarn ellistarn Jun 21, 2022

Choose a reason for hiding this comment

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

Created the role

  - apiGroups: [""]
    resources: ["configmaps"]
    verbs: ["get", "list", "watch"]
    resourceNames:
      - karpenter-global-settings
      - karpenter-leader-election
      - config-logging
karpenter-6746fb6c66-m7ttl controller E0621 17:29:47.216205       1 reflector.go:138] k8s.io/[email protected]/tools/cache/reflector.go:167: Failed to watch *v1.ConfigMap: failed to list *v1.ConfigMap: configmaps is forbidden: User "system:serviceaccount:karpenter:karpenter" cannot list resource "configmaps" in API group "" in the namespace "karpenter"

Get is subsumed by list, so I don't see the value in restricting get.

njtran
njtran previously approved these changes Jun 20, 2022
Copy link
Contributor

@njtran njtran left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

lgtm

@ellistarn ellistarn merged commit eab7dea into aws:main Jun 21, 2022
@ellistarn ellistarn deleted the rbac branch June 21, 2022 19:56
@satishweb
Copy link

Chart Version: 0.12.0

E0622 06:03:29.093779       1 leaderelection.go:329] error initially creating leader election record: configmaps is forbidden: User "system:serviceaccount:xxxxx-karpenter:xxxxx-karpenter" cannot create resource "configmaps" in API group "" in the namespace "xxxxxxx-karpenter"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RBAC is too permissive
5 participants