Skip to content
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.

Sign up for GitHub

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

Jump to bottom

MustRunAsNonRoot should reject a pod if it has non-numeric USER #56503

Merged
merged 1 commit into from Nov 30, 2017
Merged

MustRunAsNonRoot should reject a pod if it has non-numeric USER #56503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 4 additions & 7 deletions pkg/kubelet/kuberuntime/kuberuntime_container.go
View file
Open in desktop
Expand Up @@ -183,13 +183,10 @@ func (m *kubeGenericRuntimeManager) generateContainerConfig(container *v1.Contai
if err != nil {
return nil, err
}
if uid != nil {
// Verify RunAsNonRoot. Non-root verification only supports numeric user.
if err := verifyRunAsNonRoot(pod, container, *uid); err != nil {
return nil, err
}
} else if username != "" {
glog.Warningf("Non-root verification doesn't support non-numeric user (%s)", username)

// Verify RunAsNonRoot. Non-root verification only supports numeric user.
if err := verifyRunAsNonRoot(pod, container, uid, username); err != nil {
return nil, err
}

command, args := kubecontainer.ExpandContainerCommandAndArgs(container, opts.Envs)
Expand Down
14 changes: 13 additions & 1 deletion pkg/kubelet/kuberuntime/kuberuntime_container_test.go
View file
Open in desktop
Expand Up @@ -236,7 +236,7 @@ func makeExpectedConfig(m *kubeGenericRuntimeManager, pod *v1.Pod, containerInde
}

func TestGenerateContainerConfig(t *testing.T) {
_, _, m, err := createTestRuntimeManager()
_, imageService, m, err := createTestRuntimeManager()
assert.NoError(t, err)

pod := &v1.Pod{
Expand Down Expand Up @@ -290,6 +290,18 @@ func TestGenerateContainerConfig(t *testing.T) {

_, err = m.generateContainerConfig(&podWithContainerSecurityContext.Spec.Containers[0], podWithContainerSecurityContext, 0, "", podWithContainerSecurityContext.Spec.Containers[0].Image)
assert.Error(t, err)

imageId, _ := imageService.PullImage(&runtimeapi.ImageSpec{Image: "busybox"}, nil)
image, _ := imageService.ImageStatus(&runtimeapi.ImageSpec{Image: imageId})

image.Uid = nil
image.Username = "test"

podWithContainerSecurityContext.Spec.Containers[0].SecurityContext.RunAsUser = nil
podWithContainerSecurityContext.Spec.Containers[0].SecurityContext.RunAsNonRoot = &runAsNonRootTrue

_, err = m.generateContainerConfig(&podWithContainerSecurityContext.Spec.Containers[0], podWithContainerSecurityContext, 0, "", podWithContainerSecurityContext.Spec.Containers[0].Image)
assert.Error(t, err, "RunAsNonRoot should fail for non-numeric username")
}

func TestLifeCycleHook(t *testing.T) {
Expand Down
11 changes: 7 additions & 4 deletions pkg/kubelet/kuberuntime/security_context.go
View file
Open in desktop
Expand Up @@ -75,7 +75,7 @@ func (m *kubeGenericRuntimeManager) determineEffectiveSecurityContext(pod *v1.Po
}

// verifyRunAsNonRoot verifies RunAsNonRoot.
func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid int64) error {
func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid *int64, username string) error {
effectiveSc := securitycontext.DetermineEffectiveSecurityContext(pod, container)
// If the option is not set, or if running as root is allowed, return nil.
if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil || !*effectiveSc.RunAsNonRoot {
Expand All @@ -89,11 +89,14 @@ func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid int64) error {
return nil
}

if uid == 0 {
switch {
case uid != nil && *uid == 0:
return fmt.Errorf("container has runAsNonRoot and image will run as root")
case uid == nil && len(username) > 0:
return fmt.Errorf("container has runAsNonRoot and image has non-numeric user (%s), cannot verify user is non-root", username)
default:
return nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two questions for @kubernetes/sig-node-pr-reviews

  1. what happens if there is no uid and no username? is that valid?
  2. what happens if there is a non-zero uid and a username? does the numeric uid govern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the answers for sure but I'd expect the that behavior will mimic the Docker:

what happens if there is no uid and no username? is that valid?

In this case container will be run with zero uid. And it isn't valid when runAsNonRoot is specified.

what happens if there is a non-zero uid and a username? does the numeric uid govern?

AFIU uid is translated to --user options and overrides USER from Dockerfile:

$ docker run --rm -it --entrypoint /usr/bin/id phpcoder/mystamps 
uid=1000(mystamps) gid=1000(mystamps) groups=1000(mystamps)
$ docker run --rm -u 2000 -it --entrypoint /usr/bin/id phpcoder/mystamps 
uid=2000 gid=0(root)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if there is no uid and no username? is that valid?

In this case container will be run with zero uid. And it isn't valid when runAsNonRoot is specified.

I'd expect that to be invalid as well, so we'd need this case:

case uid == nil && len(username) == 0:
  return fmt.Errorf("container has runAsNonRoot and no uid or username, cannot verify user is non-root", username)

@sjenning, does that sound right to you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old code behavior was

  1. no validation; assumed valid
  2. username is ignored if uid != nil

looks to me that the old behaviour is maintained in this refactor.

Copy link
Contributor Author

@php-coder php-coder Nov 30, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect that to be invalid as well, so we'd need this case:

I can add it but it should never happen because GetImageUser that we use for getting uid/username returns zero uid when uid/username couldn't be determined:

kubernetes/pkg/kubelet/kuberuntime/helpers.go

Lines 130 to 150 in a0ed616

// getImageUser gets uid or user name that will run the command(s) from image. The function
// guarantees that only one of them is set.
func (m *kubeGenericRuntimeManager) getImageUser(image string) (*int64, string, error) {
imageStatus, err := m.imageService.ImageStatus(&runtimeapi.ImageSpec{Image: image})
if err != nil {
return nil, "", err
}
if imageStatus != nil {
if imageStatus.Uid != nil {
return &imageStatus.GetUid().Value, "", nil
}
if imageStatus.Username != "" {
return nil, imageStatus.Username, nil
}
}
// If non of them is set, treat it as root.
return new(int64), "", nil
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, this is fine as-is then

}

return nil
}

// convertToRuntimeSecurityContext converts v1.SecurityContext to runtimeapi.SecurityContext.
Expand Down
3 changes: 2 additions & 1 deletion pkg/kubelet/kuberuntime/security_context_test.go
View file
Open in desktop
Expand Up @@ -105,7 +105,8 @@ func TestVerifyRunAsNonRoot(t *testing.T) {
},
} {
pod.Spec.Containers[0].SecurityContext = test.sc
err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], int64(0))
uid := int64(0)
err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], &uid, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add also test with verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], nil, "test") ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a one test already and it should be enough to make this merged. In order to not block this PR, I'm going to add a test next week as a follow-up. @liggitt didn't object against this.

if test.fail {
assert.Error(t, err, test.desc)
} else {
Expand Down