create SCCExecRestriction admission plugin #4755
Conversation
|
@pmorie since I associate the Pauls together |
|
Otherwise it's hard to debug admin controllers |
|
Upstream, this admission controller has already changed to limit on three attributes of pods: privileged, host pid, and host IPC. |
Yes, ideally, if you can create the pod you should have privs to exec/attach to the pod. |
Alright, I'd say that's an argument to keep this as an upstream carry to make sure we stay in sync. |
|
This looks fine to me. My only concern is that it's just another upstream patch we're carrying indefinitely. I would rather see the OpenShift code not register the upstream denial plugin and have a custom exec/attach plugin that first checks if you would have permissions to create the pod and deny based on that. That is significantly more work and (in discussions with @deads2k) also has the risk of drift from upstream. Ie. pull the pod definition, run it through the constraint plugin's Admit(), accept/deny based on errors. |
|
Rather than a carry patch, we could have a function hook for custom allow/deny behavior with a default implementation of using the booleans in the admission controller. That would let us plug in our own allow behavior. |
A plugin for our plugin? Turtles all the way down! I don't think that helps with staying in sync. |
|
just subclass it. oh, wait |
deads2k
force-pushed
the
privileged-pods
branch
from
2c10280
to
77fc9d0
Compare
deads2k
changed the title
|
@pweil- updated to new admission controller [test] |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5145/) |
| pod.Spec.ServiceAccountName = "" | ||
| createAttributes := admission.NewAttributesRecord(pod, "pods", a.GetNamespace(), a.GetName(), a.GetResource(), a.GetSubresource(), admission.Create, a.GetUserInfo()) | ||
| if err := d.constraintAdmission.Admit(createAttributes); err != nil { | ||
| return admission.NewForbidden(a, err) |
There was a problem hiding this comment.
Not sure if this will be the most useful error to return though it's a start. I definitely think this locks down any use case where you are trying to exec/attach to a pod that you wouldn't have permissions to create though which I like if everyone agrees that that is the right direction.
And if so, unit tests please 😃
deads2k
force-pushed
the
privileged-pods
branch
from
77fc9d0
to
beca326
Compare
|
unit tests added. |
And it allows the ability to exec into pods that you could created (unlike upstream). Based on comments above in the pull, this looks non-contentious |
|
Evaluated for origin test up to beca326 |
|
👍 thanks, and e2e test so double 👍 👍 |
|
Any further comments? |
|
none from me |
Is that as good as an lgtm? |
yes, LGTM |
|
Cancelling the merge. It would conflict with the rebase. |
|
[merge] since the rebase already has conflicts. |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3403/) (Image: devenv-fedora_2405) |
|
Evaluated for origin merge up to beca326 |
Merged by openshift-bot
This allows users who can create privileged pods to exec into privileged pods. It seems to make sense to relax this restriction.
If SCC moves into origin, then this should change to be our own controller that we include instead of modifying an existing one.
@pweil- is this keeping with you thoughts on SCC?