Check the order of bootstrapped SCCs. #15923

adelton · 2017-08-23T13:02:23Z

Cc @simo5 @openshift/sig-security

php-coder · 2017-08-23T13:07:25Z

pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go

 		SecurityContextConstraintNonRoot,
 		SecurityContextConstraintHostMountAndAnyUID,
 		SecurityContextConstraintHostNS,
+		SecurityContextConstraintRestricted,


expectedConstraints were not sorted properly?

Not according to my running this on master. I specifically made this completely separate commit on top of master, so that we get to agree on the proper semantics first.

expectedConstraints didn't need to be sorted until this PR, if that's what you're asking. Let's add a comment here that this ordering is now important.

Done -> 90e5ef3.

adelton · 2017-08-23T13:08:18Z

Cc @openshift/sig-security.

php-coder · 2017-08-23T13:10:36Z

pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go

+	sort.Sort(scc.ByPriority(orderedBootstrappedConstraints))
+
+	for i := 0; i < len(orderedBootstrappedConstraints); i++ {
+		constraint := bootstrappedConstraints[i]


- for i := 0; i < len(orderedBootstrappedConstraints); i++ { + for i := range orderedBootstrappedConstraints {

I've corrected my comment.

Hmm, this is not equivalent -- I then get

securitycontextconstraints_test.go:43: unexpected contraint no. 0 (by priority). Found hostnetwork, wanted privileged securitycontextconstraints_test.go:43: unexpected contraint no. 1 (by priority). Found hostnetwork, wanted nonroot securitycontextconstraints_test.go:43: unexpected contraint no. 2 (by priority). Found hostnetwork, wanted hostmount-anyuid securitycontextconstraints_test.go:43: unexpected contraint no. 3 (by priority). Found hostnetwork, wanted hostaccess securitycontextconstraints_test.go:43: unexpected contraint no. 4 (by priority). Found hostnetwork, wanted restricted securitycontextconstraints_test.go:43: unexpected contraint no. 5 (by priority). Found hostnetwork, wanted anyuid

shouldn't you be getting the constraint from orderedBootstrappedConstraints to compare against expectedConstraintNames rather than bootstrappedConstraints? You still want the counter anyway to know which expected name to check against.

Yes, you are right.
I'll need to rethink this.

@adelton Could you try the following then?

- for i := 0; i < len(orderedBootstrappedConstraints); i++ { - constraint := bootstrappedConstraints[i] + for i, constraint := range orderedBootstrappedConstraints {

Fixed -> d3c8d27.

php-coder · 2017-08-23T13:12:05Z

pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go

 	"testing"

 	"k8s.io/apiserver/pkg/authentication/serviceaccount"

 	securityapi "github.com/openshift/origin/pkg/security/apis/security"
+	scc "github.com/openshift/origin/pkg/security/securitycontextconstraints"
 )

 func TestBootstrappedConstraints(t *testing.T) {
 	expectedConstraints := []string{


Consider to rename expectedConstraints to expectedConstraintNames to better express it's content.

Done -> 70aaf89.

php-coder · 2017-08-23T13:18:03Z

pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go

 	}

+	var orderedBootstrappedConstraints []*securityapi.SecurityContextConstraints


Why do we need to introduce orderedBootstrappedConstraints? Why we can't sort bootstrappedConstraints and use it later?

Because ByPriority wants pointers and bootstrappedConstraints does not get pointers from GetBootstrapSecurityContextConstraints.

I tried sort.Sort(scc.ByPriority(bootstrappedConstraints)) but that did not pan out.

Because ByPriority wants pointers and bootstrappedConstraints does not get pointers from GetBootstrapSecurityContextConstraints.

Ok, I see. I'll try to prepare PR that changes the return result of GetBootstrapSecurityContextConstraints.

pweil- · 2017-08-23T13:14:32Z

pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go

 		SecurityContextConstraintNonRoot,
 		SecurityContextConstraintHostMountAndAnyUID,
 		SecurityContextConstraintHostNS,
+		SecurityContextConstraintRestricted,


expectedConstraints didn't need to be sorted until this PR, if that's what you're asking. Let's add a comment here that this ordering is now important.

pweil-

added pending question

php-coder · 2017-08-23T14:04:29Z

pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go

 		SecurityContextConstraintNonRoot,
 		SecurityContextConstraintHostMountAndAnyUID,
 		SecurityContextConstraintHostNS,
+		SecurityContextConstraintRestricted,
 		SecurityContextConstraintsAnyUID,


Is this order correct? Why anyuid isn't the last? It has priority: 10 AFAIR. What I'm missing?

With the latest logic fixes, anyuid is now actually first. Which sounds correct, it matches

[...] // a higher priority is considered "less" so that it moves to the front of the line if iSCCPriority > jSCCPriority { return true }

adelton · 2017-08-23T14:22:30Z

added pending question

Logic fixed -> d3c8d27.

Logic now fixed in d3c8d27.

php-coder · 2017-08-23T14:47:51Z

pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go

-		SecurityContextConstraintsAnyUID,
-		SecurityContextConstraintsHostNetwork,
+		SecurityContextConstraintHostMountAndAnyUID,
+		SecurityContextConstraintPrivileged,


@pweil- Do you agree with such order?

I don't understand it: if user has access to all the SCCs then I'd expect the following order: anyuid, restricted, ..., privileged.

In other words, the question is why hostnetwork goes before restricted?

this looks like a bug. The host namespaces should be involved in the values. I'm betting they have matching point value and no priority so they're sorting by name.

@pweil- I've submitted #15933

@pweil-

…sult Automatic merge from submit-queue (batch tested with PRs 15964, 15624, 15924) GetBootstrapSecurityContextConstraints: change return type to a slice of pointers Extracted from #15923 (comment): It turned out that in all the places we need `[]*SecurityContextConstraints`. This PR updates `GetBootstrapSecurityContextConstraints` function to return this type. This change simplify our code. PTAL @pweil- @adelton CC @simo5

adelton · 2017-09-07T08:07:04Z

Rebased on master -> 4a1bceb.

Could we get this reviewed and merged? Yes, the order will have to change but I'd like to have the test in as a safeguard for any other changes that might go into the code in the mean time. When #15933 is addressed, it will explicitly change the order in expectedConstraintNames.

php-coder

/lgtm

php-coder · 2017-09-07T09:38:22Z

/unassign

enj · 2017-09-07T12:42:49Z

@pweil- can you approve? I do not have enough context to do so.

adelton · 2017-09-07T13:06:56Z

The context is basically: we pre-create some SCCs, so we wonder what their ordering is. This test adds the check. Yes, it uncovered that the order is actually not what we expected, hence #15933 was filed. But to address that, we need some changes that we have in queue for #14530 in #14825. That's why I'd like to add this test now to cover the situation as it is now, and amend the expected results in the test once we actually change the behaviour.

php-coder · 2017-09-07T14:29:30Z

@pweil- can you approve? I do not have enough context to do so.

@enj Paul is on PTO this week.

@liggitt could you take a look and approve?

liggitt · 2017-09-07T14:36:52Z

/approve
agree we should check the order and agree hostnetwork should come after restricted

openshift-merge-robot · 2017-09-07T14:37:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adelton, liggitt, php-coder

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cmd/OWNERS~~ [liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-09-07T16:29:24Z

Automatic merge from submit-queue (batch tested with PRs 15923, 16172)

openshift-merge-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 23, 2017

openshift-merge-robot assigned liggitt and csrwng Aug 23, 2017

adelton mentioned this pull request Aug 23, 2017

Make SCC with less capabilities more restrictive. #14825

Merged

csrwng assigned pweil- and unassigned csrwng Aug 23, 2017

php-coder reviewed Aug 23, 2017

View reviewed changes

adelton force-pushed the issue-14530-sort-test branch from 05f482f to 2cec279 Compare August 23, 2017 13:14

php-coder reviewed Aug 23, 2017

View reviewed changes

pweil- approved these changes Aug 23, 2017

View reviewed changes

adelton force-pushed the issue-14530-sort-test branch from 2cec279 to 70aaf89 Compare August 23, 2017 13:27

adelton changed the title ~~Check the order fo bootstrapped SCCs.~~ Check the order of bootstrapped SCCs. Aug 23, 2017

adelton force-pushed the issue-14530-sort-test branch from 70aaf89 to 90e5ef3 Compare August 23, 2017 13:32

pweil- previously requested changes Aug 23, 2017

View reviewed changes

php-coder mentioned this pull request Aug 23, 2017

GetBootstrapSecurityContextConstraints: change return type to a slice of pointers #15924

Merged

adelton changed the title ~~Check the order of bootstrapped SCCs.~~ [do not merge] Check the order of bootstrapped SCCs. Aug 23, 2017

php-coder reviewed Aug 23, 2017

View reviewed changes

adelton force-pushed the issue-14530-sort-test branch from 90e5ef3 to d3c8d27 Compare August 23, 2017 14:13

openshift-merge-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 23, 2017

adelton changed the title ~~[do not merge] Check the order of bootstrapped SCCs.~~ Check the order of bootstrapped SCCs. Aug 23, 2017

php-coder reviewed Aug 23, 2017

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2017

Check the order of bootstrapped SCCs.

4a1bceb

adelton force-pushed the issue-14530-sort-test branch from d3c8d27 to 4a1bceb Compare September 7, 2017 07:57

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 7, 2017

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2017

php-coder approved these changes Sep 7, 2017

View reviewed changes

openshift-ci-robot assigned php-coder Sep 7, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2017

openshift-ci-robot unassigned php-coder Sep 7, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2017

openshift-merge-robot merged commit ef2a29e into openshift:master Sep 7, 2017

adelton deleted the issue-14530-sort-test branch September 8, 2017 06:51

		}

		var orderedBootstrappedConstraints []*securityapi.SecurityContextConstraints

Check the order of bootstrapped SCCs. #15923

Check the order of bootstrapped SCCs. #15923

Uh oh!

Conversation

adelton commented Aug 23, 2017 • edited by php-coder Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelton commented Aug 23, 2017

Uh oh!

php-coder Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pweil- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelton commented Aug 23, 2017

Uh oh!

php-coder Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelton commented Sep 7, 2017

Uh oh!

php-coder left a comment

Choose a reason for hiding this comment

Uh oh!

php-coder commented Sep 7, 2017

Uh oh!

enj commented Sep 7, 2017

Uh oh!

adelton commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

php-coder commented Sep 7, 2017

Uh oh!

liggitt commented Sep 7, 2017

Uh oh!

openshift-merge-robot commented Sep 7, 2017

Uh oh!

adelton commented Aug 23, 2017 •

edited by php-coder

Loading

php-coder Aug 23, 2017 •

edited

Loading

php-coder Aug 23, 2017 •

edited

Loading

adelton commented Sep 7, 2017 •

edited

Loading