SCC: recognize that SELinux levels can be logically equivalent #16432

php-coder · 2017-09-19T13:17:06Z

For example: s0:c0,c6 is the same as s0:c6,c0

adelton · 2017-09-19T13:52:17Z

We should be able to support ranges, like s0-s0:c0.c255. Also, the categories shouldn't be sorted as strings as the numerical part can have multiple digits.

I'm also not sure that we are looking for equality here -- don't we look for dominance and superset instead?

php-coder · 2017-09-19T15:30:37Z

We should be able to support ranges, like s0-s0:c0.c255.

Ok, I'll add it.

Also, the categories shouldn't be sorted as strings as the numerical part can have multiple digits.

It doesn't matter how they are sorted as we're sorting both parts. In other words, the sorting logic doesn't affect the result.

php-coder · 2017-09-19T15:51:53Z

We should be able to support ranges, like s0-s0:c0.c255.

Added. @adelton PTAL

php-coder · 2017-09-22T15:01:24Z

/retest

adelton · 2017-09-25T07:41:33Z

It doesn't matter how they are sorted as we're sorting both parts. In other words, the sorting logic doesn't affect the result.

But are we really looking for equality? IMHO, s0:c0.c2 should admit pod which asks for s0:c1, shouldn't it?

rhatdan · 2017-09-25T13:27:52Z

I think we should omit single category labels altogether. People understand the uniqueness, they don't understand dominance. MCS Separation has a hidden secret, dominance. In order to stop people confusion we should stick to a single count of Categories, and guarantee uniqueness.

php-coder · 2017-09-26T15:26:51Z

@adelton @rhatdan I don't understand what are you talking about unfortunately :( If you want me to edit this PR somehow then could you be more specific, please?

smarterclayton · 2017-09-26T16:30:10Z

pkg/security/securitycontextconstraints/selinux/mustrunas.go

+func parseCategories(categories string) []string {
+	parts := strings.Split(categories, ",")
+
+	// "c0.c3" => [ "c0", "c1", "c2", c3" ]


why isn't this

categories := strings.Split('.') sort.Strings(categories)

? Why are you go into deeper parsing?

Oh, I see that this is an expansion. Please add godoc to the method

smarterclayton · 2017-09-26T16:30:55Z

pkg/security/securitycontextconstraints/selinux/mustrunas_test.go

+			expectedSeLinux: newValidOpts(),
+			expectedMsg:     "",
+		},
+		"valid level with abbreviated categories": { // "s0:c0.c3" == "s0:c0,c1,c2,c3"


need some additional tests, test edges cases and multiple equivalences.

What do you mean by "multiple equivalences"?

Miciah

Maybe the parseFoo functions would be better named canonicalizeFoo, assuming I correctly understand the intent.

Miciah · 2017-09-28T23:25:20Z

pkg/security/securitycontextconstraints/selinux/mustrunas.go

+func parseCategories(categories string) []string {
+	parts := strings.Split(categories, ",")
+
+	// "c0.c3" => [ "c0", "c1", "c2", c3" ]


s/c3"/"c3"/

Miciah · 2017-09-28T23:27:39Z

pkg/security/securitycontextconstraints/selinux/mustrunas.go

+			if err1 == nil && err2 == nil && from < to {
+				parts = make([]string, to-from+1)
+				for i := from; i <= to; i++ {
+					parts[i] = fmt.Sprintf("c%d", i)


This will go out of bounds if categoryRange[0] is not c0 (e.g., parseCategories("c1.c4") panics—might be a good test case). You cannot use the same variable for the index and the format, at least not without some arithmetic.

@Miciah Indeed, good catch, thanks!

php-coder · 2017-10-03T18:16:18Z

More code and tests were added. Feedback is highly appreciated!

php-coder · 2017-10-09T15:46:26Z

Test flake #14085
/test extended_conformance_gce

php-coder · 2017-10-09T15:47:35Z

Test flake #16402
/test cmd

php-coder · 2017-10-09T15:49:47Z

Bug in Kubernetes: kubernetes/kubernetes#53590
/test origin-ut

php-coder · 2017-10-09T15:51:27Z

@openshift/sig-security PTAL

php-coder · 2017-10-17T13:57:29Z

Ping.

adelton · 2017-10-17T14:09:37Z

In general, my question is if the SCC allows c0.c4 and the pod asks for c0, should it be permitted or not?

php-coder · 2017-10-23T15:30:25Z

In general, my question is if the SCC allows c0.c4 and the pod asks for c0, should it be permitted or not?

<semushin> pweil, WDYT? Sounds like yes, a pod should be permitted; but it could be a case for another PR.
<pweil> semushin, yes, I think that would match, probably a lower priority follow up

@adelton Could you create an issue (or even a PR) for that?

eparis · 2017-10-23T16:26:37Z

What dan is saying in #16432 (comment) is that you should NOT try to determine dominance or any special relationship between category sets. That is all policy and you can't know what policy says. Some policies may have very wierd inter-relations between level and category sets. Please only account of exact equivalence.

This really needs to be a libselinux function :-(

@stephensmalley I can't find a libselinux function to really help us. We just want to know is the string s-s0:c1,c2,c3 the same as so:c1.c3. I was hoping that a combination of context_new, context_str and selinux_file_context_cmp could be used to get this answer but it doesn't look like we have anything which does some form of semantic category normalization...

So maybe this PR is the best thing we can do.

stephensmalley · 2017-10-23T17:30:02Z

security_canonicalize_context(3) from libselinux. Oddly, no man page.
Sample usage:

$ cat canon.c
#include <selinux/selinux.h>
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char **argv)
{
	const char *ctx;
	char *canoncon;
	int rc;

	ctx = argv[1];
	rc = security_canonicalize_context(ctx, &canoncon);
	if (rc < 0)
		perror(ctx);
	else {
		printf("%s\n", canoncon);
		free(canoncon);
	}
	exit(rc);
}
$ make LDLIBS+=-lselinux canon
$ $ ./canon unconfined_u:unconfined_r:unconfined_t:s0:c0,c1,c2
unconfined_u:unconfined_r:unconfined_t:s0:c0.c2

php-coder · 2018-01-25T12:33:16Z

The PR has been updated. In order to merge in the upstream, it was decided to make it as simple as possible and solve only the original issue with ordering. No support for ranges and expansion at this point.

@openshift/sig-security PTAL

php-coder · 2018-01-25T12:34:36Z

pkg/security/securitycontextconstraints/selinux/mustrunas.go


 	"k8s.io/apimachinery/pkg/util/validation/field"
 	api "k8s.io/kubernetes/pkg/apis/core"
+	psputil "k8s.io/kubernetes/pkg/security/podsecuritypolicy/util"


I decided to use PSP's utils instead of duplicating them. That's why this PR has vendor-update tag.

pweil-

@php-coder do you think there is any benefit to just using the PSP strategy that is going to merge upstream for the comparison functions? They're just taking strings so it isn't type specific. It would require a change to the upstream PR but would reduce duplication here if we went that route. WDYT?

In any case this matches upstream and is LGTM

simo5 · 2018-01-25T13:35:07Z

@pweil- given we will eventually migrate away from SCC to PSP, I think I prefer to just copy here for now, or we'll have public functions in PSP that are used by nothing but PSP once we drop SCC. Also any change to PSP would end up with more maintenance to keep SCC working until we can drop it.
So IMO it is good as proposed.

simo5 · 2018-01-25T13:37:13Z

Actually I would even endorse copying the psputil function as a private function for mustrunas.go and avoid the vendoring warnings.

pweil- · 2018-01-25T13:40:05Z

@pweil- given we will eventually migrate away from SCC to PSP, I think I prefer to just copy here for now, or we'll have public functions in PSP that are used by nothing but PSP once we drop SCC. Also any change to PSP would end up with more maintenance to keep SCC working until we can drop it.
So IMO it is good as proposed.

I'm ok with this strategy, however if the comparisons were exposed in psputil and used then any changes we do upstream to enhance it wouldn't have to be copied, we'd just get them upon rebase. It is also not something we'd need to do an in depth review on when we decide to migrate to make sure we're not missing a difference. That burden is on your team though so it's your call. 👍

…make its meaning obvious.

…tions configurable.

… to improve code coverage.

php-coder · 2018-01-25T14:04:14Z

@pweil- @simo5 Thanks for the feedback!

I updated the PR and removed the dependency to PSP's util package.

simo5 · 2018-01-25T14:11:55Z

/lgtm

pweil-

/lgtm

openshift-ci-robot · 2018-01-25T15:03:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: php-coder, pweil-, simo5

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~~ [pweil-,simo5]

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

php-coder · 2018-01-25T22:18:52Z

/test gcp

openshift-bot · 2018-01-26T06:50:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-01-26T16:52:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

php-coder · 2018-01-26T21:22:22Z

/test gcp

openshift-merge-robot · 2018-01-27T01:38:48Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2018-01-27T01:59:56Z

Automatic merge from submit-queue (batch tested with PRs 16432, 18308, 18311).

openshift-ci-robot · 2018-01-27T02:38:59Z

@php-coder: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/gcp	`bcd5b33`	link	`/test gcp`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 19, 2017

openshift-merge-robot assigned pweil- and deads2k Sep 19, 2017

php-coder force-pushed the gh15627_scc_and_selinux_levels branch from 4513467 to 391aba5 Compare September 19, 2017 15:51

php-coder mentioned this pull request Sep 20, 2017

PSP: improve parsing and validation of SELinux levels kubernetes/kubernetes#52799

Merged

smarterclayton reviewed Sep 26, 2017

View reviewed changes

Miciah reviewed Sep 28, 2017

View reviewed changes

php-coder force-pushed the gh15627_scc_and_selinux_levels branch 2 times, most recently from b08f614 to 4d85ee1 Compare October 3, 2017 18:14

php-coder force-pushed the gh15627_scc_and_selinux_levels branch from 4d85ee1 to 3ff59a9 Compare October 9, 2017 14:44

php-coder force-pushed the gh15627_scc_and_selinux_levels branch from 3ff59a9 to 7f13165 Compare October 23, 2017 15:27

openshift deleted a comment from php-coder Oct 23, 2017

openshift-merge-robot added the vendor-update Touching vendor dir or related files label Jan 25, 2018

openshift-ci-robot added the sig/security label Jan 25, 2018

php-coder commented Jan 25, 2018

View reviewed changes

pweil- reviewed Jan 25, 2018

View reviewed changes

php-coder added 4 commits January 25, 2018 15:02

SCC: recognize that SELinux levels can be logically equivalent.

c09fc9f

selinux/mustrunas_test.go(TestMustRunAsValidate): rename a member to …

aa158a6

…make its meaning obvious.

selinux/mustrunas_test.go(TestMustRunAsValidate): make SCC SeLinux op…

c3440df

…tions configurable.

selinux/mustrunas_test.go(TestMustRunAsValidate): add more test cases…

bcd5b33

… to improve code coverage.

php-coder force-pushed the gh15627_scc_and_selinux_levels branch from 84d7867 to bcd5b33 Compare January 25, 2018 14:02

openshift-merge-robot removed the vendor-update Touching vendor dir or related files label Jan 25, 2018

openshift-ci-robot assigned simo5 Jan 25, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 25, 2018

pweil- approved these changes Jan 25, 2018

View reviewed changes

openshift-merge-robot merged commit 3a9bec1 into openshift:master Jan 27, 2018

php-coder deleted the gh15627_scc_and_selinux_levels branch January 28, 2018 22:15

SCC: recognize that SELinux levels can be logically equivalent #16432

SCC: recognize that SELinux levels can be logically equivalent #16432

Conversation

php-coder commented Sep 19, 2017 • edited Loading

adelton commented Sep 19, 2017

php-coder commented Sep 19, 2017

php-coder commented Sep 19, 2017

php-coder commented Sep 22, 2017

adelton commented Sep 25, 2017

rhatdan commented Sep 25, 2017

php-coder commented Sep 26, 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

Miciah 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

php-coder commented Oct 3, 2017

php-coder commented Oct 9, 2017

php-coder commented Oct 9, 2017

php-coder commented Oct 9, 2017

php-coder commented Oct 9, 2017

php-coder commented Oct 17, 2017

adelton commented Oct 17, 2017

php-coder commented Oct 23, 2017 • edited Loading

eparis commented Oct 23, 2017

stephensmalley commented Oct 23, 2017 • edited by php-coder Loading

php-coder commented Jan 25, 2018

Choose a reason for hiding this comment

pweil- left a comment

Choose a reason for hiding this comment

simo5 commented Jan 25, 2018

simo5 commented Jan 25, 2018

pweil- commented Jan 25, 2018

php-coder commented Jan 25, 2018

simo5 commented Jan 25, 2018

pweil- left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 25, 2018

php-coder commented Jan 25, 2018

openshift-bot commented Jan 26, 2018

openshift-bot commented Jan 26, 2018

php-coder commented Jan 26, 2018

openshift-merge-robot commented Jan 27, 2018

openshift-merge-robot commented Jan 27, 2018

openshift-ci-robot commented Jan 27, 2018 • edited Loading

php-coder commented Sep 19, 2017 •

edited

Loading

php-coder commented Oct 23, 2017 •

edited

Loading

stephensmalley commented Oct 23, 2017 •

edited by php-coder

Loading

openshift-ci-robot commented Jan 27, 2018 •

edited

Loading