Add RBAC good practice guide #32812
Add RBAC good practice guide #32812
Conversation
|
| Name | Link |
|---|---|
| 4125718 | |
| https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6281586bcd0ab40009e76b86 | |
| https://deploy-preview-32812--kubernetes-io-main-staging.netlify.app | |
Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
| @@ -0,0 +1,101 @@ | |||
| --- | |||
| reviewers: | |||
| title: RBAC Good Practices | |||
There was a problem hiding this comment.
I'm wondering if this topic should be put under the 'reference' section.
There was a problem hiding this comment.
I like having it in Concepts. To me that's a closer fit than Reference: reference information should always be objective, whereas good practice is often subjective.
There was a problem hiding this comment.
I'm fine with either but yep I can see this being more concepts
|
If this is a draft, it is best to either mark it as a draft using the relevant GitHub API (there is a button for that in the web GUI) or to start the PR title with [WIP] . |
Thanks for the work on this. It's very useful.
In terms of wording, here are a bunch of suggestions. I'm afraid this isn't a full review of the page, but it is something that I hope will move this PR towards its next review and further refinement.
| @@ -0,0 +1,101 @@ | |||
| --- | |||
| reviewers: | |||
| title: RBAC Good Practices | |||
There was a problem hiding this comment.
I like having it in Concepts. To me that's a closer fit than Reference: reference information should always be objective, whereas good practice is often subjective.
|
/label tide/merge-method-squash |
|
@sftim think I've hit almost all the review points so far :) On the draft thing I only really meant draft in the sense of (needs this review) rather than thinking the page is incomplete. |
|
/retitle Add RBAC good practice guide This doesn't sound like it's a draft in the sense we'd normally use that term. |
|
@raesene the PR description suggests this is still a draft—is that right? |
|
@sftim Amended the draft bit and thanks for the feedback, good stuff :) |
|
https://deploy-preview-32812--kubernetes-io-main-staging.netlify.app/docs/concepts/security/rbac-good-practices/ looks good. A few nits make it short of an LGTM - if you fix the fragment identifiers (see my last review) @raesene, I'll be happy to LGTM the Markdown. This also wants tech review from SIG Auth and / or SIG Security. |
|
@kubernetes/sig-auth-pr-reviews how's this looking? |
|
Technical content lgtm I still have a slight preference for avoiding new mentions of PodSecurityPolicy, but don't feel that strongly... it would just be one more place to clean up in 1.25 |
|
@sftim What do you think on this one, I think we've got most of the changes needed in place now, just a couple of points to confirm? |
|
@raesene this is looking good. Would you be willing to squash it down to 1 commit and reword that single commit to summarise the change(s)? LGTM |
|
@sftim cool addressed what I think were the last two nits. I've squashed some of the commits down, but for the rest I seem to be getting |
|
Try this (POSIX shell, BTW): cd "$(command git rev-parse --show-toplevel)"
git checkout main
# go back to first commit from this PR
git reset --hard 295b66863a65053b8db1e466724d75f90ed0e5cf
# include subsequent changes
git checkout 36c27fc -- content
git commit --amend # this is where you set a new commit message
# your text editor fires up, save the message once you edited it
# update your fork
# BTW somewhat dangerous IF you were a maintainer given branch name is "main"
git push --force-with-lease That should do it. |
|
Another open PR covers similar ground: #33653 |
|
Hi @sftim I had a shot at that procedure, but getting I can see that commit ID in k/website repo. but not in my fork . Unfortunately my git fu is pretty limited, so I'm not too sure where I'm going wrong there.
|
|
Revised squash steps cd "$(command git rev-parse --show-toplevel)"
git checkout main
# go back to first commit from this PR
git reset --hard 295b66863a65053b8db1e466724d75f90ed0e5cf
# include subsequent changes
git checkout 8842512786f6f1b3bdc66ebe9e669715701610fe -- content/en/docs/concepts/security/rbac-good-practices.md
git commit --amend # this is where you set a new commit message
# your text editor fires up, save the message once you edited it
# update your fork
# BTW somewhat dangerous IF you were a maintainer given branch name is "main"
git push --force-with-lease |
|
@sftim Thanks! that seems to have worked |
|
/remove-label tide/merge-method-squash |
|
Technical LGTM from #32812 (comment) stands |
A couple of nits, but I'd sooner see this merged and a follow-up PR come in to tweak things.
/lgtm
/approve
| can avoid accidental modification of cluster resources. | ||
| - Avoid adding users to the `system:masters` group. Any user who is a member of this group | ||
| bypasses all RBAC rights checks and will always have unrestricted superuser access, which cannot be | ||
| revoked by removing Role Bindings or Cluster Role Bindings. As an aside, if a cluster is |
There was a problem hiding this comment.
(nit)
| revoked by removing Role Bindings or Cluster Role Bindings. As an aside, if a cluster is | |
| revoked by removing RoleBindings or ClusterRoleBindings. As an aside, if a cluster is |
|
|
||
| ### Minimize distribution of privileged tokens | ||
|
|
||
| Ideally, pods shouldn't be assigned service accounts granted powerful permissions (listed [here](#Kubernetes-RBAC---Privilege-Escalation-Risks)). |
There was a problem hiding this comment.
| Ideally, pods shouldn't be assigned service accounts granted powerful permissions (listed [here](#Kubernetes-RBAC---Privilege-Escalation-Risks)). | |
| Ideally, pods shouldn't be assigned service accounts that have been granted powerful permissions | |
| (for example, any of the rights listed under | |
| [privilege escalation risks](#privilege-escalation-risks)). |
|
LGTM label has been added. Git tree hash: d882fa3e1f225ba3057a39c88b2246fa1461c29f
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sftim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
RBAC Good Practice guide as discussed in SIG-Security-Docs and SIG-Security (issue kubernetes/sig-security#41).
[Preview]
cc @savitharaghunathan @reylejano