-
Notifications
You must be signed in to change notification settings - Fork 456
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 new test to critest for privileged container #377
Conversation
@mrunalp PTAL |
pkg/validate/security_context.go
Outdated
Expect(err).NotTo(HaveOccurred(), msg) | ||
|
||
if privileged { | ||
Expect(string(stdout)).To(ContainSubstring("system_r:spc_t")) |
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.
not sure all this is going to work on non rhel systems (like fedora).
btw, I was thinking about having 2 separate tests:
- mount label: just have a cmd for the pod that does
ls -ldZ /
and make sure level is applied even if privileged - process label: just have a cmd that
cat /proc/self/attr/current
all of that to avoid having pods running top
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.
For mount_label we can grep for it in /proc/1/mountinfo.
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.
The tests failed on Travis (based on Ubuntu):
Expected
<string>: lrwxrwxrwx 1 root root ? 7 Feb 7 2018 bin -> usr/bin
to contain substring
<string>: object_r:container_file_t
/home/travis/gopath/src/github.com/kubernetes-sigs/cri-tools/pkg/validate/security_context.go:1161
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.
The labels work fine on my fedora 28. Also split the it into 2 separate tests.
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.
ubuntu doesn't have selinux enabled/installed by default for sure
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.
@runcom I moved the test to selinux_linux.go, which checks if selinux is enabled before running the tests. That should be good right?
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.
yup yup! sorry overlooked that 👍
pkg/validate/security_context.go
Outdated
@@ -451,6 +451,24 @@ var _ = framework.KubeDescribe("Security Context", func() { | |||
checkNetworkManagement(rc, containerID, notPrivileged) | |||
}) | |||
|
|||
It("selinux mount label should persist when container is privileged", func() { |
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.
could you move this case to selinux_linux.go
?
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.
Moved.
Test ensures that when a privileged container is run, the mount label persists and is in the right format. Also checks the process label. Signed-off-by: Urvashi Mohnani <[email protected]>
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, umohnani8 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 |
Test ensures that when a privileged container is run,
the mount label persists and is in the right format.
Also checks the process label.
Adds test for cri-o/cri-o#1781
Signed-off-by: Urvashi Mohnani [email protected]