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

set proper file permission for projected service account volume #89193

Merged
merged 1 commit into from May 5, 2020

Conversation

zshihang
Copy link
Contributor

@zshihang zshihang commented Mar 17, 2020

What type of PR is this?
/kind feature

What this PR does / why we need it:
implement the KEP kubernetes/enhancements#1598.

NONE

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

[KEP]: https://github.com/kubernetes/enhancements/pull/1598

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 17, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 17, 2020

Hi @zshihang. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@zshihang
Copy link
Contributor Author

zshihang commented Mar 17, 2020

/assign @mikedanese @liggitt @tallclair

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 17, 2020
pkg/securitycontext/util.go Outdated Show resolved Hide resolved
pkg/volume/volume.go Show resolved Hide resolved
})
}

func changeFilePermission(filename string, fsGroup *int64, readonly bool, info os.FileInfo) error {
Copy link
Member

@liggitt liggitt Mar 19, 2020

Choose a reason for hiding this comment

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

This is making significant changes in this method, and needs very close review by @kubernetes/sig-storage-pr-reviews

I don't have a good sense for the test coverage we have of all of the permissions permutations here, especially across different types of storage

/assign @jsafrane @msau42

Copy link
Member

@msau42 msau42 Mar 20, 2020

Choose a reason for hiding this comment

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

/assign @gnufied
more eyes the better :-)

Copy link
Member

@msau42 msau42 Mar 20, 2020

Choose a reason for hiding this comment

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

Is this behavior protected by any feature gate? When we added the new fsgroup change policy behavior, we guarded the change to this method with a feature gate so we could disable it if it caused regressions to existing behavior.

Copy link
Member

@msau42 msau42 Mar 20, 2020

Choose a reason for hiding this comment

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

Regarding tests, we do have some testing fsgroup + projected volumes, but I don't think they cover the projected service account volume:

https://github.com/kubernetes/kubernetes/blob/master/test/e2e/common/projected_configmap.go#L59
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/common/projected_downwardapi.go
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/common/projected_secret.go

We don't have any e2es covering persistent volumes with runAsUser set (only fsgroup). And for fsgroup, it's only set in some basic tests like: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/testsuites/volumes.go#L177

I thought we had added fsgroup tests to subpath due to various regression issues, but I don't see fsgroup being set in those tests. I will dig further to figure out what happened to them.

@mikedanese mikedanese added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 20, 2020
@mikedanese
Copy link
Member

mikedanese commented Mar 20, 2020

cc @micahhausler

@msau42
Copy link
Member

msau42 commented Mar 20, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 20, 2020
pkg/api/v1/pod/util.go Outdated Show resolved Hide resolved
pkg/api/v1/pod/util.go Outdated Show resolved Hide resolved
pkg/api/v1/pod/util.go Outdated Show resolved Hide resolved
pkg/api/v1/pod/util.go Outdated Show resolved Hide resolved
@zshihang
Copy link
Contributor Author

zshihang commented May 1, 2020

/remove-kind api-change

@k8s-ci-robot k8s-ci-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 1, 2020
@zshihang
Copy link
Contributor Author

zshihang commented May 1, 2020

/retest

1 similar comment
@zshihang
Copy link
Contributor Author

zshihang commented May 1, 2020

/retest

@msau42
Copy link
Member

msau42 commented May 2, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2020
@lavalamp
Copy link
Member

lavalamp commented May 4, 2020

/approve

(Just propagating @mikedanese and @msau42 lgtm/approvals; I haven't actually reviewed this)

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 4, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, msau42, zshihang

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 5, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 5, 2020

New changes are detected. LGTM label has been removed.

@mikedanese mikedanese added the lgtm Indicates that a PR is ready to be merged. label May 5, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented May 5, 2020

@zshihang: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-iscsi 7cc5839331e75b70950e04dbbcade135b600fee7 link /test pull-kubernetes-e2e-gce-iscsi
pull-kubernetes-e2e-aks-engine-azure 7cc5839331e75b70950e04dbbcade135b600fee7 link /test pull-kubernetes-e2e-aks-engine-azure
pull-kubernetes-e2e-azure-file-windows 7cc5839331e75b70950e04dbbcade135b600fee7 link /test pull-kubernetes-e2e-azure-file-windows

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@zshihang
Copy link
Contributor Author

zshihang commented May 5, 2020

/retest

Copy link
Member

@tallclair tallclair left a comment

Sorry I missed this earlier.

podutil.VisitContainers(&pod.Spec, podutil.InitContainers|podutil.Containers, func(container *v1.Container, containerType podutil.ContainerType) bool {
runAsUser, ok := securitycontext.DetermineEffectiveRunAsUser(pod, container)
// One container doesn't specify user or there are more than one
// non-root UIDs.
Copy link
Member

@tallclair tallclair Oct 14, 2020

Choose a reason for hiding this comment

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

I think there might be a couple problems with this logic, if I'm reading it correctly:

  1. A container could set the user in the container image. To get the image user, you can use the getImageUser(...) function, but you'd need to plumb through the runtime and export the method. Also note that the username can't be depended on.
  2. A root user can only be ignored if it also has CAP_DAC_OVERRIDE (although it's included by default).

Even with these precautions, it's still possible that the container could drop permissions at runtime, but I think it should be acceptable to just cover that case by documentation (i.e. use FSGroup)

Copy link
Member

@tallclair tallclair Oct 14, 2020

Choose a reason for hiding this comment

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

Actually, looks like I was misreading the code... I think this says that if the user isn't explicitly specified in all the containers (and the explicit users don't match) then return nil. In that case, I think the code is correct but the comment is incorrect (since it doesn't care about non-root).

Copy link
Contributor Author

@zshihang zshihang Oct 14, 2020

Choose a reason for hiding this comment

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

using getImageUser would require a lot of changes in kubelet in terms of the image pulling and volume mounts.

rfranzke added a commit to rfranzke/gardener that referenced this pull request Nov 30, 2021
krgostev pushed a commit to krgostev/gardener that referenced this pull request Apr 21, 2022
krgostev pushed a commit to krgostev/gardener that referenced this pull request Jul 5, 2022
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/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/kubelet 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 Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Development

Successfully merging this pull request may close these issues.

None yet