Support selinux options/label #157

miaoyq · 2017-08-23T13:39:33Z

Support selinux optios/label

Signed-off-by: Yanqiang Miao [email protected]

/cc @Random-Liu

Random-Liu · 2017-08-23T17:44:50Z

@miaoyq Thanks! We do need this, but haven't got time to work on it. Will review it.

Random-Liu · 2017-08-24T06:04:04Z

Also assign @mikebrow because he's recently working on security related stuff. :)

Random-Liu · 2017-08-25T01:04:55Z

@miaoyq Do you have a selinux environment? Have you tested this? :)

miaoyq · 2017-08-25T02:19:09Z

@Random-Liu Not yet:pensive:, I will add this part

mikebrow · 2017-08-25T02:29:05Z

I'm rooting for ya!

crosbymichael · 2017-08-29T20:20:10Z

I think the approach is good.

Labels should be generated on the CRI side because you will have to make sure the mount labels match up for various containers in a pod.

I'll add a couple of helper Opts in containerd for setting the labels but I think this is the right approach by generating them here.

Random-Liu · 2017-08-30T05:07:04Z

pkg/server/container_create.go

 	"k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime"

 	containerstore "github.com/kubernetes-incubator/cri-containerd/pkg/store/container"
+	"syscall"


Use golang.org/x/sys/unix and move this to the first group, syscall is deprecated.

Random-Liu · 2017-08-30T05:27:29Z

pkg/server/container_create.go

+
+		if mount.GetSelinuxRelabel() {
+			if err := label.Relabel(src, mountLabel, true); err != nil && err != syscall.ENOTSUP {
+				return fmt.Errorf("relabel failed %s: %v", src, err)


also include mountlabel in the error message.

Random-Liu · 2017-08-30T05:30:03Z

pkg/server/container_create.go


 	securityContext := config.GetLinux().GetSecurityContext()

+	processLabel, mountLabel, err := initSelinuxOpts(securityContext.GetSelinuxOptions())


nit: Create a local variable for securityContext.GetSelinuxOptions(), so that we could make the next line shorter. :p

Please add unit test for this. Basically, just set selinux options, and check whether ProcessSelinuxLabel and LinuxMountLabel are set correctly.

We don't need to add SelinuxRelabel for any mount, so that Relabel won't be actually run in the unit test.

@Random-Liu I have tried to add unit test for this, but the result of test depends on whether
selinux have been enabled, see https://github.com/opencontainers/selinux/blob/4a2974bf1ee960774ffd517717f1f45325af0206/go-selinux/label/label_selinux.go#L28.

@Random-Liu I have add the unit test with build tag selinux.

Random-Liu · 2017-08-30T05:31:05Z

pkg/server/container_create.go

 				securityContext.GetCapabilities(), err)
 		}

-		// TODO(random-liu): [P1] Set selinux options.


I think we should also set this for privileged container, let's keep it consistent with docker. https://github.com/moby/moby/blob/master/daemon/oci_linux.go#L831-L834

Random-Liu · 2017-08-30T05:39:03Z

pkg/server/helpers.go

 }
+
+func initSelinuxOpts(selinuxOpt *runtime.SELinuxOption) (string, string, error) {
+	var labelOpts []string


nit: Move this down.

And this seems cleaner https://github.com/opencontainers/selinux/blob/63bd19516561f1973f86c9f777b263ee1e0db140/go-selinux/selinux.go#L572

label.InitLabels(label.DupSecOpt("user:role:type:level")).

Random-Liu · 2017-08-30T05:40:16Z

pkg/server/sandbox_run.go


-	// TODO(random-liu): [P1] Apply SeLinux options.
+	securityContext := config.GetLinux().GetSecurityContext()
+


nit: unnecessary empty line.

Random-Liu · 2017-08-30T05:40:24Z

pkg/server/sandbox_run.go

+	if err != nil {
+		return nil, fmt.Errorf("failed to init selinux options %+v: %v", securityContext.GetSelinuxOptions(), err)
+	}
+


nit: ditto.

Random-Liu · 2017-08-30T05:55:44Z

@miaoyq Please rebase the PR and address comments.

We don't have selinux test and test environment for CRI validation test.
We do have cluster e2e test for it, but we are not running them in any of our own environment https://github.com/kubernetes/kubernetes/blob/master/test/e2e/node/security_context.go#L98-L108.

If possible, could you try to verify this in a selinux environment? If it's hard for you, please file an issue, and we could get back to verify this later.

miaoyq · 2017-08-30T15:58:07Z

@Random-Liu Thanks for review!
Sorry for my late reply, I was busy with other things.
Now I'm free. I'll rebase the PR and address comments.
It's hard for you to verify this actually, but I have built a selinux environment, if it is not urgent, I would like to complete this verification with your guidance. :-)

Random-Liu · 2017-08-30T17:25:20Z

@miaoyq Thanks! Feel free to comment, file issue or ping me if you encountered any problem on that distro.

miaoyq · 2017-08-31T03:06:12Z

@Random-Liu OK, I will, thanks!

Support selinux optios/label Signed-off-by: Yanqiang Miao <[email protected]>

miaoyq · 2017-08-31T11:36:30Z

@Random-Liu Rebased and addressed comments, PTAL

Random-Liu · 2017-08-31T23:30:19Z

LGTM. @miaoyq Please help validate this after merged. Thanks! :)

Support selinux options/label

k8s-ci-robot requested a review from Random-Liu August 23, 2017 13:39

k8s-ci-robot added the cncf-cla: yes label Aug 23, 2017

Random-Liu assigned Random-Liu and mikebrow Aug 24, 2017

Random-Liu reviewed Aug 30, 2017

View reviewed changes

Random-Liu mentioned this pull request Aug 30, 2017

CRI-Containerd Missing Pieces #62

Closed

42 tasks

Support selinux options/label

0c3304e

Support selinux optios/label Signed-off-by: Yanqiang Miao <[email protected]>

miaoyq force-pushed the apply-selinux-opt branch from 45f72b0 to 0c3304e Compare August 31, 2017 11:22

k8s-ci-robot added the size/XXL label Aug 31, 2017

Random-Liu added the lgtm label Aug 31, 2017

Random-Liu merged commit 9c49624 into containerd:master Aug 31, 2017

Random-Liu mentioned this pull request Aug 31, 2017

Verify Selinux support. #199

Closed

miaoyq deleted the apply-selinux-opt branch September 1, 2017 00:19

lanchongyizu pushed a commit to lanchongyizu/cri-containerd that referenced this pull request Sep 3, 2017

Merge pull request containerd#157 from miaoyq/apply-selinux-opt

940de33

Support selinux options/label


		securityContext := config.GetLinux().GetSecurityContext()

		processLabel, mountLabel, err := initSelinuxOpts(securityContext.GetSelinuxOptions())


		// TODO(random-liu): [P1] Apply SeLinux options.
		securityContext := config.GetLinux().GetSecurityContext()

Support selinux options/label #157

Support selinux options/label #157

Uh oh!

Conversation

miaoyq commented Aug 23, 2017

Uh oh!

Random-Liu commented Aug 23, 2017

Uh oh!

Random-Liu commented Aug 24, 2017

Uh oh!

Random-Liu commented Aug 25, 2017

Uh oh!

miaoyq commented Aug 25, 2017

Uh oh!

mikebrow commented Aug 25, 2017

Uh oh!

crosbymichael commented Aug 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Random-Liu commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miaoyq commented Aug 30, 2017

Uh oh!

Random-Liu commented Aug 30, 2017

Uh oh!

miaoyq commented Aug 31, 2017

Uh oh!

miaoyq commented Aug 31, 2017

Uh oh!

Random-Liu commented Aug 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Random-Liu commented Aug 30, 2017 •

edited

Loading