Add validation for NoNewPrivs #141
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
LGTM |
Random-Liu
added
the
lgtm
pkg/validate/security_context.go
Outdated
| }, | ||
| LogPath: fmt.Sprintf("%s.log", name), | ||
| } | ||
| if noNewPrivs != nil { |
There was a problem hiding this comment.
I don't understand the nil test. The spec doesn't use *bool.. so nil isn't an option. Will be false or true.
There was a problem hiding this comment.
This is for testing the default behavior. Do you think we should remove this case?
pkg/validate/security_context.go
Outdated
| verifyLogContents(podConfig, fmt.Sprintf("%s.log", name), expectedLog) | ||
| } | ||
|
|
||
| It("should allow privilege escalation when not explicitly set and uid != 0", func() { |
There was a problem hiding this comment.
same test as below below since unset isn't an option. If the CRI guys want unset let's see if it's not to late to change it to *bool.
There was a problem hiding this comment.
@mikebrow yep, default is equal to false.
There was a problem hiding this comment.
So yeah, since it's not a *bool, nil isn't an option. Go is one of those languages where a bool starts out life as a false, an int starts with the value 0, a float 0.0 etc.. I'd just change the internal api parameter to a bool instead of *bool.. and remove the nil test..
Cheers!
feiskyer
force-pushed
the
no-new-privs-validation
branch
from
bc740dc to
d59dc4c
Compare
|
@mikebrow Addressed comments. PTAL. |
| By(fmt.Sprintf("create container %s", name)) | ||
| containerConfig := &runtimeapi.ContainerConfig{ | ||
| Metadata: framework.BuildContainerMetadata(name, framework.DefaultAttempt), | ||
| Image: &runtimeapi.ImageSpec{Image: noNewPrivsImage}, |
There was a problem hiding this comment.
I think you either lost your noNewPrivsImage in a rebase or maybe while cut and pasting the constant to move it to this context?
But this looks good to me.
There was a problem hiding this comment.
hmm, lost in the rebase. Fixed.
feiskyer
force-pushed
the
no-new-privs-validation
branch
from
d59dc4c to
b091a22
Compare
|
Seems good to go? @feiskyer Feel free to merge it. |
Part of #131.