From e490c20c22fffa7c78b9ddd596dcd8605d93aabd Mon Sep 17 00:00:00 2001 From: Paul Weil Date: Mon, 10 Aug 2015 13:30:34 -0400 Subject: [PATCH] add non-root directive to SC and kubelet checking --- api/swagger-spec/v1.json | 4 + pkg/api/deep_copy_generated.go | 1 + pkg/api/types.go | 5 + pkg/api/v1/conversion_generated.go | 2 + pkg/api/v1/deep_copy_generated.go | 1 + pkg/api/v1/types.go | 5 + pkg/kubelet/dockertools/manager.go | 68 +++++++++++++ pkg/kubelet/dockertools/manager_test.go | 126 ++++++++++++++++++++++++ pkg/kubelet/rkt/rkt.go | 2 + pkg/securitycontext/util.go | 21 ++++ pkg/securitycontext/util_test.go | 121 +++++++++++++++++++++++ 11 files changed, 356 insertions(+) diff --git a/api/swagger-spec/v1.json b/api/swagger-spec/v1.json index f87b425e484..9a5b5b7b673 100644 --- a/api/swagger-spec/v1.json +++ b/api/swagger-spec/v1.json @@ -12774,6 +12774,10 @@ "type": "integer", "format": "int64", "description": "the user id that runs the first process in the container; see http://releases.k8s.io/HEAD/docs/design/security_context.md#security-context" + }, + "runAsNonRoot": { + "type": "boolean", + "description": "indicates the container must be run as a non-root user either by specifying the runAsUser or in the image specification" } } }, diff --git a/pkg/api/deep_copy_generated.go b/pkg/api/deep_copy_generated.go index 5044b286574..15ecb80bb5f 100644 --- a/pkg/api/deep_copy_generated.go +++ b/pkg/api/deep_copy_generated.go @@ -1803,6 +1803,7 @@ func deepCopy_api_SecurityContext(in SecurityContext, out *SecurityContext, c *c } else { out.RunAsUser = nil } + out.RunAsNonRoot = in.RunAsNonRoot return nil } diff --git a/pkg/api/types.go b/pkg/api/types.go index 72be0d37c1d..3e5de52b68f 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -2166,6 +2166,11 @@ type SecurityContext struct { // RunAsUser is the UID to run the entrypoint of the container process. RunAsUser *int64 `json:"runAsUser,omitempty"` + + // RunAsNonRoot indicates that the container should be run as a non-root user. If the RunAsUser + // field is not explicitly set then the kubelet may check the image for a specified user or + // perform defaulting to specify a user. + RunAsNonRoot bool } // SELinuxOptions are the labels to be applied to the container. diff --git a/pkg/api/v1/conversion_generated.go b/pkg/api/v1/conversion_generated.go index dc78306a632..2328de7b1b4 100644 --- a/pkg/api/v1/conversion_generated.go +++ b/pkg/api/v1/conversion_generated.go @@ -2002,6 +2002,7 @@ func convert_api_SecurityContext_To_v1_SecurityContext(in *api.SecurityContext, } else { out.RunAsUser = nil } + out.RunAsNonRoot = in.RunAsNonRoot return nil } @@ -4344,6 +4345,7 @@ func convert_v1_SecurityContext_To_api_SecurityContext(in *SecurityContext, out } else { out.RunAsUser = nil } + out.RunAsNonRoot = in.RunAsNonRoot return nil } diff --git a/pkg/api/v1/deep_copy_generated.go b/pkg/api/v1/deep_copy_generated.go index bac49bba915..192b44d48cc 100644 --- a/pkg/api/v1/deep_copy_generated.go +++ b/pkg/api/v1/deep_copy_generated.go @@ -1812,6 +1812,7 @@ func deepCopy_v1_SecurityContext(in SecurityContext, out *SecurityContext, c *co } else { out.RunAsUser = nil } + out.RunAsNonRoot = in.RunAsNonRoot return nil } diff --git a/pkg/api/v1/types.go b/pkg/api/v1/types.go index 10a3e71ec3d..3ea22287f52 100644 --- a/pkg/api/v1/types.go +++ b/pkg/api/v1/types.go @@ -2030,6 +2030,11 @@ type SecurityContext struct { // RunAsUser is the UID to run the entrypoint of the container process. RunAsUser *int64 `json:"runAsUser,omitempty" description:"the user id that runs the first process in the container; see http://releases.k8s.io/HEAD/docs/design/security_context.md#security-context"` + + // RunAsNonRoot indicates that the container should be run as a non-root user. If the RunAsUser + // field is not explicitly set then the kubelet may check the image for a specified user or + // perform defaulting to specify a user. + RunAsNonRoot bool `json:"runAsNonRoot,omitempty" description:"indicates the container must be run as a non-root user either by specifying the runAsUser or in the image specification"` } // SELinuxOptions are the labels to be applied to the container diff --git a/pkg/kubelet/dockertools/manager.go b/pkg/kubelet/dockertools/manager.go index f613b62a49d..85ba77a415c 100644 --- a/pkg/kubelet/dockertools/manager.go +++ b/pkg/kubelet/dockertools/manager.go @@ -1619,6 +1619,15 @@ func (dm *DockerManager) SyncPod(pod *api.Pod, runningPod kubecontainer.Pod, pod continue } + if container.SecurityContext != nil && container.SecurityContext.RunAsNonRoot { + err := dm.verifyNonRoot(container) + dm.updateReasonCache(pod, container, err) + if err != nil { + glog.Errorf("Error running pod %q container %q: %v", kubecontainer.GetPodFullName(pod), container.Name, err) + continue + } + } + // TODO(dawnchen): Check RestartPolicy.DelaySeconds before restart a container namespaceMode := fmt.Sprintf("container:%v", podInfraContainerID) _, err = dm.runContainerInPod(pod, container, namespaceMode, namespaceMode) @@ -1635,3 +1644,62 @@ func (dm *DockerManager) SyncPod(pod *api.Pod, runningPod kubecontainer.Pod, pod return nil } + +// verifyNonRoot returns an error if the container or image will run as the root user. +func (dm *DockerManager) verifyNonRoot(container *api.Container) error { + if securitycontext.HasRunAsUser(container) { + if securitycontext.HasRootRunAsUser(container) { + return fmt.Errorf("container's runAsUser breaks non-root policy") + } + return nil + } + + imgRoot, err := dm.isImageRoot(container.Image) + if err != nil { + return err + } + if imgRoot { + return fmt.Errorf("container has no runAsUser and image will run as root") + } + + return nil +} + +// isImageRoot returns true if the user directive is not set on the image, the user is set to 0 +// or the user is set to root. If there is an error inspecting the image this method will return +// false and return the error. +func (dm *DockerManager) isImageRoot(image string) (bool, error) { + img, err := dm.client.InspectImage(image) + if err != nil { + return false, err + } + if img == nil || img.Config == nil { + return false, fmt.Errorf("unable to inspect image %s, nil Config", image) + } + + user := getUidFromUser(img.Config.User) + // if no user is defined container will run as root + if user == "" { + return true, nil + } + // do not allow non-numeric user directives + uid, err := strconv.Atoi(user) + if err != nil { + return false, fmt.Errorf("unable to validate image is non-root, non-numeric user (%s) is not allowed", user) + } + // user is numeric, check for 0 + return uid == 0, nil +} + +// getUidFromUser splits the uid out of a uid:gid string. +func getUidFromUser(id string) string { + if id == "" { + return id + } + // split instances where the id may contain uid:gid + if strings.Contains(id, ":") { + return strings.Split(id, ":")[0] + } + // no gid, just return the id + return id +} diff --git a/pkg/kubelet/dockertools/manager_test.go b/pkg/kubelet/dockertools/manager_test.go index 6973fde9bf8..2fc6ee1ebc1 100644 --- a/pkg/kubelet/dockertools/manager_test.go +++ b/pkg/kubelet/dockertools/manager_test.go @@ -2085,3 +2085,129 @@ func TestGetPodStatusSortedContainers(t *testing.T) { } } } + +func TestVerifyNonRoot(t *testing.T) { + dm, fakeDocker := newTestDockerManager() + + // setup test cases. + var rootUid int64 = 0 + var nonRootUid int64 = 1 + + tests := map[string]struct { + container *api.Container + inspectImage *docker.Image + expectedError string + }{ + // success cases + "non-root runAsUser": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: &nonRootUid, + }, + }, + }, + "numeric non-root image user": { + container: &api.Container{}, + inspectImage: &docker.Image{ + Config: &docker.Config{ + User: "1", + }, + }, + }, + "numeric non-root image user with gid": { + container: &api.Container{}, + inspectImage: &docker.Image{ + Config: &docker.Config{ + User: "1:2", + }, + }, + }, + + // failure cases + "root runAsUser": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: &rootUid, + }, + }, + expectedError: "container's runAsUser breaks non-root policy", + }, + "non-numeric image user": { + container: &api.Container{}, + inspectImage: &docker.Image{ + Config: &docker.Config{ + User: "foo", + }, + }, + expectedError: "unable to validate image is non-root, non-numeric user", + }, + "numeric root image user": { + container: &api.Container{}, + inspectImage: &docker.Image{ + Config: &docker.Config{ + User: "0", + }, + }, + expectedError: "container has no runAsUser and image will run as root", + }, + "numeric root image user with gid": { + container: &api.Container{}, + inspectImage: &docker.Image{ + Config: &docker.Config{ + User: "0:1", + }, + }, + expectedError: "container has no runAsUser and image will run as root", + }, + "nil image in inspect": { + container: &api.Container{}, + expectedError: "unable to inspect image", + }, + "nil config in image inspect": { + container: &api.Container{}, + inspectImage: &docker.Image{}, + expectedError: "unable to inspect image", + }, + } + + for k, v := range tests { + fakeDocker.Image = v.inspectImage + err := dm.verifyNonRoot(v.container) + if v.expectedError == "" && err != nil { + t.Errorf("%s had unexpected error %v", k, err) + } + if v.expectedError != "" && !strings.Contains(err.Error(), v.expectedError) { + t.Errorf("%s expected error %s but received %s", k, v.expectedError, err.Error()) + } + } +} + +func TestGetUidFromUser(t *testing.T) { + tests := map[string]struct { + input string + expect string + }{ + "no gid": { + input: "0", + expect: "0", + }, + "uid/gid": { + input: "0:1", + expect: "0", + }, + "empty input": { + input: "", + expect: "", + }, + "multiple spearators": { + input: "1:2:3", + expect: "1", + }, + } + for k, v := range tests { + actual := getUidFromUser(v.input) + if actual != v.expect { + t.Errorf("%s failed. Expected %s but got %s", k, v.expect, actual) + } + } +} diff --git a/pkg/kubelet/rkt/rkt.go b/pkg/kubelet/rkt/rkt.go index 4ca198538c2..04e7b7fb226 100644 --- a/pkg/kubelet/rkt/rkt.go +++ b/pkg/kubelet/rkt/rkt.go @@ -852,6 +852,8 @@ func (r *runtime) SyncPod(pod *api.Pod, runningPod kubecontainer.Pod, podStatus continue } + // TODO: check for non-root image directives. See ../docker/manager.go#SyncPod + // TODO(yifan): Take care of host network change. containerChanged := c.Hash != 0 && c.Hash != expectedHash if containerChanged { diff --git a/pkg/securitycontext/util.go b/pkg/securitycontext/util.go index e9da03d13c2..893a4d825fa 100644 --- a/pkg/securitycontext/util.go +++ b/pkg/securitycontext/util.go @@ -66,3 +66,24 @@ func ParseSELinuxOptions(context string) (*api.SELinuxOptions, error) { Level: fields[3], }, nil } + +// HasNonRootUID returns true if the runAsUser is set and is greater than 0. +func HasRootUID(container *api.Container) bool { + if container.SecurityContext == nil { + return false + } + if container.SecurityContext.RunAsUser == nil { + return false + } + return *container.SecurityContext.RunAsUser == 0 +} + +// HasRunAsUser determines if the sc's runAsUser field is set. +func HasRunAsUser(container *api.Container) bool { + return container.SecurityContext != nil && container.SecurityContext.RunAsUser != nil +} + +// HasRootRunAsUser returns true if the run as user is set and it is set to 0. +func HasRootRunAsUser(container *api.Container) bool { + return HasRunAsUser(container) && HasRootUID(container) +} diff --git a/pkg/securitycontext/util_test.go b/pkg/securitycontext/util_test.go index 155707e5da8..d2f1e48d014 100644 --- a/pkg/securitycontext/util_test.go +++ b/pkg/securitycontext/util_test.go @@ -83,3 +83,124 @@ func compareContexts(name string, ex, ac *api.SELinuxOptions, t *testing.T) { t.Errorf("%v: expected level: %v, got: %v", name, e, a) } } + +func TestHaRootUID(t *testing.T) { + var nonRoot int64 = 1 + var root int64 = 0 + + tests := map[string]struct { + container *api.Container + expect bool + }{ + "nil sc": { + container: &api.Container{SecurityContext: nil}, + }, + "nil runAsuser": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: nil, + }, + }, + }, + "runAsUser non-root": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: &nonRoot, + }, + }, + }, + "runAsUser root": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: &root, + }, + }, + expect: true, + }, + } + + for k, v := range tests { + actual := HasRootUID(v.container) + if actual != v.expect { + t.Errorf("%s failed, expected %t but received %t", k, v.expect, actual) + } + } +} + +func TestHasRunAsUser(t *testing.T) { + var runAsUser int64 = 0 + + tests := map[string]struct { + container *api.Container + expect bool + }{ + "nil sc": { + container: &api.Container{SecurityContext: nil}, + }, + "nil runAsUser": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: nil, + }, + }, + }, + "valid runAsUser": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: &runAsUser, + }, + }, + expect: true, + }, + } + + for k, v := range tests { + actual := HasRunAsUser(v.container) + if actual != v.expect { + t.Errorf("%s failed, expected %t but received %t", k, v.expect, actual) + } + } +} + +func TestHasRootRunAsUser(t *testing.T) { + var nonRoot int64 = 1 + var root int64 = 0 + + tests := map[string]struct { + container *api.Container + expect bool + }{ + "nil sc": { + container: &api.Container{SecurityContext: nil}, + }, + "nil runAsuser": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: nil, + }, + }, + }, + "runAsUser non-root": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: &nonRoot, + }, + }, + }, + "runAsUser root": { + container: &api.Container{ + SecurityContext: &api.SecurityContext{ + RunAsUser: &root, + }, + }, + expect: true, + }, + } + + for k, v := range tests { + actual := HasRootRunAsUser(v.container) + if actual != v.expect { + t.Errorf("%s failed, expected %t but received %t", k, v.expect, actual) + } + } +}