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

Introduce new SCCs #5498

Merged
merged 1 commit into from
Oct 31, 2015
Merged

Introduce new SCCs #5498

merged 1 commit into from
Oct 31, 2015

Conversation

pweil-
Copy link
Contributor

@pweil- pweil- commented Oct 29, 2015

@smarterclayton @liggitt - First pass at new SCCs (step one of the new sorting). Next I'll do the priority field and sorting.

@markturansky - this contains the way to pass additional access methods that was referred to in #5465

@pmorie - I'll probably need to rebase my commit for volume security to pull these in if it merges quickly

@pweil-
Copy link
Contributor Author

pweil- commented Oct 29, 2015

[test]

// SecurityContextConstraintNonRoot is used as the name for the system default non-root scc.
SecurityContextConstraintNonRoot = "nonroot"
// SecurityContextConstraintHostMount is used as the name for the system default host mount only scc.
SecurityContextConstraintHostMount = "hostmount"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is host mount useful? This is effectively root access on all nodes, why would you still try constrain nodes? This is effectively privileged.

Copy link
Contributor

Choose a reason for hiding this comment

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

PV recycler pods need it and nothing else (afaik)

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 was specifically for the recycler

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm - we could really use a description on this one...

On Thu, Oct 29, 2015 at 1:42 PM, Jordan Liggitt [email protected]
wrote:

In pkg/cmd/server/bootstrappolicy/securitycontextconstraints.go
#5498 (comment):

@@ -9,12 +9,19 @@ const (
SecurityContextConstraintPrivileged = "privileged"
// SecurityContextConstraintRestricted is used as the name for the system default restricted scc.
SecurityContextConstraintRestricted = "restricted"

  • // SecurityContextConstraintNonRoot is used as the name for the system default non-root scc.
  • SecurityContextConstraintNonRoot = "nonroot"
  • // SecurityContextConstraintHostMount is used as the name for the system default host mount only scc.
  • SecurityContextConstraintHostMount = "hostmount"

PV recycler pods need it and nothing else (afaik)


Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/5498/files#r43418964.

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually should have descriptions on all of these that describe exactly
what they do, and how insecure they are. Like giant warning kind of output.

On Thu, Oct 29, 2015 at 1:45 PM, Clayton Coleman [email protected]
wrote:

Hrm - we could really use a description on this one...

On Thu, Oct 29, 2015 at 1:42 PM, Jordan Liggitt [email protected]
wrote:

In pkg/cmd/server/bootstrappolicy/securitycontextconstraints.go
#5498 (comment):

@@ -9,12 +9,19 @@ const (
SecurityContextConstraintPrivileged = "privileged"
// SecurityContextConstraintRestricted is used as the name for the system default restricted scc.
SecurityContextConstraintRestricted = "restricted"

  • // SecurityContextConstraintNonRoot is used as the name for the system default non-root scc.
  • SecurityContextConstraintNonRoot = "nonroot"
  • // SecurityContextConstraintHostMount is used as the name for the system default host mount only scc.
  • SecurityContextConstraintHostMount = "hostmount"

PV recycler pods need it and nothing else (afaik)


Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/5498/files#r43418964.

Copy link
Member

Choose a reason for hiding this comment

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

The immediate need for this is our customer who has the single NFS mount on every single node and is using qtrees with netapp for capacity constraints. He just needed a local pointer to the directory for his PV, so we told him to use PV.HostPath. His alternative is to create many NFS PVs, which he was disinclined to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

That user can just create an SCC. Nothing special about these. Also, why does that user need host mount? The PV shouldn't need to go through the SCC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added better description comments and big ole warnings on ones that grant host accessing or privileged operations

Copy link
Member

Choose a reason for hiding this comment

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

The recycler creates a pod that runs "rm -rf" on the volume. The pod has to mount the PV.

If we want to support the recycler making HostPath recycling pods, it needs this permission.

@pweil- pweil- force-pushed the addl-sccs branch 2 times, most recently from a409fbb to 161f408 Compare October 29, 2015 18:32
// to be run with a UID, SELinux context, fs group, and supplemental groups that are
// allocated to the namespace.
//
// WARNING: this SCC allows host access to namespaces, file systems, and PIDS. It should only
Copy link
Contributor

Choose a reason for hiding this comment

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

By descriptions I meant annotations on the SCCs themselves (kubernetes.io/description). Doesn't do much good here for end users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, fail...adding annotations. No emoji for derp face... 8^/\ is the best I can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added as annotations. Didn't see a constant for description already so created a local constant. Non-derp face restored.

@pweil- pweil- force-pushed the addl-sccs branch 2 times, most recently from 2252a46 to 4a2ff60 Compare October 29, 2015 19:45
@smarterclayton
Copy link
Contributor

Is there something users can run to reconcile these on an upgrade?

@smarterclayton
Copy link
Contributor

LGTM [merge]

@liggitt
Copy link
Contributor

liggitt commented Oct 29, 2015

Not yet, on our list

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

pweil- commented Oct 30, 2015

Volume security SCCs beat this in. Will rebase first thing in the morning and add fsgroup/sup group strategies to this.

@pweil-
Copy link
Contributor Author

pweil- commented Oct 30, 2015

@liggitt @smarterclayton - added fsgroup/sup group strats in the second commit if you could take a quick glance. The 'anyuid' and 'privileged' sccs allow you to run as any group, the rest are requiring or defaulting.

If that's good I'll squash.

@smarterclayton
Copy link
Contributor

Can you update the godoc real quick?

@pweil- pweil- force-pushed the addl-sccs branch 2 times, most recently from 0e5f9db to 87bf28a Compare October 30, 2015 15:07
@pweil-
Copy link
Contributor Author

pweil- commented Oct 30, 2015

updated docs based on comment and added a godoc reference to how to get the default set of grants.

@pweil-
Copy link
Contributor Author

pweil- commented Oct 30, 2015

re[test]

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2015
@smarterclayton
Copy link
Contributor

[merge]

@smarterclayton
Copy link
Contributor

LGTM

@pweil- pweil- mentioned this pull request Oct 30, 2015
@pweil-
Copy link
Contributor Author

pweil- commented Oct 30, 2015

This is a valid failure and I'll fix it pending the answer to #5543 (comment)

@smarterclayton
Copy link
Contributor

[test]

On Oct 30, 2015, at 7:21 PM, OpenShift Bot [email protected] wrote:

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


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

@pweil-
Copy link
Contributor Author

pweil- commented Oct 31, 2015

re[test]

@smarterclayton
Copy link
Contributor

[test]

On Oct 31, 2015, at 11:40 AM, OpenShift Bot [email protected]
wrote:

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


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

@pweil-
Copy link
Contributor Author

pweil- commented Oct 31, 2015

keeps hitting a known flake #5559

[test]

@smarterclayton
Copy link
Contributor

[test]

On Oct 31, 2015, at 12:56 PM, OpenShift Bot [email protected]
wrote:

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


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

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 6f86f78

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/6552/) (Image: devenv-rhel7_2617)

@openshift-bot
Copy link
Contributor

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

@pweil-
Copy link
Contributor Author

pweil- commented Oct 31, 2015

clean run from jenkins, the ghosts must have moved back to @Kargakis's PR 👻 🎃

@pweil-
Copy link
Contributor Author

pweil- commented Oct 31, 2015

re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 6f86f78

openshift-bot pushed a commit that referenced this pull request Oct 31, 2015
@openshift-bot openshift-bot merged commit 7b5a685 into openshift:master Oct 31, 2015
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.

5 participants