SCC sorting by priority #5543

pweil- · 2015-10-30T18:59:25Z

Sorts SCCs by a priority field and then by name. Adds a priority to the privileged SCC so that if you have administrative access you will use that SCC by default (though this is largely unnecessary since the first commit allows authenticated users to run as any uid).

Builds on #5498 which is hanging out in the merge queue right now so only the last three commits need reviewed.

Fixes #5317

@liggitt @smarterclayton

pweil- · 2015-10-30T19:00:22Z

fixing commits so I don't incur rebase wrath, I packaged v1beta3 in with non-v1beta3.

edit: fixed. Last 3 commits need reviewed

liggitt · 2015-10-30T19:10:56Z

Ones with priority specified should come before ones without. Equal priority should maybe sort by the current sort behavior, rather than name, for backwards compatibility

liggitt · 2015-10-30T19:14:48Z

In the case of a tie between priority AND old security score, name is fine to break the tie

liggitt · 2015-10-30T19:28:05Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/api/v1/types.go

+	// SortPriority influences the sort order of SCCs when evaluating which SCCs to try first for
+	// a given pod request based on access in the Users and Groups fields.  The higher the int, the
+	// higher priority.  If scores for multiple SCCs are equal they will be sorted by name.
+	SortPriority int `json:"sortPriority" description:"influences the sort order when evaluating SCCs available to the user"`


"determines which SCC is used when multiple SCCs allow a particular pod; higher priority SCCs are preferred"

I would prefer priority over sortPriority... priority implies a sort, I think

deads2k · 2015-10-31T00:13:09Z

I was wondering this during SCC review as well. Why not simply merge all the user's available SCCs together, taking the highest privilege and preserving ranges alongside the "any's" for defaulting purposes. Then attempt to fill in the default pod a single time instead of once per SCC with the full set of available options. Wouldn't that solve the union problem, the separate of SCCs without specifying all combinations problem, the improper priority problem, and make the code easier to understand all at the same time? Seems like that would only become weird if you wanted crazy things like, "you can use these GIDs, but only as this UID".

I'm guessing you guys considered and discarded this, but I'd like to understand why.

liggitt · 2015-10-31T02:20:46Z

Why not simply merge all the user's available SCCs together, taking the highest privilege and preserving ranges alongside the "any's" for defaulting purposes. Then attempt to fill in the default pod a single time instead of once per SCC with the full set of available options. Wouldn't that solve the union problem, the separate of SCCs without specifying all combinations problem, the improper priority problem, and make the code easier to understand all at the same time?

Mixing and matching bits of different SCCs isn't a good idea... privileges can combine to allow things that weren't intended. For example:

SCC A lets you host mount, but only as a particular user and with a specific selinux context
SCC B does not let you host mount, but lets you run as any non-root user

Mixing and matching would let someone host mount without getting the accompanying uid and selinux restriction the admin specified.

deads2k · 2015-10-31T12:25:29Z

Maybe there's a better structure to use for constraints or the bindings. Having to specify every possible combination is pretty ridiculous and scales horribly as more options are added. Even just assuming on/off, we have 11 options making 2048 different potential policies to manage instead of 11.

Also for amusement, 2048 options taken 2048 at a time where order matters yields 1.6x10^5894 options. A simple, predictable system, :).

liggitt · 2015-10-31T13:36:28Z

1.6x10^5894 options.

Sounds pretty powerful to me :)

smarterclayton · 2015-10-31T14:23:59Z

I doubt you're going to see more than 10 in use at any one site. And we
can survive until then.

On Oct 31, 2015, at 8:25 AM, David Eads [email protected] wrote:

Maybe there's a better structure to use for constraints or the bindings.
Having to specify every possible combination is pretty ridiculous and
scales horribly as more options are added. Even just assuming on/off, we
have 11 options making 2048 different potential policies to manage instead
of 11.

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

pweil- · 2015-11-02T15:04:02Z

@liggitt - round 2 ready for your eyes

sort by priority first
if priorities are equal, sort by score in the old behavior
if priorities and score are equal sort by name
admin has access to privileged and anyuid
anyuid is given priority 1
changed field and descriptions according to comments

liggitt · 2015-11-02T15:06:23Z

Godeps/_workspace/src/k8s.io/kubernetes/pkg/api/v1/types.go

+	// Priority influences the sort order of SCCs when evaluating which SCCs to try first for
+	// a given pod request based on access in the Users and Groups fields.  The higher the int, the
+	// higher priority.  If scores for multiple SCCs are equal they will be sorted by name.
+	Priority int `json:"sortPriority" description:"determines which SCC is used when multiple SCCs allow a particular pod; higher priority SCCs are preferred"`


change json field name to priority

Damn, you're too quick. I just caught it. Fixed.

liggitt · 2015-11-02T15:13:32Z

pkg/security/admission/admission_test.go

@@ -68,9 +68,11 @@ func TestAdmit(t *testing.T) {
 			Type: kapi.SupplementalGroupsStrategyMustRunAs,
 		},
 		Groups: []string{"system:serviceaccounts"},
+		// give this scc priority since it is what we want to validate with
+		SortPriority: 1,


don't you need a more restrictive SCC that also matches to actually check that you're matching on sort priority, and not just the secondary pointScore sort?

actually, I don't need this priority at all. The SCC below it is more restrictive (a MustRunAs uid strategy rather than a MustRunAsRange) that is in scc-sa. Removing the priority they would have equal priorities. The scores should ensure that scc-sa-exact is tried before scc-sa but it should be failing to validate. We check the matched name in the test run which will ensure we're matching against what we expect....removing.

liggitt · 2015-11-02T18:21:03Z

LGTM

liggitt · 2015-11-03T15:37:34Z

it matters when we want to set defaults if the admin hasn't expressed a priority intent. Also, validate priority to be >= 0.

liggitt · 2015-11-03T15:39:06Z

I would expect the following behavior:

Missing priority behaves like priority=0 for purposes of sorting
Missing priority gets set on reconcile
Present priority only gets set on reconcile if they explicitly do --overwrite-priority or similar

pweil- · 2015-11-03T15:50:43Z

Missing priority gets set on reconcile
Present priority only gets set on reconcile if they explicitly do --overwrite-priority or similar

I see what you're saying here. I was considering it an option of either "overwrite all priorities" or "don't touch priorities" during reconcile but if the options are really "set missing" or "overwrite all" then nil makes sense. That was my disconnect.

In that case, the first reconcile run (regardless of option) will set all the missing priorities on bootstrapped commands and subsequent changes to the bootstrap SCC priorities will never update until --overwrite-priority is used. Sound about right to you?

liggitt · 2015-11-03T16:12:30Z

In that case, the first reconcile run (regardless of option) will set all the missing priorities on bootstrapped commands and subsequent changes to the bootstrap SCC priorities will never update until --overwrite-priority is used. Sound about right to you?

Yes, so choose priorities carefully (or just set the one we care about to 1?)

pweil- · 2015-11-03T18:39:40Z

@liggitt - updated to be a pointer, added checks in sorting, and a nil vs non-nil test. Also updated the printer so that nil priority doesn't show as 0 which would be confusing since 0 > nil now. Please take another pass.

pweil- · 2015-11-03T18:44:30Z

oh, btw, I set anyuid to 10 to give wiggle room if an admin wants to adjust other priorities (so they don't have to immediately adjust anyuid just to keep it prioritized).

liggitt · 2015-11-03T18:51:47Z

0 should equal nil for sorting purposes, imo

liggitt · 2015-11-03T18:53:44Z

add validation to make sure non-nil priority is >= 0

liggitt · 2015-11-03T18:54:23Z

pkg/cmd/server/bootstrappolicy/securitycontextconstraints.go

+	var (
+		// this is set to 10 to allow wiggle room for admins to set other priorities without
+		// having to adjust anyUID.
+		securityContextConstraintsAnyUIDPriority = 10


10 PRINT "HELLO" 20 PRINT "WORLD"

parse exception. Do you want the lower score (except 0) to be considered higher priority?

I was just amused at setting to 10 to allow space... took me back...

pweil- · 2015-11-03T19:24:40Z

made nil == 0 for sorting, added in the printer, added validation method for negative priorities.

liggitt · 2015-11-03T19:32:50Z

LGTM

smarterclayton · 2015-11-04T02:01:58Z

[test]

openshift-bot · 2015-11-04T02:22:26Z

Evaluated for origin test up to 4c9aeb1

openshift-bot · 2015-11-04T03:21:57Z

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

SCC sorting by priority

pweil- force-pushed the scc-sorting branch 2 times, most recently from a711f59 to 209fc92 Compare October 30, 2015 19:10

liggitt reviewed Oct 30, 2015
View reviewed changes

pweil- mentioned this pull request Oct 30, 2015

Introduce new SCCs #5498

Merged

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2015

pweil- force-pushed the scc-sorting branch from 209fc92 to 2e4ab75 Compare November 2, 2015 15:02

liggitt reviewed Nov 2, 2015
View reviewed changes

smarterclayton added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2015

pweil- force-pushed the scc-sorting branch 2 times, most recently from 2abbb7b to d40d15e Compare November 2, 2015 15:10

liggitt reviewed Nov 2, 2015
View reviewed changes

pweil- force-pushed the scc-sorting branch 2 times, most recently from 390e56c to 3ee3f37 Compare November 2, 2015 15:44

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2015

pweil- force-pushed the scc-sorting branch from 3ee3f37 to 21ed92f Compare November 2, 2015 18:06

liggitt self-assigned this Nov 3, 2015

pweil- force-pushed the scc-sorting branch 2 times, most recently from 25e7686 to 0b829bc Compare November 3, 2015 18:38

liggitt reviewed Nov 3, 2015
View reviewed changes

pweil- added 3 commits November 3, 2015 14:23

UPSTREAM:<carry>:scc priority field

e0399d9

UPSTREAM:<carry>:v1beta3 scc priority field

39daf35

scc sort by priority

4c9aeb1

pweil- force-pushed the scc-sorting branch from 0b829bc to 4c9aeb1 Compare November 3, 2015 19:23

liggitt added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2015

smarterclayton added area/usability priority/P1 area/security and removed lgtm Indicates that a PR is ready to be merged. labels Nov 3, 2015

smarterclayton added a commit that referenced this pull request Nov 4, 2015

Merge pull request #5543 from pweil-/scc-sorting

6e3342f

SCC sorting by priority

smarterclayton merged commit 6e3342f into openshift:master Nov 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCC sorting by priority #5543

SCC sorting by priority #5543

pweil- commented Oct 30, 2015

pweil- commented Oct 30, 2015

liggitt commented Oct 30, 2015

liggitt commented Oct 30, 2015

liggitt Oct 30, 2015

liggitt Oct 30, 2015

deads2k commented Oct 31, 2015

liggitt commented Oct 31, 2015

deads2k commented Oct 31, 2015

liggitt commented Oct 31, 2015

smarterclayton commented Oct 31, 2015

pweil- commented Nov 2, 2015

liggitt Nov 2, 2015

pweil- Nov 2, 2015

liggitt Nov 2, 2015

pweil- Nov 2, 2015

liggitt commented Nov 2, 2015

liggitt commented Nov 3, 2015

liggitt commented Nov 3, 2015

pweil- commented Nov 3, 2015

liggitt commented Nov 3, 2015

pweil- commented Nov 3, 2015

pweil- commented Nov 3, 2015

liggitt commented Nov 3, 2015

liggitt commented Nov 3, 2015

liggitt Nov 3, 2015

pweil- Nov 3, 2015

liggitt Nov 3, 2015

pweil- commented Nov 3, 2015

liggitt commented Nov 3, 2015

smarterclayton commented Nov 4, 2015

openshift-bot commented Nov 4, 2015

openshift-bot commented Nov 4, 2015

SCC sorting by priority #5543

SCC sorting by priority #5543

Conversation

pweil- commented Oct 30, 2015

pweil- commented Oct 30, 2015

liggitt commented Oct 30, 2015

liggitt commented Oct 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 31, 2015

liggitt commented Oct 31, 2015

deads2k commented Oct 31, 2015

liggitt commented Oct 31, 2015

smarterclayton commented Oct 31, 2015

pweil- commented Nov 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Nov 2, 2015

liggitt commented Nov 3, 2015

liggitt commented Nov 3, 2015

pweil- commented Nov 3, 2015

liggitt commented Nov 3, 2015

pweil- commented Nov 3, 2015

pweil- commented Nov 3, 2015

liggitt commented Nov 3, 2015

liggitt commented Nov 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pweil- commented Nov 3, 2015

liggitt commented Nov 3, 2015

smarterclayton commented Nov 4, 2015

openshift-bot commented Nov 4, 2015

openshift-bot commented Nov 4, 2015