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

Allow PSP's to specify a whitelist of allowed paths for host volume #43946

Merged
merged 7 commits into from
May 30, 2017

Conversation

jhorwit2
Copy link
Contributor

@jhorwit2 jhorwit2 commented Apr 1, 2017

What this PR does / why we need it:

This PR adds the ability to whitelist paths for the host volume to ensure pods cannot access directories they aren't supposed to. E.g. /var/lib/kubelet, /etc/kubernetes/*, etc.

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

Special notes for your reviewer:

Release note:

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.


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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 1, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 1, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @jhorwit2. 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 @k8s-bot 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.

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.

@jhorwit2 jhorwit2 force-pushed the jah/host-path-psp branch from 83195d9 to 791bbe2 Compare April 1, 2017 14:26
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 1, 2017
}
}

return fmt.Errorf("Host path %s is not allowed to be used. Allowed host paths: %v", hostPath, allowedPaths)
Copy link
Contributor

Choose a reason for hiding this comment

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

@php-coder
Copy link
Contributor

It's a bit confusing that release note and PSP member imply path names but algorithm working like path prefix. I think it's counter-intuitive. Could we improve this?

Also I would better to use strict comparison by default. I could imagine situation when dir contains two project dirs named foo and foobar and we want to restrict access only to foo dir. With the current approach it's impossible because it also deny access to foobar.

What do you think about adding support for special * character at the end for a prefix behavior? IMHO it also emphasizes a fact that we're restricting by wildcard.

@php-coder
Copy link
Contributor

@mfischer-zd (as initial requester) what do you think about suggested behavior? Is it ok?

@jhorwit2
Copy link
Contributor Author

jhorwit2 commented Apr 3, 2017

I was going back and forth on strict vs prefix. I didn't think about the example you said (/foo vs /foobar).

With *, would you want to support /foo/bar/*/baz?

@mfischer-zd
Copy link

My own view is that prefixes that look like mount points (with an implicit / suffix to prevent the /foo matching /foobar problem, but without an explicit * suffix) are sufficient. I've never personally needed any other kind of matching for bind mount whitelists.

Copy link
Contributor

@pweil- pweil- left a comment

Choose a reason for hiding this comment

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

I like where this is going. In addition to the question posed by @php-coder on syntax:

Should we validate against the host plugin for this to be set and throw errors if not or does it make sense to allow this to exist even with no host volume in Volumes?

Using an attached slice like this means you need a permutation of the PSP every time you want to give some (but not others) access to a different list of host volumes. Is there a way to not have that coupling? Would you just recommend namespace level PSPs at that point?

@jhorwit2
Copy link
Contributor Author

jhorwit2 commented Apr 3, 2017

@pweil- I feel like multiple permutations of PSP's are going to be common. That's already the case with host ports. The reason I want this feature is to restrict pods based on service accounts & namespaces to specific host volume paths. In that scenario i'd have a PSP per SA/namespace.

@pweil-
Copy link
Contributor

pweil- commented Apr 3, 2017

I feel like multiple permutations of PSP's are going to be common.

Agree. Very similar to roles IMO, just a pain point I've been wary of.

In that scenario i'd have a PSP per SA/namespace.

Fair.

@pweil-
Copy link
Contributor

pweil- commented Apr 3, 2017

@kubernetes/api-reviewers @liggitt @erictune @timstclair

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

liggitt commented Apr 3, 2017

cc @kubernetes/sig-auth-api-reviews

@jhorwit2 jhorwit2 force-pushed the jah/host-path-psp branch from 791bbe2 to 7255703 Compare April 3, 2017 19:41
@smarterclayton
Copy link
Contributor

Rbac has path rules, can we make these consistent with those safely?

@smarterclayton
Copy link
Contributor

On an unrelated note we should have called this ClusterPodSecurityPolicy so we could also have a namespace scoped rule that further confines (so you could have access to a wider scope and then tighten). We should look at discussing what use cases for namespace scope need to be solved in a separate issue

@jhorwit2
Copy link
Contributor Author

jhorwit2 commented Apr 3, 2017

@smarterclayton there was a recent PR to make PSP's have the option to be namespace scoped, but yeah I agree. Like ClusterRole vs Role.

@smarterclayton
Copy link
Contributor

smarterclayton commented Apr 3, 2017 via email

@jhorwit2
Copy link
Contributor Author

jhorwit2 commented Apr 3, 2017

#42360

@liggitt
Copy link
Member

liggitt commented Apr 3, 2017

I could imagine situation when dir contains two project dirs named foo and foobar and we want to restrict access only to foo dir.

I would not expect to deny access to specific directories, but just to enable a whitelist mode.

I think allowing all subpaths of any whitelisted paths automatically makes sense, but definition need to require them to be subpaths, not just suffixes

}

for _, allowedPath := range allowedPaths {
if strings.HasPrefix(hostPath, allowedPath) {
Copy link
Member

Choose a reason for hiding this comment

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

this logic needs to check for exact matches or prefix matches with a trailing /

@jhorwit2 jhorwit2 force-pushed the jah/host-path-psp branch from 43c73df to e8ec654 Compare May 29, 2017 17:08
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2017
@jhorwit2
Copy link
Contributor Author

@smarterclayton rebase and tests look good. 👍

@smarterclayton
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhorwit2, smarterclayton

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 k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46489, 46281, 46463, 46114, 43946)

@k8s-github-robot k8s-github-robot merged commit b5eadb5 into kubernetes:master May 30, 2017
jhorwit2 added a commit to jhorwit2/kubernetes that referenced this pull request Jun 21, 2017
…th-psp"

This reverts commit b5eadb5, reversing
changes made to 1889d65.
@liggitt liggitt added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 21, 2017
@liggitt
Copy link
Member

liggitt commented Jun 21, 2017

removed release note pending revert of the API change in #47851

k8s-github-robot pushed a commit that referenced this pull request Jun 21, 2017
…list

Automatic merge from submit-queue (batch tested with PRs 47851, 47824, 47858, 46099)

Revert "Merge pull request #43946 from jhorwit2/jah/host-path-psp"

fixes #47863

This reverts commit b5eadb5, reversing
changes made to 1889d65.



**What this PR does / why we need it**:

Revert whitelist host paths in psp due to API concerns. Please refer to #47811 for the concerns.

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

**Special notes for your reviewer**:

cc @liggitt @ericchiang @smarterclayton 

**Release note**:

```release-note
```
jianhuiz pushed a commit to jianhuiz/kubernetes that referenced this pull request Sep 1, 2017
Automatic merge from submit-queue (batch tested with PRs 50719, 51216, 50212, 51408, 51381)

Allow PSP's to specify a whitelist of allowed paths for host volume

**What this PR does / why we need it**:

Reverts the revert for the allowed host path feature that was moved from v1.7 to v1.8. This PR also changes the api as discussed in kubernetes#47811.

Original pr: kubernetes#43946
revert: kubernetes#47851

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

**Special notes for your reviewer**:

cc @liggitt @ericchiang @php-coder 

It seems the api build process has changed. Hopefully I did it right 👼 .

**Release note**:

```release-note
Allow PSP's to specify a whitelist of allowed paths for host volume
```
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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pods should not be able to mount arbitrary host volumes