Additional Seccomp tests #137

mikebrow · 2017-09-15T18:57:17Z

Adding support for seccomp testing.

Will add pod testing after validating these container tests.

Random-Liu · 2017-09-15T22:23:50Z

@mikebrow According to #135, seccomp is not supported in the current version of Docker we are using.

I'll review and get that PR merged first, and you could see whether your test overlaps with that one or not. And we could consolidate.

mikebrow · 2017-09-16T00:59:47Z

@feiskyer Wish I'd known someone else was also drafting seccomp tests :-)

mikebrow · 2017-09-16T01:54:40Z

@Random-Liu @feiskyer all good, I don't see any overlap in our test cases. I'll normalize and combine them on Monday. For example, he's using chmod and grep which should work with docker/default and they do not require caps to be set before generating the default seccomp profile (which from my investigation is different with and without certain caps and on different platforms). I'm using hostname which requires caps to make docker/default allow sethostname, so I'm testing with and without caps on to cover both fail and pass cases. I'm also testing the privileged case to make sure you can't block a command when privileged, and i've got a test for runtime/default as that may also be used instead of docker/default. And I just added a test to make sure the profile name is in the format "localhost/*"

feiskyer · 2017-09-16T11:21:34Z

@mikebrow Sorry for didn't let you know this. But cool with more complete test cases.

feiskyer · 2017-09-18T00:42:00Z

@mikebrow needs a rebase now

mikebrow · 2017-09-18T22:44:09Z

Rebased.

@feiskyer I see there was a change to the CRI api documentation

kubernetes/kubernetes@c242432#diff-df996ad3ce05a000c2009d6e8cfba507L486

Talked to @Random-Liu about this. Will just remove the runtime/default tests for now.

mikebrow · 2017-09-18T23:13:55Z

@Random-Liu @feiskyer rebased. Tests passing ready for review.

feiskyer · 2017-09-19T01:39:00Z

pkg/validate/security_context.go

@@ -402,6 +424,41 @@ var _ = framework.KubeDescribe("Security Context", func() {

 			checkNetworkManagement(rc, containerID, false)
 		})
+	})
+
+	Context("runtime should support seccomp security context", func() {


nits: [security context] same with other places.

the top level bucket is "[k8s.io] security context"
This context is for seccomp tests it is already within [k8s.io] security context bucket.

No need to be repeating the phrase security context 2-3 times one at each level in the buckets.

Let me refactor all of the titles making it easier to use focus and skip regexps and only having brackets on the left for ease of reading.

@feiskyer On second thought, I also think it is fine to remove the tag given how cri validation test is organized today. :)

Signed-off-by: Mike Brown <brownwm@us.ibm.com> adds support for seccomp testing Signed-off-by: Mike Brown <brownwm@us.ibm.com>

mikebrow · 2017-09-19T15:23:46Z

@feiskyer I refactored the context container descriptions to fix regexp options and make it easier to read. I also removed repetitive text from the suite descriptions, again to make it easier to read and easier to identify regexp options. Nit addressed.

feiskyer · 2017-09-20T00:41:25Z

@mikebrow thanks 👍

feiskyer · 2017-09-20T00:41:31Z

/lgtm

Random-Liu · 2017-09-20T00:42:41Z

@feiskyer I'm still reviewing. There are issues. :p

feiskyer · 2017-09-20T00:43:42Z

hmm, didn't notice issues. we could open another fix pr for the issues.

mikebrow · 2017-09-20T00:49:41Z

@feiskyer @Random-Liu There was an offline discussion (15min ago) about adding another context to separate docker/default tests. I'll push a pr for that in the morn...

Random-Liu · 2017-09-20T00:00:13Z

pkg/validate/security_context.go

@@ -225,7 +247,7 @@ var _ = framework.KubeDescribe("Security Context", func() {
 		})
 	})

-	Context("runtime should support container with security context", func() {
+	Context("bucket", func() {


What do you mean by "bucket"?

a bucket of tests... This is the second "bucket", bucket-2 would be another possible name. This context is already within the "Security Context" .. the other buckets at this level are NamespaceOption and SeccompProfilePath.

This bucket comprises tests for: SupplementalGroups, RunAsUser, RunAsUserName, ReadOnlyRootfs, Privileged, and Capability. I couldn't come up with a better name for these than the "Security Context bucket"

Random-Liu · 2017-09-20T00:01:59Z

pkg/validate/security_context.go

@@ -402,8 +424,43 @@ var _ = framework.KubeDescribe("Security Context", func() {

 			checkNetworkManagement(rc, containerID, false)
 		})
+	})
+
+	Context("SeccompProfilePath", func() {


Probably just Seccomp?

I like using the litteral CRI spec name here as it doesn't test all of Seccomp here. It is literally testing the CRI spec defined options for the SeccompProfilePath:

// LinuxSandboxSecurityContext holds linux security configuration that will be // applied to a sandbox. Note that: // 1) It does not apply to containers in the pods. // 2) It may not be applicable to a PodSandbox which does not contain any running // process. type LinuxSandboxSecurityContext struct { // Configurations for the sandbox's namespaces. // This will be used only if the PodSandbox uses namespace for isolation. NamespaceOptions *NamespaceOption `protobuf:"bytes,1,opt,name=namespace_options,json=namespaceOptions" json:"namespace_options,omitempty"` // Optional SELinux context to be applied. SelinuxOptions *SELinuxOption `protobuf:"bytes,2,opt,name=selinux_options,json=selinuxOptions" json:"selinux_options,omitempty"` // UID to run sandbox processes as, when applicable. RunAsUser *Int64Value `protobuf:"bytes,3,opt,name=run_as_user,json=runAsUser" json:"run_as_user,omitempty"` // If set, the root filesystem of the sandbox is read-only. ReadonlyRootfs bool `protobuf:"varint,4,opt,name=readonly_rootfs,json=readonlyRootfs,proto3" json:"readonly_rootfs,omitempty"` // List of groups applied to the first process run in the sandbox, in // addition to the sandbox's primary GID. SupplementalGroups []int64 `protobuf:"varint,5,rep,packed,name=supplemental_groups,json=supplementalGroups" json:"supplemental_groups,omitempty"` // Indicates whether the sandbox will be asked to run a privileged // container. If a privileged container is to be executed within it, this // MUST be true. // This allows a sandbox to take additional security precautions if no // privileged containers are expected to be run. Privileged bool `protobuf:"varint,6,opt,name=privileged,proto3" json:"privileged,omitempty"` // Seccomp profile for the sandbox, candidate values are: // * docker/default: the default profile for the docker container runtime // * unconfined: unconfined profile, ie, no seccomp sandboxing // * localhost/<full-path-to-profile>: the profile installed on the node. // <full-path-to-profile> is the full path of the profile. // Default: "", which is identical with unconfined. SeccompProfilePath string `protobuf:"bytes,7,opt,name=seccomp_profile_path,json=seccompProfilePath,proto3" json:"seccomp_profile_path,omitempty"` }

Random-Liu · 2017-09-20T00:08:43Z

pkg/validate/security_context.go

@@ -37,6 +38,27 @@ import (

 const (
 	nginxContainerImage string = "nginx"
+	localhost           string = "localhost/"
+	// profile which denies sethostname syscall
+	seccompBlockHostNameProfile = `{


Why not moving those 2 consts into the Seccomp context. No one else is using it.

sure I'll move..

Random-Liu · 2017-09-20T00:10:02Z

pkg/validate/security_context.go

+
+		BeforeEach(func() {
+			profileDir, err = createSeccompProfileDir()
+			if err != nil {


Don't think we could just log and return, it will still run test then.
You should do assertion here Expect(err).NotTo(HaveOccurred()).

@mikebrow This is the issue we need to fix.

agreed will update ..

Random-Liu · 2017-09-20T00:47:47Z

pkg/validate/security_context.go

+			checkSetHostname(rc, containerID, true)
+		})
+
+		It("runtime should block setting hostname with no seccomp profile specified", func() {


no seccomp profile means "unconfined", why set hostname will be blocked? By whom?
Default capabilities? If so, I feel like this is more a capability test instead of seccomp test.

WE can add a test to create a default container, and check whether seccomp is enabled. That is useful.

This is a duplicate test, I'll delete it.

Random-Liu · 2017-09-20T00:50:21Z

pkg/validate/security_context.go

+			checkSetHostname(rc, containerID, false)
+		})
+
+		It("runtime should support setting hostname with docker/default seccomp profile and sysCaps", func() {


Move this to a separate docker/default context.

Yes, we both had that idea at the same time :-)

Random-Liu · 2017-09-20T00:50:26Z

pkg/validate/security_context.go

+			checkSetHostname(rc, containerID, true)
+		})
+
+		It("runtime should block sethostname with docker/default seccomp profile and no sysCaps", func() {


Random-Liu · 2017-09-20T00:51:42Z

pkg/validate/security_context.go

+			checkSetHostname(rc, containerID, false)
+		})
+
+		It("runtime should block sethostname with nil seccomp profile and no sysCaps", func() {


Same. This is a default capability test, not seccomp test.

see above response to your comments.. maybe we should do a quick call on this.

Random-Liu · 2017-09-20T00:52:34Z

pkg/validate/security_context.go

+			checkSetHostname(rc, containerID, false)
+		})
+
+		It("runtime should block sethostname with unconfined seccomp profile and no sysCaps", func() {


Same. This is a default capability test, not seccomp test.

it's both a default caps test and a seccomp test. See unconfined and default rules below:

// Seccomp profile for the sandbox, candidate values are: // * docker/default: the default profile for the docker container runtime // * unconfined: unconfined profile, ie, no seccomp sandboxing // * localhost/<full-path-to-profile>: the profile installed on the node. // <full-path-to-profile> is the full path of the profile. // Default: "", which is identical with unconfined. SeccompProfilePath string `protobuf:"bytes,7,opt,name=seccomp_profile_path,json=seccompProfilePath,proto3" json:"seccomp_profile_path,omitempty"`

If you have an idea for a better way to test the unconfined and nil SeccompProfilePath options.. I'm all ears :)

Random-Liu · 2017-09-20T00:55:17Z

@mikebrow Please address the comments.

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

Responding to comments on #137

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 15, 2017

mikebrow force-pushed the seccomp-test branch from 8a05047 to 32c8e24 Compare September 15, 2017 19:08

mikebrow force-pushed the seccomp-test branch 2 times, most recently from 2c9f66d to edebeb8 Compare September 16, 2017 02:58

mikebrow force-pushed the seccomp-test branch from edebeb8 to 8da211a Compare September 18, 2017 21:37

mikebrow force-pushed the seccomp-test branch from 8da211a to 3c2a89b Compare September 18, 2017 22:48

mikebrow changed the title ~~[WIP] Seccomp test~~ Additional Seccomp tests Sep 18, 2017

feiskyer reviewed Sep 19, 2017

View reviewed changes

feiskyer mentioned this pull request Sep 19, 2017

critest: experimental features #24

Closed

3 tasks

adds tests for seccomp

1590cb7

Signed-off-by: Mike Brown <brownwm@us.ibm.com> adds support for seccomp testing Signed-off-by: Mike Brown <brownwm@us.ibm.com>

mikebrow force-pushed the seccomp-test branch from 3c2a89b to 1590cb7 Compare September 19, 2017 15:18

feiskyer merged commit 046c028 into kubernetes-sigs:master Sep 20, 2017

Random-Liu reviewed Sep 20, 2017

View reviewed changes

mikebrow added a commit to mikebrow/cri-tools that referenced this pull request Sep 20, 2017

respond to comments on kubernetes-sigs#137

00524d1

Signed-off-by: Mike Brown <brownwm@us.ibm.com>

mikebrow mentioned this pull request Sep 20, 2017

Responding to comments on #137 #143

Merged

feiskyer added a commit that referenced this pull request Sep 21, 2017

Merge pull request #143 from mikebrow/docker-default-context

48f8614

Responding to comments on #137

feiskyer mentioned this pull request Sep 22, 2017

Releasing cri-tools v0.2 #145

Closed

3 tasks

Additional Seccomp tests #137

Additional Seccomp tests #137

Conversation

mikebrow commented Sep 15, 2017

Random-Liu commented Sep 15, 2017 • edited Loading

mikebrow commented Sep 16, 2017 • edited Loading

mikebrow commented Sep 16, 2017 • edited Loading

feiskyer commented Sep 16, 2017

feiskyer commented Sep 18, 2017

mikebrow commented Sep 18, 2017

mikebrow commented Sep 18, 2017

Choose a reason for hiding this comment

mikebrow Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow commented Sep 19, 2017 • edited Loading

feiskyer commented Sep 20, 2017

feiskyer commented Sep 20, 2017

Random-Liu commented Sep 20, 2017

feiskyer commented Sep 20, 2017

mikebrow commented Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

mikebrow Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Sep 20, 2017

Random-Liu commented Sep 15, 2017 •

edited

Loading

mikebrow commented Sep 16, 2017 •

edited

Loading

mikebrow commented Sep 16, 2017 •

edited

Loading

mikebrow Sep 19, 2017 •

edited

Loading

mikebrow commented Sep 19, 2017 •

edited

Loading

mikebrow commented Sep 20, 2017 •

edited

Loading

mikebrow Sep 20, 2017 •

edited

Loading

mikebrow Sep 20, 2017 •

edited

Loading

mikebrow Sep 20, 2017 •

edited

Loading