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

Windows: ensure runAsNonRoot does case-insensitive comparison on username #111009

Merged
merged 1 commit into from Jul 29, 2022

Conversation

marosset
Copy link
Contributor

@marosset marosset commented Jul 7, 2022

Signed-off-by: Mark Rossetti marosset@microsoft.com

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR fixes an issue where kubelet is doing a case-sensitive comparison for 'ContainerAdministrator' (root account for Windows containers) if runAsNonRoot is set. Windows usernames are case-insensitive so the check should be case-insensitive as well

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

UserName check for 'ContainerAdministrator' is now case-insensitive if runAsNonRoot is set to true on Windows.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


/sig windows node
/area kubelet

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. sig/windows Categorizes an issue or PR as relevant to SIG Windows. sig/node Categorizes an issue or PR as relevant to SIG Node. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubelet needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 7, 2022
@marosset
Copy link
Contributor Author

marosset commented Jul 7, 2022

/assign @jsturtevant @dchen1107 @mrunalp @sjenning

@marosset
Copy link
Contributor Author

marosset commented Jul 7, 2022

/retest

}
return nil
}
}
if len(username) > 0 && username == windowsRootUserName {
if len(username) > 0 && strings.EqualFold(username, windowsRootUserName) {
return fmt.Errorf("container's runAsUser (%s) which will be regarded as root identity and will break non-root policy (pod: %q, container: %s)", username, format.Pod(pod), container.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

should be *effectiveSc.WindowsOptions.RunAsUserName here too

framework.ExpectNotEqual(event, nil, "event should not be empty")
framework.Logf("Got event: %v", event)
expectedEventError := "container's runAsUser (ContainerAdministrator) which will be regarded as root identity and will break non-root policy"
framework.ExpectEqual(true, strings.Contains(event.Message, expectedEventError), "Event error should indicate non-root policy caused container to not start")
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the pod failed to start as expected but this check isn't passing as expected. The output message is

container's runAsUser (ContainerUser)

looks like the log error statement above isn't right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I thought I fixed that in line 62 of security_context_windows.go in this PR.
Hmm

@marosset
Copy link
Contributor Author

marosset commented Jul 8, 2022

@jsturtevant - I'm pretty sure that pull-kubernetes-e2e-capz-windows-containerd is not using the kubelet built from this commit.

Looking at the test logs at https://prow.k8s.io/log?container=test&id=1545445015973531648&job=pull-kubernetes-e2e-capz-windows-containerd I see it building K8s binaries using version v1.25.0-alpha.2.166_fd54a9b94b8e6f but then when we dump node info for test failures I see the Windows nodes reporting using KubeletVersion:v1.25.0-alpha.2.160+3251d4cff6b3b6 for the Windows nodes but KubeletVersion:v1.25.0-alpha.2.166+fd54a9b94b8e6f for the linux / control-plane node.

@marosset
Copy link
Contributor Author

marosset commented Jul 8, 2022

@jsturtevant - I'm pretty sure that pull-kubernetes-e2e-capz-windows-containerd is not using the kubelet built from this commit.

Looking at the test logs at https://prow.k8s.io/log?container=test&id=1545445015973531648&job=pull-kubernetes-e2e-capz-windows-containerd I see it building K8s binaries using version v1.25.0-alpha.2.166_fd54a9b94b8e6f but then when we dump node info for test failures I see the Windows nodes reporting using KubeletVersion:v1.25.0-alpha.2.160+3251d4cff6b3b6 for the Windows nodes but KubeletVersion:v1.25.0-alpha.2.166+fd54a9b94b8e6f for the linux / control-plane node.

These new test cases are passing for me when I run then manually on a cluster I created using a kubelet built from this commit.

@marosset
Copy link
Contributor Author

marosset commented Jul 8, 2022

I have confirmed that we 1) do build windows binaries and upload them to a storage bucket form windows CAPZ PR jobs and 2) we do not use those binaries when provisioning the windows nodes (we use the latest CI binaries instead)

kubernetes-sigs/cluster-api-provider-azure#2467 should address this

@marosset
Copy link
Contributor Author

/test pull-kubernetes-e2e-capz-windows-containerd-test

2 similar comments
@marosset
Copy link
Contributor Author

/test pull-kubernetes-e2e-capz-windows-containerd-test

@marosset
Copy link
Contributor Author

/test pull-kubernetes-e2e-capz-windows-containerd-test

@marosset
Copy link
Contributor Author

/retest
(the pull-kubernetes-e2e-capz-windows-containerd failures are network flakes)

@marosset
Copy link
Contributor Author

/test pull-kubernetes-e2e-capz-windows-containerd

@endocrimes
Copy link
Member

/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 26, 2022
@endocrimes
Copy link
Member

/test pull-kubernetes-e2e-capz-windows-containerd

^ although green on github, the tests did not actually run

@endocrimes endocrimes moved this from Triage to Done in SIG Node PR Triage Jul 27, 2022
Copy link
Member

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

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

/lgtm

tests now run successfully.

/hold
unhold when you're ready to merge (CI pipelines are a bit of a mess today because of go1.19 stuff, so leaving it for you)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2022
@endocrimes endocrimes moved this from Triage to PRs - Needs Reviewer in SIG Node CI/Test Board Jul 27, 2022
@endocrimes endocrimes moved this from PRs - Needs Reviewer to PRs - Needs Approver in SIG Node CI/Test Board Jul 27, 2022
@marosset
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2022
@k8s-ci-robot k8s-ci-robot merged commit 73b3be3 into kubernetes:master Jul 29, 2022
SIG-Windows automation moved this from In Review (v1.25) to Done (v1.25) Jul 29, 2022
SIG Node CI/Test Board automation moved this from PRs - Needs Approver to Done Jul 29, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jul 29, 2022
@marosset marosset deleted the runasnonroot-windows-fix branch September 6, 2022 17:50
k8s-ci-robot added a commit that referenced this pull request Sep 7, 2022
…1009-upstream-release-1.23

Automated cherry pick of #111009: Windows: ensure runAsNonRoot does case-insensitive comparison
k8s-ci-robot added a commit that referenced this pull request Sep 7, 2022
…1009-upstream-release-1.24

Automated cherry pick of #111009: Windows: ensure runAsNonRoot does case-insensitive comparison
k8s-ci-robot pushed a commit that referenced this pull request Sep 7, 2022
…ase-insensitive comparison (#112213)

* Windows: ensure runAsNonRoot does case-insensitive comparison on user name

Signed-off-by: Mark Rossetti <marosset@microsoft.com>

* fixing build error in test/e2e/windows/security_context.go

Signed-off-by: Mark Rossetti <marosset@microsoft.com>
Co-authored-by: Mark Rossetti <marosset@microsoft.com>
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. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
SIG-Windows
  
Done (v1.25)
Development

Successfully merging this pull request may close these issues.

None yet

9 participants