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 caps defaults #6627

Merged
merged 1 commit into from
Feb 1, 2016
Merged

Conversation

pweil-
Copy link
Contributor

@pweil- pweil- commented Jan 13, 2016

Part 2, dependent on #6470 (second commit only)

Card: https://trello.com/c/7eesi2lw/586-add-defaulting-and-restrictions-in-scc-for-capabilities

@eparis @smarterclayton

Based on the SCCs we currently have and the request in the card here is what I came up with:

References:

  1. default set of caps that are kept by Docker (corresponds with Eric's testing and comment in the card - I think this is correct but not 100%) https://github.com/docker/docker/blob/938d28e772ec32ed3b09bfb8907852e497990076/daemon/execdriver/native/template/default_template_linux.go
  2. confirmed we cannot drop audit_write Add AUDIT_WRITE cap moby/moby#7179

SCCs

  1. restricted scc - drops unsafe caps, should have the default caps from docker but nothing is allowed or required to be explicitly added.
  2. anyuid scc - drops unsafe caps, should have the default caps from docker but nothing is allowed or required to be explicitly added.
  3. "safe" scc - I didn't implement anything for this - how is it different than the restricted SCC if the restricted SCC does not allow any additions?

I still need to test through the different scenarios to make sure things are working as expected.

cc @pmorie

@smarterclayton
Copy link
Contributor

My assumption originally that safe would drop almost all caps. Are there any on the fence?

@deads2k
Copy link
Contributor

deads2k commented Jan 13, 2016

@soltysh note the additional carry when you rebase the rebase pull.

@pweil-
Copy link
Contributor Author

pweil- commented Jan 13, 2016

My assumption originally that safe would drop almost all caps. Are there any on the fence?

So, the default caps that are given by docker are

"CHOWN",
"DAC_OVERRIDE",
"FSETID",
"FOWNER",
"MKNOD",
"NET_RAW",
"SETGID",
"SETUID",
"SETFCAP",
"SETPCAP",
"NET_BIND_SERVICE",
"SYS_CHROOT",
"KILL",
"AUDIT_WRITE",

in restricted and anyuid we are force dropping "KILL", "MKNOD", "SYS_CHROOT", "SETUID", "SETGID". Additionally, since no one can actually request a cap to be added unless it is in the AllowedCapabilities or RequiredCapabilities.Add list you can't gain anything which should leave you with CHOWN, DAC_OVERRIDE, FSETID, FOWNER, NET_RAW, SETFCAP, SETPCAP, NET_BIND_SERVICE, AUDIT_WRITE.

@eparis are any of those questionable?

The other alternative for a "safer" version would be to have RequiredCapabilities.Drop = ALL and RequiredCapabilities.Add = {set of things to force folks to add back}.

@smarterclayton
Copy link
Contributor

Safe in my head is very restrictive (as safe as possible) that can run
50%-80% of software (not Docker images - that's what anyuid is for)

On Wed, Jan 13, 2016 at 8:38 AM, Paul Weil notifications@github.com wrote:

My assumption originally that safe would drop almost all caps. Are there
any on the fence?

So, the default caps that are given by docker are

"CHOWN",
"DAC_OVERRIDE",
"FSETID",
"FOWNER",
"MKNOD",
"NET_RAW",
"SETGID",
"SETUID",
"SETFCAP",
"SETPCAP",
"NET_BIND_SERVICE",
"SYS_CHROOT",
"KILL",
"AUDIT_WRITE",

in restricted and anyuid we are dropping "KILL", "MKNOD", "SYS_CHROOT",
"SETUID", "SETGID". Additionally, since no one can actually request a cap
to be added unless it is in the AllowedCapabilities or
RequiredCapabilities.Add list you can't gain anything which should leave
you with CHOWN, DAC_OVERRIDE, FSETID, FOWNER, NET_RAW, SETFCAP, SETPCAP,
NET_BIND_SERVICE, AUDIT_WRITE.

@eparis https://github.com/eparis are any of those questionable?

The other alternative for a "safer" version would be to have RequiredCapabilities.Drop
= ALL and RequiredCapabilities.Add = {set of things to force folks to add
back}.


Reply to this email directly or view it on GitHub
#6627 (comment).

@pweil- pweil- mentioned this pull request Jan 13, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2016
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2016
@pweil-
Copy link
Contributor Author

pweil- commented Jan 20, 2016

@eparis - bump

@smarterclayton
Copy link
Contributor

Part three will be to change the default in origin and update our readme. We then need to clearly indicate for enterprise the more restrictive default, and reset to the more restrictive default per project in our tests. Then we can say "anyuid is allowed for origin"

@smarterclayton
Copy link
Contributor

I'd like to merge this soon.

@smarterclayton
Copy link
Contributor

[test]

@pweil-
Copy link
Contributor Author

pweil- commented Feb 1, 2016

re[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 6cb0a4c

@openshift-bot
Copy link
Contributor

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

@smarterclayton
Copy link
Contributor

LGTM [merge] - let's be sure to catch eric or dan when they have some cycles to make sure the list is correct.

I'll draft the email and messaging around changing the default mode to "anyuid" in Origin and sync with Brenton on the productization impacts.

@pweil-
Copy link
Contributor Author

pweil- commented Feb 1, 2016

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4815/) (Image: devenv-rhel7_3301)

@smarterclayton
Copy link
Contributor

[merge]

On Mon, Feb 1, 2016 at 4:40 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4813/
)


Reply to this email directly or view it on GitHub
#6627 (comment).

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 6cb0a4c

@pweil-
Copy link
Contributor Author

pweil- commented Feb 1, 2016

openshift-bot pushed a commit that referenced this pull request Feb 1, 2016
@openshift-bot openshift-bot merged commit 596502a into openshift:master Feb 1, 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.

4 participants