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

Jump to bottom

Run pause image as non-root user and group #97963

Merged
merged 1 commit into from Mar 16, 2021
Merged

Run pause image as non-root user and group #97963

merged 1 commit into from Mar 16, 2021

Conversation

saschagrunert
Copy link
Member

What type of PR is this?

/kind feature

What this PR does / why we need it:
We now build the pause image to use a pseudo user and group 65535:65535.
This increases the security aspect of the container image, if a
vulnerability would directly affect the pause container.

Which issue(s) this PR fixes:

Fixes #95038

Special notes for your reviewer:
/hold

I'm not sure what the CI thinks about this change, nor if it has other implications.

Does this PR introduce a user-facing change?:

Update pause container to run as pseudo user and group `65535:65535`. This implies the release of version 3.5 of the container images.

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

None

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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 Jan 12, 2021
@saschagrunert
Copy link
Member Author

/sig release
/priority important-soon

@k8s-ci-robot k8s-ci-robot added sig/release Categorizes an issue or PR as relevant to SIG Release. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 12, 2021
@saschagrunert saschagrunert mentioned this pull request Jan 12, 2021
Closed
@saschagrunert saschagrunert changed the title WIP: Run pause image as non-root user and group Run pause image as non-root user and group Jan 12, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 12, 2021
@kubeedge-bot
Copy link

@saschagrunert: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2021
@saschagrunert
Copy link
Member Author

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@saschagrunert
Copy link
Member Author

@BenTheElder @cblecker @fejta is this something we can do?

pjbgf
pjbgf approved these changes Feb 4, 2021
View reviewed changes
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

It would be great to see this one on 1.21. 🙏

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2021
@justaugustus justaugustus added the area/release-eng Issues or PRs related to the Release Engineering subproject label Feb 7, 2021
@BenTheElder
Copy link
Member

would kind of like someone from node to LGTM / check in on this, as the primary users of pause

@saschagrunert
Copy link
Member Author

cc @mrunalp @ehashman for an LGTM (or not)

@saschagrunert
Copy link
Member Author

/retest

ehashman
ehashman reviewed Mar 12, 2021
View reviewed changes
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

/lgtm

with the assumption this isn't landing in 1.21

/retest
/sig security

@k8s-ci-robot k8s-ci-robot added the sig/security Categorizes an issue or PR as relevant to SIG Security. label Mar 12, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 12, 2021
@ehashman ehashman moved this from Triage to Done in SIG Node PR Triage Mar 12, 2021
@ehashman
Copy link
Member

/cc @pacoxu

@spiffxp
Copy link
Member

spiffxp commented Mar 13, 2021

/lgtm

with the assumption this isn't landing in 1.21

/retest

/sig security

@ehashman I think I agree but to help us understand why this isn't worth filing an exception for, what concerns do you have with this landing in v1.21

@ehashman
Copy link
Member

@ehashman I think I agree but to help us understand why this isn't worth filing an exception for, what concerns do you have with this landing in v1.21

I don't have any specific concerns, I was just quoting the discussion above with @saschagrunert :)

@pacoxu
Copy link
Member

pacoxu commented Mar 13, 2021

There is a related issue with windows. #92963

Windows pods need to support RunAsUserName like Linux pods, not just work containers, not pause containers #92963

Does this will support windows non-root as well?

@saschagrunert
Copy link
Member Author

Does this will support windows non-root as well?

I don't think so, since the related issue seems to be feature related, whereas this is more an increasing default security enhancement.

@ehashman @spiffxp I think we could raise an exception for this PR.

Pinging @kubernetes/sig-release-leads @kubernetes/release-engineering to apply the milestone (or not).

@cpanato
Copy link
Member

cpanato commented Mar 15, 2021

looks ok to me to raise an exception for this change.

+1 from my side

@pacoxu
Copy link
Member

pacoxu commented Mar 16, 2021

like #98205, we may update runtimes and k/k later.

BTW, update change log https://github.com/kubernetes/kubernetes/blob/master/build/pause/CHANGELOG.md would be better.

@saschagrunert
Copy link
Member Author

like #98205, we may update runtimes and k/k later.

BTW, update change log https://github.com/kubernetes/kubernetes/blob/master/build/pause/CHANGELOG.md would be better.

Sure, I can follow up on that. 👍

@cpanato
Copy link
Member

cpanato commented Mar 16, 2021

everybody agrees that we need to get this for 1.21 so adding the milestone

/milestone v1.21

@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 16, 2021
@cpanato
Copy link
Member

cpanato commented Mar 16, 2021

/hold for any other comments, feel free to cancel that

@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 Mar 16, 2021
@saschagrunert
Copy link
Member 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 Mar 16, 2021
@saschagrunert
Copy link
Member Author

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot merged commit 2a26f27 into kubernetes:master Mar 16, 2021
@saschagrunert saschagrunert deleted the pause-non-root branch March 16, 2021 12:33
@saschagrunert saschagrunert mentioned this pull request Mar 16, 2021
Merged
This was referenced May 3, 2021
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehashman ehashman ehashman left review comments

@pjbgf pjbgf pjbgf approved these changes

@justaugustus justaugustus Awaiting requested review from justaugustus

@listx listx Awaiting requested review from listx

@pacoxu pacoxu Awaiting requested review from pacoxu

Assignees

@spiffxp spiffxp

@justaugustus justaugustus

@BenTheElder BenTheElder

@ehashman ehashman

@pjbgf pjbgf

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm area/kubectl area/kubelet area/provider/gcp Issues or PRs related to gcp provider area/release-eng Issues or PRs related to the Release Engineering subproject area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. 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/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/security Categorizes an issue or PR as relevant to SIG Security. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node CI/Test Board
Triage
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Done
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.

Run Linux Pause container as a non-root user
10 participants
@saschagrunert @kubeedge-bot @BenTheElder @k8s-ci-robot @ehashman @spiffxp @pacoxu @cpanato @pjbgf @justaugustus