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

SCC seccomp support #9715

Merged
merged 3 commits into from Aug 2, 2016
Merged

SCC seccomp support #9715

merged 3 commits into from Aug 2, 2016

Conversation

pweil-
Copy link
Contributor

@pweil- pweil- commented Jul 6, 2016

Support for kube 1.3 alpha seccomp feature.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2016
@pweil- pweil- changed the title DO NOT MERGE SCC seccomp SCC seccomp support Jul 29, 2016
@pweil-
Copy link
Contributor Author

pweil- commented Jul 29, 2016

@smarterclayton @liggitt ready for review. I want to do some final manual testing before merge but this is the back port of the proposed changes upstream in a 1.3 compatible way.

A good question here is if we really care about this control in the alpha phase and if we should maybe be disabling seccomp altogether until it is out of alpha.

Some items to consider:

  1. This is annotation based which kind of stinks - pulling in the 1.4 proposed changes for field based seccomp is too risky at this point
  2. Moving from unconfined to seccomp enforcing poses a security risk

Scenario: currently all seccomp annotations are ignored. If someone adds an annotation before the upgrade process happens it is an unvalidated annotation and can contain ../../<any file I want. This file will be read and passed as security opts to docker. I'm not sure if that can be exploited in any way but we should be aware of it.

In any case, this gives admin control of validation and defaulting but does not protect against the above scenario.

@pweil-
Copy link
Contributor Author

pweil- commented Jul 29, 2016

[test]

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2016
@pweil-
Copy link
Contributor Author

pweil- commented Aug 1, 2016

re[test]

@pweil-
Copy link
Contributor Author

pweil- commented Aug 1, 2016

re[test]

@mfojtik
Copy link
Member

mfojtik commented Aug 1, 2016

@smarterclayton @liggitt can we have this reviewed asap? it is 3.3 blocker :)

@pweil-
Copy link
Contributor Author

pweil- commented Aug 1, 2016

flake #9959

re[test]

@@ -81,6 +81,7 @@ func GetBootstrapSecurityContextConstraints(sccNameToAdditionalGroups map[string
SupplementalGroups: kapi.SupplementalGroupsStrategyOptions{
Type: kapi.SupplementalGroupsStrategyRunAsAny,
},
SeccompProfiles: []string{"*"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we not making this restrictive? Do we have a value that is actually secure that we can use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the privileged SCC. I left the other SCCs as is for 3.3 which means no seccomp value can be specified and the kubelet default will be used.

The discussion upstream is to change the kubelet default to docker/default but since that is untested I was thinking that the better option is to leave this up to the administrator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, and this was changed specifically because the seccomp feature conformance tests need to be able to specify the annotation.

@pweil-
Copy link
Contributor Author

pweil- commented Aug 1, 2016

flake #9959 re[test]

@stevekuznetsov
Copy link
Contributor

re[test]

@smarterclayton
Copy link
Contributor

LGTM

@pweil-
Copy link
Contributor Author

pweil- commented Aug 2, 2016

re[test]

@pweil-
Copy link
Contributor Author

pweil- commented Aug 2, 2016

flake #10080 re[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 54550af

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7387/)

@pweil-
Copy link
Contributor Author

pweil- commented Aug 2, 2016

tests are green. Last call 🍻

@smarterclayton
Copy link
Contributor

[merge]

On Tue, Aug 2, 2016 at 12:58 PM, Paul Weil notifications@github.com wrote:

tests are green. Last call 🍻


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9715 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pxh-xx5MYsvfGMeRLne3AO-qrnpSks5qb3cZgaJpZM4JGJMp
.

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 2, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7387/) (Image: devenv-rhel7_4730)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 54550af

@openshift-bot openshift-bot merged commit 570a4a3 into openshift:master Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants