-
Notifications
You must be signed in to change notification settings - Fork 459
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
Add Security section to enhancement template #1621
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cff4487
to
0432ab8
Compare
Anything more we want to do on this update or shall we get this merged? |
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale Anyone any objection to merging this? Can I get some labels please? |
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. How about enforcing the bits that are easy to enforce.
- Which service accounts will be affected by the change, what permissions will the feature code need? | ||
- Will end users need to make any security considerations when enabling this feature? | ||
- What is the scope of the required RBAC? | ||
- Are cluster wider permissions required? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a thing we want to control or know, we should add an e2e test/registry similar to the certificate ownerships that requires a high level ACK. The list can determine if the source CVO, any serviceaccount from a namespace created by CVO or created by a serviceaccount in a namespace created by the CVO (etc).
Asking without enforcement is hollow and this one is easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat idea, I wonder how many would fail this test right now.
Off the top of my head, and in my own areas, I know both CCMs and the operator would, the CAPI operator would. I suspect many have reasons to, but I can look into this
guidelines/enhancement_template.md
Outdated
- Will end users need to make any security considerations when enabling this feature? | ||
- What is the scope of the required RBAC? | ||
- Are cluster wider permissions required? | ||
- Should RBAC be limited to only the namespace(s) where the feature is enabled? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ask a question you only want a "yes" to. Why not instead ask if this one is an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworded
guidelines/enhancement_template.md
Outdated
- What is the scope of the required RBAC? | ||
- Are cluster wider permissions required? | ||
- Should RBAC be limited to only the namespace(s) where the feature is enabled? | ||
- Should RBAC be limited to just the node(s) where the feature is enabled? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't a capability that exists in kube.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this one needs rewording, but the sentiment here was about nodes writing to other nodes. I was thinking about things like the MCD and their new MCN construct. They are having to add new permissions to nodes (not Kubelet) to allow something on each node to update something within the cluster, that could be widely scoped. I want people to think about how to narrow that scope
guidelines/enhancement_template.md
Outdated
- Are cluster wider permissions required? | ||
- Should RBAC be limited to only the namespace(s) where the feature is enabled? | ||
- Should RBAC be limited to just the node(s) where the feature is enabled? | ||
- Are we adding new permissions or escalation paths from Nodes to the Cluster level? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
being more specific about: can a token on node/A be used to read or write information about a different node would likely help the answers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an extra "i.e."
guidelines/enhancement_template.md
Outdated
|
||
Principles to consider: | ||
- The permissions you grant should be well scoped and follow the principle of least privilege. Do not use wildcard permissions. | ||
- If the feature operates at the host level, the host root user should not be able to escalate to the cluster level. The feature may only modify resources directly related to the node it operates on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link to the example in kubernetes/kubernetes#124711
|
||
### Security Context | ||
|
||
What security context will the new component run with? What is the impact of the security context on the security posture of the cluster? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We added an e2e test to ensure all components specify which SCC they want. Just an example of something similar to the RBAC bits.
bc554b5
to
2a7f35b
Compare
@JoelSpeed: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This came up in conversation a while back, enhancements do not currently encourage you to think about the security aspects of a feature.
In particular this came up in the context of a feature that deployed a DaemonSet and granted it fairly wide permissions to update Node objects. There had been no discussion within the enhancement doc about limiting the scope of permissions to only the Node on which the daemonset resides.
We should add a new section to the enhancement template to prompt folks to discuss what are the implications of changes, and hopefully document scenarios like above in the future.
CC @derekwaynecarr