Allow setting volumes:["none"] to disallow all volume types in SCC #14625

liggitt · 2017-06-14T03:40:15Z

When reading old SCC objects, we default volumes: null to mean volumes: [*] for backwards compatibility.

If you wanted to disallow all volumes, previously you would specify volumes: []. Once we switch to storing in protobuf, [] will no longer be stored, which means we would default back to [*] the next time we read the object from etcd.

Given that an SCC that disallows all volumes is not usable (since all pods have a secret volume), my main concern is ensuring a previously useless SCC stored in etcd doesn't suddenly become a very permissive SCC.

This PR:

defaults an SCC with volumes: [] to volumes: [none] (so it can be persisted regardless of storage type and not escalate to allow all volumes on next read)
validates that an SCC that specifies volumes: [none] doesn't allow any other volume types

liggitt · 2017-06-14T03:44:32Z

[test]

liggitt · 2017-06-14T03:46:22Z

cc @openshift/api-review @ironcladlou @smarterclayton

deads2k · 2017-06-14T11:34:42Z

looks reasonable to me

smarterclayton · 2017-06-14T15:24:25Z

LGTM, API changes are ok (i briefly contemplated None but if anyone adds a formal none volume type I'll shoot them).

smarterclayton · 2017-06-14T15:25:34Z

@sdodson this is the reason we want to run oadm migrate storage prior to an etcd v3 upgrade (and what prompted the discussion on whether to always run migrate storage prior to upgrading).

…

On Wed, Jun 14, 2017 at 7:34 AM, David Eads ***@***.***> wrote: looks reasonable to me — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14625 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p9ldlGMPUf2D0m1TRasRkcuy9KjZks5sD8VUgaJpZM4N5WNG> .

openshift-bot · 2017-06-14T15:30:49Z

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 5

openshift-bot · 2017-06-14T15:30:50Z

Evaluated for origin merge up to 4b85a5a

liggitt · 2017-06-14T15:34:33Z

forgot I have to regenerate API docs, will update. hope I make it before I reach the head of the merge queue

liggitt · 2017-06-14T15:52:18Z

this is the reason we want to run oadm migrate storage prior to an etcd v3 upgrade

etcd v3 upgrade doesn't do json -> proto, does it? the next time the json is read from etcd v3, this would do the conversion properly

smarterclayton · 2017-06-14T15:56:02Z

It doesn't but better safe than sorry (old api server comes up and does bad things)

…

On Wed, Jun 14, 2017 at 11:52 AM, Jordan Liggitt ***@***.***> wrote: this is the reason we want to run oadm migrate storage prior to an etcd v3 upgrade etcd v3 upgrade doesn't do json -> proto, does it? the next time the json is read from etcd v3, this would do the conversion properly — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14625 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p18t4fdohxIayboqqiFT1-oD0DM8ks5sEAG1gaJpZM4N5WNG> .

liggitt · 2017-06-14T15:57:40Z

actually, an old API server fed volumes: ["none"] would also disallow all volume types, which is unexpectedly compatible

liggitt · 2017-06-14T18:33:44Z

hmm... concerning that jenkins was green (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2225/) when my UPSTREAM commit didn't pass the upstream commit checker (https://travis-ci.org/openshift/origin/builds/242888676)

@stevekuznetsov, are we not running that in jenkins?

smarterclayton · 2017-06-14T18:36:27Z

No, it runs in travis.

…

On Wed, Jun 14, 2017 at 2:33 PM, Jordan Liggitt ***@***.***> wrote: hmm... concerning that jenkins was green (https://ci.openshift.redhat. com/jenkins/job/test_pull_request_origin/2225/) when my UPSTREAM commit didn't pass the upstream commit checker (https://travis-ci.org/ openshift/origin/builds/242888676) @stevekuznetsov <https://github.com/stevekuznetsov>, are we not running that in jenkins? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14625 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pwPIASr2sBdxyyH2-SfGNml9iMxIks5sECePgaJpZM4N5WNG> .

liggitt · 2017-06-14T18:37:10Z

does red travis block merge?

smarterclayton · 2017-06-14T18:57:09Z

Not yet.

…

On Wed, Jun 14, 2017 at 2:37 PM, Jordan Liggitt ***@***.***> wrote: does red travis block merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14625 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p0s_F3h_GRffFJM4L5aM8B-1L_Quks5sEChYgaJpZM4N5WNG> .

stevekuznetsov · 2017-06-14T21:07:46Z

We definitely do run that in the check job -- before we jump into hack/env and run the tests:

+ RESTORE_AND_VERIFY_GODEPS=1
+ make verify-commits -j
hack/verify-upstream-commits.sh
[WARNING] No compiled `commitchecker` binary was found. Attempting to build one using:
[WARNING]   $ hack/build-go.sh tools/rebasehelpers/commitchecker
++ Building go targets for linux/amd64: tools/rebasehelpers/commitchecker
/data/src/github.com/openshift/origin/hack/build-go.sh took 14 seconds
===== Verifying UPSTREAM Commits =====
SUCCESS: All commits are valid.

Is that checker broken?

liggitt · 2017-06-14T21:13:42Z

Is that checker broken?

It turned Travis red, so I don't think so

stevekuznetsov · 2017-06-14T21:26:36Z

Once we've got the right git state we run:

# run commitchecker outside release container as it needs
# access to git; also explicitly force godeps verification
branch="$( git rev-parse --abbrev-ref --symbolic-full-name HEAD )"
if [[ "${branch}" == "master" ]]; then
  RESTORE_AND_VERIFY_GODEPS=1 make verify-commits -j
fi

OS_BUILD_ENV_PRESERVE=_output/scripts hack/env TEST_KUBE='true' JUNIT_REPORT='true' make check -j -k

stevekuznetsov · 2017-06-14T21:43:17Z

@liggitt do you have a git reflog to show us the history of this branch? It looks like Travis recorded the commit it merged as 2db9358 and our Jenkins CI says 793f7b1

stevekuznetsov · 2017-06-14T21:45:34Z

Nevermind... Travis was pointing at the merge commit but nevertheless had 793f7b1

We should probably create logging for commitchecker so we can debug post-facto

stevekuznetsov · 2017-06-14T21:46:21Z

@liggitt would appreciate it if you could push that bad ref up as a new PR for repro

smarterclayton · 2017-06-15T05:45:23Z

[test]

php-coder · 2017-06-15T14:29:54Z

vendor/k8s.io/kubernetes/pkg/api/validation/validation.go

+		}
+		if hasNone {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("volumes"), scc.Volumes,
+				"if 'none' is specified, no other values are allowed"))


If you inline this to the if, it will be possible to use field.Index() to complain on particular element.

Do we need a test for that?

php-coder · 2017-06-15T14:31:16Z

vendor/k8s.io/kubernetes/pkg/api/validation/validation.go

+	if len(scc.Volumes) > 1 {
+		hasNone := false
+		for _, fsType := range scc.Volumes {
+			if fsType == api.FSTypeNone {


Is it possible that user specify 'None' or 'NONE'? Would it work?

php-coder · 2017-06-15T14:32:26Z

Does Kubernetes has the same issue? Are they going to do the same?

liggitt · 2017-06-15T14:34:05Z

Does Kubernetes has the same issue? Are they going to do the same?

They do not, because the first version of PSP had the Volumes field, so they don't have to default nil to mean "allow all"

php-coder · 2017-06-15T14:40:26Z

vendor/k8s.io/kubernetes/pkg/api/v1/defaults.go

+
+	default:
+		// defaults the volume slice of the SCC.
+		// In order to support old clients the boolean fields will always take precedence.


In order to support old clients the boolean fields will always take precedence.

About what "boolean fields" this comment is talking? Does it mean AllowHostDirVolumePlugin?

php-coder · 2017-06-15T17:00:39Z

It would be good to also ignore this new value when we're comparing SCCs to sort them by priority:

origin/pkg/security/scc/byrestrictions.go

Lines 69 to 70 in 84b8802

    
           case kapi.FSTypeSecret, kapi.FSTypeConfigMap, 
        
           	kapi.FSTypeEmptyDir, kapi.FSTypeDownwardAPI:

liggitt · 2017-06-15T21:27:12Z

updated byrestrictions, will hold for move of scc types to origin package

deads2k · 2017-06-16T15:31:16Z

Chances of my pull testing green today are low. Go ahead and merege, but remind me that I need to mess with this.

liggitt · 2017-06-16T16:20:51Z

[merge][severity:blocker]

smarterclayton · 2017-06-16T17:17:47Z

[test]

smarterclayton · 2017-06-16T19:49:42Z

[test]

openshift-bot · 2017-06-16T20:15:55Z

Evaluated for origin test up to 2136bfc

openshift-bot · 2017-06-16T22:11:40Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2335/) (Base Commit: ee67917) (PR Branch Commit: 2136bfc)

liggitt added the area/compatibility label Jun 14, 2017

liggitt modified the milestone: 3.6.0 Jun 14, 2017

php-coder reviewed Jun 15, 2017

View reviewed changes

liggitt added 2 commits June 15, 2017 17:24

UPSTREAM: <carry>: SCC FSType none

55a4e34

Update openapi, scc sorting

2136bfc

mtnbikenc mentioned this pull request Jun 16, 2017

Run storage upgrade pre and post master upgrade openshift/openshift-ansible#4476

Merged

smarterclayton merged commit ca96ee9 into openshift:master Jun 16, 2017

liggitt deleted the scc-volumes branch June 17, 2017 01:02

php-coder mentioned this pull request Jul 21, 2017

architecture/additional_concepts/authorization: improvements to the list of allowed volume types openshift/openshift-docs#4831

Merged

Allow setting volumes:["none"] to disallow all volume types in SCC #14625

Allow setting volumes:["none"] to disallow all volume types in SCC #14625

Conversation

liggitt commented Jun 14, 2017 • edited Loading

liggitt commented Jun 14, 2017

liggitt commented Jun 14, 2017

deads2k commented Jun 14, 2017

smarterclayton commented Jun 14, 2017 • edited by liggitt Loading

smarterclayton commented Jun 14, 2017 via email

openshift-bot commented Jun 14, 2017 • edited Loading

openshift-bot commented Jun 14, 2017

liggitt commented Jun 14, 2017

liggitt commented Jun 14, 2017

smarterclayton commented Jun 14, 2017 via email

liggitt commented Jun 14, 2017

liggitt commented Jun 14, 2017

smarterclayton commented Jun 14, 2017 via email

liggitt commented Jun 14, 2017

smarterclayton commented Jun 14, 2017 via email

stevekuznetsov commented Jun 14, 2017

liggitt commented Jun 14, 2017

stevekuznetsov commented Jun 14, 2017

stevekuznetsov commented Jun 14, 2017

stevekuznetsov commented Jun 14, 2017

stevekuznetsov commented Jun 14, 2017

smarterclayton commented Jun 15, 2017

php-coder Jun 15, 2017

Choose a reason for hiding this comment

php-coder Jun 15, 2017

Choose a reason for hiding this comment

php-coder Jun 15, 2017

Choose a reason for hiding this comment

php-coder commented Jun 15, 2017

liggitt commented Jun 15, 2017

php-coder Jun 15, 2017

Choose a reason for hiding this comment

liggitt Jun 15, 2017

Choose a reason for hiding this comment

php-coder commented Jun 15, 2017

liggitt commented Jun 15, 2017

deads2k commented Jun 16, 2017

liggitt commented Jun 16, 2017

smarterclayton commented Jun 16, 2017

smarterclayton commented Jun 16, 2017

openshift-bot commented Jun 16, 2017

openshift-bot commented Jun 16, 2017

liggitt commented Jun 14, 2017 •

edited

Loading

smarterclayton commented Jun 14, 2017 •

edited by liggitt

Loading

openshift-bot commented Jun 14, 2017 •

edited

Loading