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.

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

kuberuntime: check the value of RunAsNonRoot when verifying #47009

Merged
merged 2 commits into from
Jun 6, 2017

Conversation

yujuhong
Copy link
Contributor

@yujuhong yujuhong commented Jun 6, 2017

The verification function is fixed to check the value of RunAsNonRoot,
not just the existence of it. Also adds unit tests to verify the correct
behavior.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #46996

Release note:

Fix the bug where container cannot run as root when SecurityContext.RunAsNonRoot is false.

@yujuhong yujuhong added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jun 6, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 6, 2017
@yujuhong
Copy link
Contributor Author

yujuhong commented Jun 6, 2017

/cc @feiskyer

@k8s-ci-robot k8s-ci-robot requested a review from feiskyer June 6, 2017 00:44
@yujuhong yujuhong added this to the v1.7 milestone Jun 6, 2017
Copy link
Member

@feiskyer feiskyer left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 6, 2017
@Q-Lee
Copy link
Contributor

Q-Lee commented Jun 6, 2017

@liggitt this should fix the kubelet issue we discussed earlier.

@liggitt liggitt added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Jun 6, 2017
@liggitt
Copy link
Member

liggitt commented Jun 6, 2017

LGTM, would like a second from @pweil-

The verification function is fixed to check the value of RunAsNonRoot,
not just the existence of it. Also adds unit tests to verify the correct
behavior.
@yujuhong
Copy link
Contributor Author

yujuhong commented Jun 6, 2017

I pushed again just to remove the two redundant lines (48-49 in security_context_test.go) from the original test. Nothing else was changed.

@@ -72,7 +72,8 @@ func (m *kubeGenericRuntimeManager) determineEffectiveSecurityContext(pod *v1.Po
// verifyRunAsNonRoot verifies RunAsNonRoot.
func verifyRunAsNonRoot(pod *v1.Pod, container *v1.Container, uid int64) error {
effectiveSc := securitycontext.DetermineEffectiveSecurityContext(pod, container)
if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil {
// If the option is not set, or if running as root is allowed, return nil.
if effectiveSc == nil || effectiveSc.RunAsNonRoot == nil || !*effectiveSc.RunAsNonRoot {
Copy link
Member

Choose a reason for hiding this comment

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

effectiveSc.GetRunAsNonRoot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a k8s api pod object. I don't think we have getters for them.

Copy link
Member

Choose a reason for hiding this comment

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

ACK. Confused initially.

@@ -244,7 +244,7 @@ func TestGenerateContainerConfig(t *testing.T) {
Command: []string{"testCommand"},
WorkingDir: "testWorkingDir",
SecurityContext: &v1.SecurityContext{
RunAsNonRoot: &RunAsNonRoot,
RunAsNonRoot: &runAsNonRootTrue,
Copy link
Member

Choose a reason for hiding this comment

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

What is the following expectedConfig used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably can be removed. @feiskyer
But I don't want to turn this into a test cleanup PR. That can be done in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Missing one assertion

	assert.Equal(t, expectedConfig, containerConfig, "generate container config for kubelet runtime v1.")

errStr: "container's runAsUser breaks non-root policy",
},
{
desc: "Fail if image's user is root and RunAsNonRoot is true",
Copy link
Member

Choose a reason for hiding this comment

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

Add case Pass if image's user is non-root and RunAsNonRoot is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -46,59 +46,59 @@ func TestVerifyRunAsNonRoot(t *testing.T) {
}

err := verifyRunAsNonRoot(pod, &pod.Spec.Containers[0], int64(0))
assert.NoError(t, err)
assert.NoError(t, err, "should pass if SecurityContext is not set")
Copy link
Member

Choose a reason for hiding this comment

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

Why not merge this case into the test table?

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 removed this before you left the comment ;)

Copy link
Member

Choose a reason for hiding this comment

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

ACK

RunAsNonRoot: &runAsNonRootTrue,
RunAsUser: &rootUser,
},
errStr: "container's runAsUser breaks non-root policy",
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, not sure whether we want to check error string.

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 didn't want to change the original test behavior too much, but remove it any way. Done.

@liggitt
Copy link
Member

liggitt commented Jun 6, 2017

@k8s-bot pull-kubernetes-unit test this

@Random-Liu
Copy link
Member

LGTM

@Q-Lee
Copy link
Contributor

Q-Lee commented Jun 6, 2017

@k8s-bot pull-kubernetes-unit test this
#46244

@pweil-
Copy link
Contributor

pweil- commented Jun 6, 2017

LGTM

@feiskyer
Copy link
Member

feiskyer commented Jun 6, 2017

/assign

@feiskyer
Copy link
Member

feiskyer commented Jun 6, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, yujuhong

Associated issue: 46996

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46775, 47009)

@k8s-github-robot k8s-github-robot merged commit 0538023 into kubernetes:master Jun 6, 2017
@yujuhong
Copy link
Contributor Author

yujuhong commented Jun 6, 2017

I haven't squashed yet... :-|

@feiskyer
Copy link
Member

feiskyer commented Jun 6, 2017

I haven't squashed yet... :-|

sorry, didn't notice there is another commit 😢

k8s-github-robot pushed a commit that referenced this pull request Jun 7, 2017
…09-upstream-release-1.6

Automatic merge from submit-queue

Automated cherry pick of #47009

Cherry pick of #47009 on release-1.6.

#47009: kuberuntime: check the value of RunAsNonRoot when
k8s-github-robot pushed a commit that referenced this pull request Jun 27, 2017
Automatic merge from submit-queue (batch tested with PRs 47038, 47105)

kuberuntime: cleanup TestGenerateContainerConfig

Followup of #47009, cleanup TestGenerateContainerConfig and remove unused expectedConfig.

/assign @yujuhong
@yujuhong yujuhong deleted the run-as-non-root branch September 11, 2017 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubelet fails to admit containers with valid securitycontexts
9 participants