[go: nahoru, domu]

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

Prefer non-mutating PSPs, then order by name #52849

Merged
merged 12 commits into from
Oct 17, 2017

Conversation

liggitt
Copy link
Member
@liggitt liggitt commented Sep 21, 2017

Fixes #36184
Fixes #23217
Related to #23217

Removes unnecessary mutation of pods:

  • Determines effective security context for pods using a wrapper containing the pod and container security context, rather than building/setting a combined struct on every admission
  • Does not set privileged:&false on security contexts with privileged:nil
  • Does not set runAsNonRoot:&true on security contexts that already have a non-nil, non-0 runAsUser
  • Does not mutate/normalize container capabilities unless changes are required (missing defaultAddCapabilities or requiredDropCapabilities)

Defines behavior when multiple PSP objects allow a pod:

  • PSPs which allow the pod as-is (no defaulting/mutating) are preferred
  • If the pod must be defaulted/mutated to be allowed, the first PSP (ordered by name) to allow the pod is selected
  • During update operations, when mutations to pod specs are disallowed, only non-mutating PSPs are used to validate the pod
PodSecurityPolicy: when multiple policies allow a submitted pod, priority is given to ones which do not require any fields in the pod spec to be defaulted. If the pod must be defaulted, the first policy (ordered by name) that allows the pod is used.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 21, 2017
@k8s-github-robot k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 21, 2017
@liggitt
Copy link
Member Author
liggitt commented Sep 21, 2017

cc @kubernetes/sig-auth-pr-reviews

// all containers in a single pod must validate under a single provider or we will reject the request
validationErrs := field.ErrorList{}
var (
allowedPod *api.Pod
allowingProvider psp.Provider
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to hold an entire provider if we need only its name?

Copy link
Member Author

Choose a reason for hiding this comment

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

the goal was to keep the existing code intact and move it out of the loop

@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Sep 21, 2017
@liggitt liggitt added area/security release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 21, 2017
@liggitt liggitt added this to the v1.9 milestone Sep 21, 2017
// but continue to see if another PSP allows without mutating
allowedPod = podCopy
allowingProvider = provider
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add more logging for all these branches? Even if they will be on level 6, it's better to have them for easy debugging.

Copy link
Member Author

Choose a reason for hiding this comment

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

added logging

@@ -142,27 +144,62 @@ func (c *podSecurityPolicyPlugin) Admit(a admission.Attributes) error {
return nil
}

// sort by name to make order deterministic
// TODO: add priority field to allow admins to bucket differently
Copy link
Contributor

Choose a reason for hiding this comment

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

open issue or something for this? Sorting on name leads people do weird things.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's already here: #45358

Copy link
Member
@tallclair tallclair left a comment

Choose a reason for hiding this comment

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

+1 For this change. I think this should be considered a cherry-pick candidate for 1.8 patch release, since this is fixing an open bug in a beta feature.

allowedPod = podCopy
allowingProvider = provider
break loop
case specMutationAllowed && allowedPod == nil:
Copy link
Member

Choose a reason for hiding this comment

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

It should be possible to tell whether a PSP is mutating without applying it. Maybe add a TODO here to optimize that in the future? I would prefer it NOT to go into this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

definitely agree... started down that road with #50169 and it got... large. The preemptive copy here is less than ideal, but this was focused on the minimal change to get the external behavior we want

if allowedPod != nil {
*pod = *allowedPod
// annotate and accept the pod
glog.V(4).Infof("pod %s (generate: %s) validated against provider %s", pod.Name, pod.GenerateName, allowingProvider.GetPSPName())
Copy link
Member

Choose a reason for hiding this comment

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

nit: include namespace

Copy link
Member

Choose a reason for hiding this comment

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

^^

@tallclair
Copy link
Member

@liggitt - Are you waiting for anything on this? If it's just a matter of finding the time, I'd be happy to help.

@liggitt
Copy link
Member Author
liggitt commented Oct 2, 2017

Got distracted by a couple 1.8.0 fires. Plan to add tests today/tomorrow, then will be ready

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 this. I think it's a clever solution that also allows people to use less restrictive PSPs if they have access without having to manipulate the pod spec or sorting.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 5, 2017
@liggitt
Copy link
Member Author
liggitt commented Oct 5, 2017

updated with tests, removed one unnecessary defaulter, included a commit fixing tolerating of non-pod-spec mutations done by GC as part of delete

Copy link
Member
@tallclair tallclair left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

providers, errs := c.createProvidersFromPolicies(matchedPolicies, pod.Namespace)
logProviders(pod, providers, errs)

if len(providers) == 0 {
return admission.NewForbidden(a, fmt.Errorf("no providers available to validate pod request"))
}

// TODO(liggitt): allow spec mutation during initializing updates?
Copy link
Member

Choose a reason for hiding this comment

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

if allowedPod != nil {
*pod = *allowedPod
// annotate and accept the pod
glog.V(4).Infof("pod %s (generate: %s) validated against provider %s", pod.Name, pod.GenerateName, allowingProvider.GetPSPName())
Copy link
Member

Choose a reason for hiding this comment

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

^^

for _, provider := range providers {
if errs := assignSecurityContext(provider, pod, field.NewPath(fmt.Sprintf("provider %s: ", provider.GetPSPName()))); len(errs) > 0 {
podCopy := pod.DeepCopy()
Copy link
Member

Choose a reason for hiding this comment

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

As long as you're doing the copying here, you might as well remove the copying logic from assignSecurityContext (it does a bunch of stuff to make sure it doesn't modify the pod if it's invalid).

Copy link
Member Author

Choose a reason for hiding this comment

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

done (which greatly simplified that function), and added lots of tests to make sure pods that pass validation as-is are not mutated by the top-level Admit() call

// the entire pod validated

switch {
case apiequality.Semantic.DeepEqual(pod, podCopy):
Copy link
Member

Choose a reason for hiding this comment

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

This might be a premature optimization, but you can skip this check if len(providers) == 1

Copy link
Member Author

Choose a reason for hiding this comment

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

only if specMutationAllowed is true... I'd rather keep this straightforward and deal with optimizations when we profile where the hotspots are

testPSPAdmitAdvanced(testCaseName, kadmission.Create, psps, pod, nil, shouldPass, true, expectedPSP, t)
}

func testPSPAdmitAdvanced(testCaseName string, op kadmission.Operation, psps []*extensions.PodSecurityPolicy, pod, oldPod *kapi.Pod, shouldPass bool, canMutate bool, expectedPSP string, t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

oldPod looks unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

oldPod *kapi.Pod
psps []*extensions.PodSecurityPolicy
shouldPass bool
canMutate bool
Copy link
Member

Choose a reason for hiding this comment

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

Can you rename this to expectMutation?

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2017
@liggitt
Copy link
Member Author
liggitt commented Oct 16, 2017

/assign @dchen1107 @smarterclayton
for pkg approval (pkg/security/podsecuritypolicy and pkg/securitycontext changes)

@smarterclayton
Copy link
Contributor

/approve no-issue

Looks good as well to me

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2017
@tallclair
Copy link
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, smarterclayton, tallclair

Associated issue: 36184

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 48665, 52849, 54006, 53755). If you want to cherry-pick this change to another branch, please follow the instructions here.

@igalarzab
Copy link

Thank you for the PR! Are there any plans on cherry-picking this for the v1.8.x branch as @tallclair mentioned earlier?

@liggitt
Copy link
Member Author
liggitt commented Oct 19, 2017

Pick is open in #54029 pending review and soak time

w.containerSC.AllowPrivilegeEscalation = v
}

func NewEffectiveContainerSecurityContextAccessor(podSC PodSecurityContextAccessor, containerSC ContainerSecurityContextMutator) ContainerSecurityContextAccessor {
Copy link
Contributor

Choose a reason for hiding this comment

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

@liggitt Why do we need ContainerSecurityContextMutator here? It looks weird to require mutate for an accessor.

Copy link
Member Author

Choose a reason for hiding this comment

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

just because it was reusing the concrete type which required it. I wouldn't object to taking an accessor and wrapping in a no-op mutator impl

@liggitt liggitt deleted the psp-defaulting-order branch October 23, 2017 04:56
@liggitt liggitt changed the title Order PSP by name, prefer non-mutating PSPs Prefer non-mutating PSPs, then order by name Oct 28, 2017
k8s-github-robot pushed a commit that referenced this pull request Oct 31, 2017
…9-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #52849

Cherry pick of #52849 on release-1.8.

#52849: PodSecurityPolicy: Do not mutate nil privileged field

This is a larger change than usual for a cherry-pick, but changes are isolated to the PSP component, the cherry-pick was clean, and this fixes severe usability issues with systems with more than one PSP. Would like to get feedback from community users making use of PSP and let the master PR soak as we consider this.

```release-note
PodSecurityPolicy: when multiple policies allow a submitted pod, priority is given to ones which do not require any fields in the pod spec to be defaulted. If the pod must be defaulted, the first policy (ordered by name) that allows the pod is used.
```
k8s-github-robot pushed a commit that referenced this pull request Nov 2, 2017
Automatic merge from submit-queue (batch tested with PRs 52367, 53363, 54989, 54872, 54643). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Basic GCE PodSecurityPolicy Config

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

This PR lays the foundation for enabling PodSecurityPolicy in GCE and other default deployments. The 3 commits are:

1. Add policies, roles & bindings for the default addons on GCE.
2. Enable the PSP admission controller & load the addon policies when the`ENABLE_POD_SECURITY_POLICY=true` environment variable is set.
3. Support the PodSecurityPolicy in the E2E environment & add PSP tests.

NOTES:

- ~~Depends on #52301 for privileged capabilities~~
- ~~Depends on #52849 for sane mutations~~
- ~~Depends on #53479 for aggregator tests to pass~~
- ~~Depends on #54175 for dedicated fluentd service~~ account
- This PR is a fork of #46064, credit to @Q-Lee

**Which issue this PR fixes**: #43538

**Release note**:
```release-note
Add support for PodSecurityPolicy on GCE: `ENABLE_POD_SECURITY_POLICY=true` enables the admission controller, and installs policies for default addons.
```
k8s-github-robot pushed a commit that referenced this pull request Dec 14, 2017
Automatic merge from submit-queue (batch tested with PRs 55557, 55504, 56269, 55604, 56202). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Create{Container,Pod}SecurityContext: modify a pod and don't return the annotations

**What this PR does / why we need it**:
Prior #52849 we couldn't modify a pod and had to return annotations from the methods. But now, as we always working with a copy of a pod, we can modify it directly and we don't need to copy&return annotations separately.

This PR simplifies the code by modifying a pod directly. Also it renames these methods and replaces returning of the `SecurityContext` by in-place modification.

In fact it reverts the changes from #30257

**Release note**:
```release-note
NONE
```

PTAL @liggitt @timstclair 
CC @simo5
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Jan 5, 2018
…ring_pod_update

Automatic merge from submit-queue (batch tested with PRs 17856, 16934, 17979, 17993, 18001).

Improve the process of pod updates by preferring non-mutating SCCs and reducing pod mutations

This is adaptation of kubernetes/kubernetes#52849 to SCC.

Details (mostly copy&pasted from original PR):
- SCCs which allow the pod as-is (no defaulting/mutating) are preferred
- During update operations, when mutations to pod specs are disallowed, only non-mutating SCCs are used to validate the pod
- Removes unnecessary mutation of pods:
  - Determines effective security context for pods using a wrapper containing the pod and container security context, rather than building/setting a combined struct on every admission
  - Does not set `privileged: &false` on security contexts with `privileged: nil`
  - Does not set `runAsNonRoot: &true` on security contexts that already have a non-nil, non-0 runAsUser
  - Does not mutate/normalize container capabilities unless changes are required (missing `defaultAddCapabilities` or `requiredDropCapabilities`)

Fixes #16467
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/security 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. 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.

None yet