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

AppArmor PodSecurityPolicy support #30183

Merged
merged 1 commit into from Aug 23, 2016

Conversation

timstclair
Copy link

@timstclair timstclair commented Aug 6, 2016

Implements the AppArmor PodSecurityPolicy support based on the alpha API proposed here

This implementation deviates from the original proposal in one way: it adds a separate option for specifying a default profile:

apparmor.security.alpha.kubernetes.io/defaultProfileName

This has several advantages over the original proposal:

  • The default is explicit, rather than implicit on the ordering
  • The default can be specified without constraining the allowed profiles
  • The allowed profiles can be restricted without specifying a default (requires every pod to explicitly set a profile)

The E2E cluster does not currently enable the PodSecurityPolicy, so I will submit E2E tests in a separate PR.

/cc @dchen1107 @pweil- @sttts @jfrazelle @Amey-D


This change is Reviewable

@timstclair timstclair added this to the v1.4 milestone Aug 6, 2016
@timstclair timstclair self-assigned this Aug 6, 2016
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Aug 6, 2016

profile := apparmor.GetProfileName(pod, container.Name)
if profile == "" {
if len(s.allowedProfiles) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't one be able to allow ""?

Copy link
Author

Choose a reason for hiding this comment

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

What's the use case for that? Would the empty string profile just mean that running without an AppArmor profile would be allowed? That would probably be valuable, but I'd prefer to have an explicit parameter for it (e.g. nil) or a separate annotation altogether. My concern is that a trailing comma shouldn't accidentally enable running unconfined.

Copy link
Contributor

@sttts sttts Aug 9, 2016

Choose a reason for hiding this comment

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

I was thinking that later the annotation will be turned into a string field. Then "" means that some default profile is selected, i.e. runtime/default. That's how we do it in seccomp. If you somehow translate runtime/default in the PSP into "" being allowed as a pod value, that's fine. Then you don't have the trailing comma issue.

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking of it as no annotation = zero value, annotation = explicitly set value. By this reasoning I don't think the empty string value needs to be allowed.

@sttts
Copy link
Contributor

sttts commented Aug 6, 2016

Looks pretty good. Please recheck the logic around empty string profiles. There should be a way to allow them with a PSP, next to other non-empty profiles.

return nil
}

apparmor.SetProfileName(pod, container.Name, s.allowedProfiles[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally I've tried to keep these methods just returning the value of what is generated and not actually setting anything. Then the provider applies the values to the copy of the SecurityContext, or in this case annotations. I'm also leaning in favor of changing the provider interface to return a set of annotations that I mentioned over email since it lets us keep the assumption that the provider does not actually change the pod and it is up to the caller to decide if it should apply the generated values.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2016
@timstclair timstclair force-pushed the aa-psp branch 2 times, most recently from 5ce6d85 to b8a4285 Compare August 17, 2016 00:52
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2016
@timstclair timstclair added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 17, 2016
@timstclair timstclair changed the title WIP: AppArmor PodSecurityPolicy support AppArmor PodSecurityPolicy support Aug 17, 2016
@timstclair
Copy link
Author

Rebased on to #30257 & updated to match new interface.

@timstclair timstclair force-pushed the aa-psp branch 4 times, most recently from 54c8d10 to c411087 Compare August 17, 2016 20:56
@timstclair timstclair assigned dchen1107 and unassigned timstclair Aug 17, 2016
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 17, 2016
@timstclair
Copy link
Author

@dchen1107 - This is ready for review. It's the last piece of AppArmor implementation for v1.4. It's a bit messy right now since it includes to dependent PRs, so you can just review the most recent commit: c2cda9f

@timstclair
Copy link
Author

Fixed unit test & validation. Squashed so the commit can still be reviewed separately from the dependent ones.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2016
@dchen1107
Copy link
Member

I only reviewed commits/69efe20d6ab571703796474e6161eff9cd728416 since others are included through different PRs.

LGTM overall, but looks like we have to wait for other prs merged first.

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 18, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2016
@timstclair
Copy link
Author

Rebased now that the dependencies are merged. This should be ready for final review.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 22, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 22, 2016

GCE e2e build/test passed for commit 293770e.

@dchen1107
Copy link
Member

LGTM

@dchen1107 dchen1107 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 23, 2016

GCE e2e build/test passed for commit 293770e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 0b5547f into kubernetes:master Aug 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants