Make SCC with less capabilities more restrictive. #14825

adelton · 2017-06-22T11:53:44Z

adelton · 2017-06-22T11:54:46Z

Would appreciate comments about the logic change (namely the * 10000 change) before looking at test updates.

adelton · 2017-06-22T11:56:47Z

Cc @simo5 @openshift/security

php-coder · 2017-06-23T10:35:59Z

pkg/security/scc/byrestrictions.go

+	return false
+}
+
+// capabilitiesPointValue returns a score based on the capabilities allowed,


Could you add more comments about why we need this and what was the logic behind the statements?

php-coder · 2017-06-23T10:36:27Z

pkg/security/scc/byrestrictions.go

+// capabilitiesPointValue returns a score based on the capabilities allowed,
+// added, or removed by the SCC.
+func capabilitiesPointValue(scc *kapi.SecurityContextConstraints) int {
+	points := 500


Why we're using 500 as initial value and not 0?

To be able to substract and not get below zero easily.

php-coder · 2017-06-23T10:37:24Z

pkg/security/scc/byrestrictions.go

+func capabilitiesPointValue(scc *kapi.SecurityContextConstraints) int {
+	points := 500
+	points += 30 * len(scc.DefaultAddCapabilities)
+	if hasCap(kapi.CapabilityAll, scc.AllowedCapabilities) {


Add also checking for all (ALL).

php-coder · 2017-06-23T10:37:44Z

pkg/security/scc/byrestrictions.go

+	}
+	points -= 50 * len(scc.RequiredDropCapabilities)
+	if (points > 1000) {
+		return 1000


Why we're reducing value to 1000?

The goal was not to interfere with the other logic ... but that caps us at 10000, not 1000, will fix.

php-coder · 2017-06-23T10:39:07Z

pkg/security/scc/byrestrictions.go

+		return 1000
+	} else if (points < 0) {
+		return 0
+	} else {


Neat: we don't need last else block.

php-coder · 2017-06-23T10:40:05Z

pkg/security/scc/byrestrictions.go

+	} else {
+		points += 10 * len(scc.AllowedCapabilities)
+	}
+	points -= 50 * len(scc.RequiredDropCapabilities)


Should we add a check for all/ALL value here?

adelton · 2017-06-23T11:31:54Z

Fixed the volumes logic, added tests, rebased on master -> aa30746.

adelton · 2017-06-23T11:48:55Z

Addressed comments by @php-coder -> 03b8ccf.

pweil- · 2017-06-23T11:58:58Z

@liggitt fyi

deads2k · 2017-06-23T12:33:44Z

please hold for #14701

php-coder · 2017-06-23T12:48:49Z

pkg/security/scc/byrestrictions_test.go

 	scc := newSCC(false, kapi.SELinuxStrategyMustRunAs, kapi.RunAsUserStrategyMustRunAs)
 	scc.Volumes = []kapi.FSType{kapi.FSTypeHostPath}
 	actualPoints := pointValue(scc)
-	if actualPoints != 12 { //1 (SELinux) + 1 (User) + 10 (host path volume)
-		t.Errorf("volume score was not added to the scc point value correctly!")
+	if actualPoints != 125000 { //10000 (SELinux) + 10000 (User) + 100000 (host path volume) + 5000 capabilities


Having these magic numbers and comments with explanation what they means, make me thinking about extracting the constants. Maybe it's too much here. WDYT?

I don't consider myself expert on Golang/OpenShift best practices to do that refactoring. If you do that for the original pkg/security/scc/byrestrictions.go, I'd certainly make my patch align with that approach.

Ok, I've submitted #15116

adelton · 2017-06-26T10:26:06Z

Fixed the nils, updated the tests structure per comments by @php-coder, rebased on master -> 25f4554.

adelton · 2017-06-27T09:51:26Z

Now that #14701 was merged, rebased on master -> 99c756d.

deads2k · 2017-06-27T12:27:29Z

Now that #14701 was merged, rebased on master -> 99c756d.

I'll turn it back over to @php-coder . Thanks for waiting on the structural change.

enj · 2017-06-28T22:39:52Z

I do not know enough about the SCC stuff to really comment here. All the magic numbers are really concerning :/ @pweil- @php-coder I will have to defer to your expertise.

adelton · 2017-07-03T13:02:42Z

@php-coder, could we continue / finish the review?

php-coder · 2017-07-10T17:10:19Z

@adelton Let's wait until #15116 will be rejected/merged first.

adelton · 2017-07-11T07:11:15Z

Let's wait until #15116 will be rejected/merged first.

I've rebased this PR on top of #15116 and have it available locally. Should I push it on top of #15116 to allow review to continue, possibly as different branch?

php-coder · 2017-07-11T10:08:21Z

@adelton Just push it so I'll be able to review it as a separate commit. After #15116 you will rebase it again and my merged changes will gone.

adelton · 2017-07-11T13:12:11Z

Rebased on top of #15116 -> 07bf572.

adelton · 2017-08-07T08:32:17Z

Brought up-to-date with the kapi.CapabilityAll -> securityapi.AllowAllCapabilities change. Rebased on master -> d36ab40.

php-coder · 2017-08-07T11:04:11Z

/lgtm

adelton · 2017-08-08T09:55:47Z

/assign @smarterclayton

adelton · 2017-08-21T08:44:25Z

@pweil- could you please review and potentially approve instead of @smarterclayton?

adelton · 2017-08-22T14:07:35Z

@eparis, could you please review and potentially approve instead of @smarterclayton?

pweil-

can we get a test that ensures the bootstrapped sccs sort in the expected order so we can protect against that in the future?

other than that I have no issues with these changes. Thanks!

pweil- · 2017-08-22T14:16:57Z

pkg/security/securitycontextconstraints/byrestrictions.go

+	capsPoints += capAddOnePoints * points(len(scc.DefaultAddCapabilities))
+	if hasCap(string(securityapi.AllowAllCapabilities), scc.AllowedCapabilities) {
+		capsPoints += capAllowAllPoints
+	} else if hasCap("ALL", scc.AllowedCapabilities) {


is there going to be a sorting issue if all is used since we don't normalize to a case?

We do normalize, hasCap calls ToUpper.

ah, we do it in the search there but not the validation. Ok, disregard. Thanks

adelton · 2017-08-23T10:16:01Z

can we get a test that ensures the bootstrapped sccs sort in the expected order so we can protect against that in the future?

Should that test go to pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go or pkg/security/securitycontextconstraints/byrestrictions_test.go (or elsewhere)?

pweil- · 2017-08-23T12:35:46Z

I'm fine with it in the byrestrictions_test.go since it's pretty tightly coupled with that code.

php-coder · 2017-08-23T12:36:05Z

Should that test go to pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go or pkg/security/securitycontextconstraints/byrestrictions_test.go (or elsewhere)?

They should be in pkg/cmd/server/bootstrappolicy/securitycontextconstraints_test.go.

php-coder · 2017-08-23T12:40:25Z

I'm fine with it in the byrestrictions_test.go since it's pretty tightly coupled with that code.

My point is that we're testing not how ByPriority sorts items but application behavior. So the test in securitycontextconstraints_test.go is like an integration test and ensures that if we modify ByPriority it will tell us whether we're breaking the backward compatibility or not.

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

Please see #15923 for the current order test.

pweil- · 2017-08-23T13:29:24Z

Please see #15923 for the current order test.

Awesome, thanks. Let's get that one merged then we can merge this one since we've proven it works as expected

@simo5

Automatic merge from submit-queue (batch tested with PRs 15923, 16172) Check the order of bootstrapped SCCs. Related to #14530 and #14825. Cc @simo5 @openshift/sig-security

php-coder

/lgtm

php-coder · 2017-09-07T16:55:46Z

Now, when #15923 was merged we can merge this one.

@smarterclayton PTAL

enj · 2017-09-07T18:01:59Z

/lgtm

openshift-merge-robot · 2017-09-07T18:02:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adelton, enj, 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/security/OWNERS~~ [enj]

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

enj · 2017-09-07T18:02:56Z

I believe this has been discussed to death on IRC and BJ already.

@php-coder @adelton I will let you guys handle the test updates.

openshift-merge-robot · 2017-09-07T20:16:26Z

Automatic merge from submit-queue (batch tested with PRs 14825, 15756, 16178, 16188, 16189)

php-coder reviewed Jun 23, 2017

View reviewed changes

adelton force-pushed the issue-14530 branch from 2bfbb6c to aa30746 Compare June 23, 2017 11:31

adelton force-pushed the issue-14530 branch from aa30746 to 03b8ccf Compare June 23, 2017 11:48

adelton mentioned this pull request Jun 23, 2017

SCC that is more strict to the capabilities than "restricted" is not getting used #14530

Closed

php-coder reviewed Jun 23, 2017

View reviewed changes

adelton force-pushed the issue-14530 branch from 03b8ccf to 25f4554 Compare June 26, 2017 10:23

adelton force-pushed the issue-14530 branch from 25f4554 to 99c756d Compare June 27, 2017 09:50

php-coder mentioned this pull request Jul 10, 2017

Use constants for representation of the SCCs restrictiveness #15116

Merged

adelton changed the title ~~Make SCC with less capabilities more restrictive.~~ Make SCC with less capabilities more restrictive. [waiting for #15116] Jul 11, 2017

adelton force-pushed the issue-14530 branch from 99c756d to 07bf572 Compare July 11, 2017 13:11

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

adelton added 2 commits August 7, 2017 10:29

Make space in the point logic for capabilities accounting.

92b409f

Make SCC with less capabilities more restrictive.

d36ab40

adelton force-pushed the issue-14530 branch from aed78c7 to d36ab40 Compare August 7, 2017 08:31

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

pweil- reviewed Aug 22, 2017

View reviewed changes

adelton mentioned this pull request Aug 23, 2017

Check the order of bootstrapped SCCs. #15923

Merged

adelton mentioned this pull request Sep 4, 2017

hostnetwork SCC takes precedence over restricted for a pod that doesn't request host network #15933

Closed

php-coder approved these changes Sep 7, 2017

View reviewed changes

openshift-ci-robot assigned enj 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 a2020c3 into openshift:master Sep 7, 2017

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

Make SCC with less capabilities more restrictive. #14825

Make SCC with less capabilities more restrictive. #14825

Conversation

adelton commented Jun 22, 2017 • edited by php-coder Loading

adelton commented Jun 22, 2017

adelton commented Jun 22, 2017

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

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

adelton commented Jun 23, 2017 • edited Loading

adelton commented Jun 23, 2017

pweil- commented Jun 23, 2017

deads2k commented Jun 23, 2017

php-coder Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelton commented Jun 26, 2017

adelton commented Jun 27, 2017

deads2k commented Jun 27, 2017

enj commented Jun 28, 2017

adelton commented Jul 3, 2017

php-coder commented Jul 10, 2017

adelton commented Jul 11, 2017

php-coder commented Jul 11, 2017

adelton commented Jul 11, 2017

adelton commented Aug 7, 2017

php-coder commented Aug 7, 2017

adelton commented Aug 8, 2017

adelton commented Aug 21, 2017

adelton commented Aug 22, 2017

pweil- left a comment

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

adelton commented Aug 23, 2017

pweil- commented Aug 23, 2017

php-coder commented Aug 23, 2017

php-coder commented Aug 23, 2017

adelton commented Aug 23, 2017

pweil- commented Aug 23, 2017

php-coder left a comment • edited Loading

Choose a reason for hiding this comment

php-coder commented Sep 7, 2017

enj commented Sep 7, 2017

openshift-merge-robot commented Sep 7, 2017

enj commented Sep 7, 2017

openshift-merge-robot commented Sep 7, 2017

adelton commented Jun 22, 2017 •

edited by php-coder

Loading

adelton commented Jun 23, 2017 •

edited

Loading

php-coder Jun 23, 2017 •

edited

Loading

php-coder left a comment •

edited

Loading