SCC: add AllowedFlexVolumes to manage a whitelist of allowed flexvolumes drivers #15558

php-coder · 2017-07-31T10:51:22Z

Proposal: kubernetes/community#723
Trello: https://trello.com/c/YT6sNEay/61-5-sccfsi-psp-scc-flex-volume-support
Examples: #15558 (comment)

deads2k · 2017-07-31T11:53:55Z

@stevekuznetsov is there an easy way to trigger auto-labeling when a types.go file is modified?

deads2k · 2017-07-31T11:57:56Z

pkg/security/apis/security/v1/types.go

@@ -81,6 +81,9 @@ type SecurityContextConstraints struct {
 	// used to generate a value for a pod the first non-wildcard profile will be used as
 	// the default.
 	SeccompProfiles []string `json:"seccompProfiles,omitempty" protobuf:"bytes,20,opt,name=seccompProfiles"`
+	// AllowedFlexDrivers is a whitelist of allowed Flexvolume drivers.
+	// Empty or nil indicates that all drivers may be used.
+	AllowedFlexDrivers []string `json:"allowedFlexDrivers" protobuf:"bytes,21,opt,name=allowedFlexDrivers"`


So this field is used when the user already has flexVolume power right? If they don't, then this is never used?

If that's the case, I'd expect a validation rule and some more detail in the doc. Also, why not move up underneath Volumes? The proto number can still be 21 up there.

So this field is used when the user already has flexVolume power right? If they don't, then this is never used?

Correct.

If that's the case, I'd expect a validation rule and some more detail in the doc.

At present, it's possible to have non-empty AllowedFlexDrivers and prohibited the usage of flexVolumes at the same time. Should we add validation to that case, and force user to have an empty AllowedFlexDrivers?

Also, why not move up underneath Volumes? The proto number can still be 21 up there.

Hm. It's not clear what do you mean, so I put AllowedFlexDrivers field right after Volumes in the pkg/security/apis/security/types.go file. I left the order the same in pkg/security/apis/security/v1/types.go file, let me know if you want to see it updated too.

Also, why not move up underneath Volumes?

Got it. Done.

deads2k · 2017-07-31T12:00:32Z

pkg/security/securitycontextconstraints/provider.go

-				allErrs = append(allErrs, field.Invalid(
-					fldPath.Child("volumes").Index(i), string(fsType),
-					fmt.Sprintf("%s volumes are not allowed to be used", string(fsType))))
+		if len(s.scc.AllowedFlexDrivers) > 0 && sccutil.SCCAllowsFSType(s.scc, securityapi.FSTypeFlexVolume) {


if the SCC doesn't allow flex volumes and you have any, wouldn't have already put errors on the return?

I'm sorry but I didn't understand what you mean here :(

if the volume type is completely disallowed, line 306 would already have added an error. there's no need to add another error saying the specific flex driver is not allowed. making a single loop over all the volumes would let you short-circuit adding specific errors if there are general ones.

allowAllVolumeTypes := sccutil.SCCAllowsAllVolumes(s.scc) allowedVolumeTypes := sccutil.FSTypeToStringSet(s.scc.Volumes) for i, v := range pod.Spec.Volumes { fsType, err := sccutil.GetVolumeFSType(v) if err != nil { ... add the error... continue } if !allowAllVolumeTypes && !allowedVolumes.Has(string(fsType)) { ... add an error about this volume type not being allowed... continue } if v.FlexVolume != nil && len(s.scc.AllowedFlexDrivers) > 0 { ... ensure the particular flex volume is allowed... } }

deads2k · 2017-07-31T12:02:15Z

pkg/security/securitycontextconstraints/provider.go

+
+				for _, allowedDriver := range s.scc.AllowedFlexDrivers {
+					driver := v.FlexVolume.Driver
+					if driver != allowedDriver {


doesn't this mean that if I allow ["foo", "bar"] and request one with "bar", I'll get an error even though I'm allowed? How about adding some tests here.

Good catch, thanks! It's fixed now. I'll add tests after passing API review.

stevekuznetsov · 2017-07-31T13:13:36Z

@deads2k yes IIRC

0xmichalis · 2017-07-31T13:15:10Z

@stevekuznetsov is there an easy way to trigger auto-labeling when a types.go file is modified?

Yes, there is. Will work on it today or tomorrow.

liggitt · 2017-07-31T14:04:04Z

pkg/security/apis/security/types.go

+	// AllowedFlexDrivers is a whitelist of allowed Flexvolume drivers.
+	// Empty or nil indicates that all drivers may be used.
+	// +optional
+	AllowedFlexDrivers []string


making this an array of structs lets us expand options in the future (like setting the readOnly option), similar to the discussion around allowedHostPaths.

pweil- · 2017-08-01T12:08:49Z

pkg/oc/cli/describe/describer.go

@@ -1865,6 +1866,13 @@ func stringOrNone(s string) string {
 	return "<none>"


nit: change this to call stringOrDefaultValue

php-coder · 2017-08-01T16:53:45Z

How to fix the following error?

++ Building go targets for linux/amd64: cmd/openshift cmd/oc cmd/kubefed pkg/sdn/plugin/sdn-cni-plugin vendor/github.com/containernetworking/cni/plugins/ipam/host-local vendor/github.com/containernetworking/cni/plugins/main/loopback
# github.com/openshift/origin/pkg/security/apis/security/v1
pkg/security/apis/security/v1/generated.pb.go:3450: undefined: allowedFlexvolume

I tried to run hack/update-generated-protobuf.sh many times but it didn't help.

php-coder · 2017-08-01T17:43:20Z

How to fix the following error?

<claytonc> semushin: your "casttype=allowedFlexVolumes" is the problem
<claytonc> did you set the protobuf tags yourself?
<claytonc> you should remove the protobuf tags from your new generated code
<claytonc> remove the changes to generated.pb.go
<claytonc> then regenerate
<semushin> claytonc, yes, I added them manually (do we have another way?)
<claytonc> semushin: they are generated for you automatically by generated-protobuf
<claytonc> never add protobuf tags by hand unless you have a reason to (the reasons are small)
<claytonc> only add json tags
<claytonc> and honestly we should have json tag generation but it's easier today

php-coder · 2017-08-02T11:15:46Z

Test flake #10327
/test cmd

php-coder · 2017-08-02T15:10:18Z

Examples:

oc describe when all allowed:

$ oc describe scc restricted | grep Flex
  Allowed Flexvolumes:			<all>

oc describe with 2 allowed volumes:

$ oc describe scc restricted | grep Flex
  Allowed Flexvolumes:			driver=google/plus,driver=foo

oc get -o yaml:

$ oc get scc restricted -o yaml  | grep Flex
allowedFlexVolumes: []

error message when pod request fails:

Error from server (Forbidden): error when creating "testing-flexvolume.json": pods "test-flex" is forbidden: unable to validate against any security context constraint: [provider restricted: .spec.containers[0].securityContext.volumes[1]: Invalid value: "example/bash": Flexvolume driver is not allowed to be used]

oc edit fails when flex volumes aren't allowed to be used but the AllowedFlexVolumes field isn't empty:

# securitycontextconstraints "restricted" was not valid:
# * allowedFlexVolumes: Invalid value: []security.AllowedFlexVolume{security.AllowedFlexVolume{Driver:""}, security.AllowedFlexVolume{Driver:""}}: if usage of Flexvolumes are disallowed, the list of allowed Flexvolumes must be empty

oc edit fails when driver wasn't specified:

# securitycontextconstraints "restricted" was not valid:
# * allowedFlexVolumes[0].driver: Required value: must specify a driver

php-coder · 2017-09-04T13:09:29Z

Test flake #15900
/test cmd

=== RUN   TestRouterDuplications
--- FAIL: TestRouterDuplications (0.00s)
	router_test.go:752: Unable to start http server: listen tcp 172.18.9.82:8080: bind: address already in use

Test fake #16144
/test end_to_end

PLAY RECAP *********************************************************************
 [WARNING]: Could not create retry file '/usr/share/ansible/openshift-
ansible/playbooks/byo/config.retry'.         [Errno 13] Permission denied:
u'/usr/share/ansible/openshift-ansible/playbooks/byo/config.retry'
localhost                  : ok=91   changed=6    unreachable=0    failed=1   


Failure summary:

  1. Host:     localhost
     Play:     Verify Requirements
     Task:     openshift_health_check
     Message:  One or more checks failed
     Details:  check "package_version":
               Not all of the required packages are available at their requested version
               docker:1.12 
               Please check your subscriptions and enabled repositories.

/test extended_conformance_install_update

adelton · 2017-09-05T08:08:53Z

For testing this pull request, I try to create SCC with

volumes:
- configMap
- downwardAPI
- emptyDir
- persistentVolumeClaim
- projected
- secret
- flexVolume
allowedFlexVolumes:
- jezek

but I get

$ oc create -f restricted-flex-jezek.yaml
Error from server (BadRequest): error when creating "restricted-flex-jezek.yaml": SecurityContextConstraints in version "v1" cannot be handled as a SecurityContextConstraints: only encoded map or array can be decoded into a struct

What is the correct way to specify the list of drivers?

php-coder · 2017-09-05T10:27:55Z

What is the correct way to specify the list of drivers?

@adelton Try this one:

allowedFlexVolumes:
- driver: jezek

php-coder · 2017-09-05T12:56:49Z

/retest

adelton · 2017-09-05T13:14:40Z

Try this one:

allowedFlexVolumes:
- driver: jezek

Thanks. Would it make sense to amend the documentation to show this syntax?

php-coder · 2017-09-05T13:19:40Z

Would it make sense to amend the documentation to show this syntax?

The Trello card has documentation label, so this new feature will be documented. I'm going to do it after this after merging this PR. Nevertheless you can prepare PR for that ;)

adelton · 2017-09-05T13:21:54Z

From functional perspective, this feature works.

My only concern is an extremely long and not really helpful error message when I try to create pod with flexVolume and the driver is not listed in allowedFlexVolumes:

Error from server (Forbidden): error when creating "openshift-origin/test-flex-pod.json": pods "test-flex" is forbidden: unable to validate against any security context constraint: [spec.volumes[0]: Invalid value: "flexVolume": flexVolume volumes are not allowed to be used provider restricted-flex-jezek: .spec.securityContext.volumes[0].driver: Invalid value: "example.test/bind-mount": Flexvolume driver is not allowed to be used spec.volumes[0]: Invalid value: "flexVolume": flexVolume volumes are not allowed to be used spec.volumes[0]: Invalid value: "flexVolume": flexVolume volumes are not allowed to be used spec.volumes[0]: Invalid value: "flexVolume": flexVolume volumes are not allowed to be used spec.volumes[0]: Invalid value: "flexVolume": flexVolume volumes are not allowed to be used]

liggitt · 2017-09-05T13:33:42Z

My only concern is an extremely long and not really helpful error message when I try to create pod with flexVolume and the driver is not listed in allowedFlexVolumes:

That's a pre-existing issue with the way SCC admission presents failures. A separate issue to improve that is probably a good idea.

php-coder · 2017-09-05T13:36:17Z

That's a pre-existing issue with the way SCC admission presents failures. A separate issue to improve that is probably a good idea.

It looks like a regression or a wrong merge. I'll look at it today more closely.

liggitt · 2017-09-05T13:49:29Z

It looks like a regression or a wrong merge. I'll look at it today more closely.

It looks normal to me (needs improvement, but normal). You get an error message for every SCC you have access to that does not allow flex volumes.

php-coder · 2017-09-05T15:01:05Z

My only concern is an extremely long and not really helpful error message

@adelton I couldn't reproduce it. In my case the error is the following:

Error from server (Forbidden): error when creating "flex-pod.yaml": pods "flexpod" is forbidden: unable to validate against any security context constraint: [provider restricted: .spec.securityContext.volumes[0].driver: Invalid value: "example-one.com/test": Flexvolume driver is not allowed to be used]

php-coder · 2017-09-05T15:06:24Z

End-to-end tests fails because of #16144

php-coder · 2017-09-05T15:16:09Z

@deads2k @enj PTAL

deads2k · 2017-09-05T15:19:20Z

/approve

@enj you own merge.

enj · 2017-09-05T15:38:31Z

/lgtm

No logic changes since last LGTM.

openshift-merge-robot · 2017-09-05T15:38:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, 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:

~~OWNERS~~ [deads2k]

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

openshift-bot · 2017-09-05T20:10:09Z

/retest

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

openshift-bot · 2017-09-06T04:14:10Z

/retest

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

adelton · 2017-09-06T06:11:02Z

I couldn't reproduce it. In my case the error is the following:

If you create bunch of differently named copies of restricted SCC, with

  groups:
  - system:authenticated

developer will have access to all of them even if they will not give the access to the flexVolume driver. And then the error message will repeat for all of them, but without showing the name of the SCC the error refers to it is really cryptic.

php-coder · 2017-09-06T10:16:08Z

If you create bunch of differently named copies of restricted SCC,

Aha, I see. This is indeed a different case. Please, create a separate issue for that. Thanks!

openshift-bot · 2017-09-06T12:18:10Z

/retest

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

openshift-merge-robot · 2017-09-06T16:39:21Z

Automatic merge from submit-queue

openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 31, 2017

openshift-merge-robot assigned deads2k and enj Jul 31, 2017

php-coder force-pushed the scc_for_flexvolumes branch from f18f739 to 785e94c Compare July 31, 2017 11:31

php-coder changed the title ~~[WIP] SCC: add AllowedFlexDrivers to manage a while list of allowed flexvolumes drivers~~ SCC: add AllowedFlexDrivers to manage a while list of allowed flexvolumes drivers Jul 31, 2017

deads2k added the needs-api-review label Jul 31, 2017

deads2k reviewed Jul 31, 2017

View reviewed changes

php-coder force-pushed the scc_for_flexvolumes branch from 785e94c to 110dacd Compare July 31, 2017 13:54

php-coder changed the title ~~SCC: add AllowedFlexDrivers to manage a while list of allowed flexvolumes drivers~~ [WIP] SCC: add AllowedFlexDrivers to manage a while list of allowed flexvolumes drivers Jul 31, 2017

liggitt reviewed Jul 31, 2017

View reviewed changes

php-coder force-pushed the scc_for_flexvolumes branch 2 times, most recently from bd56ef8 to 7578913 Compare July 31, 2017 14:25

pweil- reviewed Aug 1, 2017

View reviewed changes

php-coder force-pushed the scc_for_flexvolumes branch from 7578913 to dd7dc45 Compare August 1, 2017 16:38

openshift-merge-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 1, 2017

php-coder changed the title ~~[WIP] SCC: add AllowedFlexDrivers to manage a while list of allowed flexvolumes drivers~~ [WIP] SCC: add AllowedFlexvolumes to manage a while list of allowed flexvolumes drivers Aug 1, 2017

php-coder changed the title ~~[WIP] SCC: add AllowedFlexvolumes to manage a while list of allowed flexvolumes drivers~~ [WIP] SCC: add AllowedFlexvolumes to manage a white list of allowed flexvolumes drivers Aug 1, 2017

php-coder force-pushed the scc_for_flexvolumes branch from dd7dc45 to 44c1f27 Compare August 1, 2017 17:54

php-coder changed the title ~~[WIP] SCC: add AllowedFlexvolumes to manage a white list of allowed flexvolumes drivers~~ [WIP] SCC: add AllowedFlexVolumes to manage a white list of allowed flexvolumes drivers Aug 2, 2017

php-coder force-pushed the scc_for_flexvolumes branch from 44c1f27 to adf7679 Compare August 2, 2017 15:01

Update auto-generated files.

f67f171

php-coder force-pushed the scc_for_flexvolumes branch from c726ddb to f67f171 Compare September 4, 2017 11:20

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

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

openshift-merge-robot merged commit 074acdc into openshift:master Sep 6, 2017

php-coder deleted the scc_for_flexvolumes branch September 6, 2017 16:45

ironcladlou mentioned this pull request Sep 8, 2017

hack/test-cmd.sh:114: executing 'oc new-project 'cmd-admin'' timeout #15900

Closed

php-coder mentioned this pull request Sep 14, 2017

SCC: document AllowedFlexVolumes field openshift/openshift-docs#5260

Merged

		@@ -1865,6 +1866,13 @@ func stringOrNone(s string) string {
		return "<none>"

SCC: add AllowedFlexVolumes to manage a whitelist of allowed flexvolumes drivers #15558

SCC: add AllowedFlexVolumes to manage a whitelist of allowed flexvolumes drivers #15558

Conversation

php-coder commented Jul 31, 2017 • edited Loading

deads2k commented Jul 31, 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

stevekuznetsov commented Jul 31, 2017

0xmichalis commented Jul 31, 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

php-coder commented Aug 1, 2017

php-coder commented Aug 1, 2017

php-coder commented Aug 2, 2017

php-coder commented Aug 2, 2017

php-coder commented Sep 4, 2017 • edited Loading

adelton commented Sep 5, 2017

php-coder commented Sep 5, 2017

php-coder commented Sep 5, 2017

adelton commented Sep 5, 2017

php-coder commented Sep 5, 2017 • edited Loading

adelton commented Sep 5, 2017 • edited by php-coder Loading

liggitt commented Sep 5, 2017

php-coder commented Sep 5, 2017 • edited Loading

liggitt commented Sep 5, 2017 • edited Loading

php-coder commented Sep 5, 2017

php-coder commented Sep 5, 2017

php-coder commented Sep 5, 2017

deads2k commented Sep 5, 2017

enj commented Sep 5, 2017

openshift-merge-robot commented Sep 5, 2017

openshift-bot commented Sep 5, 2017

openshift-bot commented Sep 6, 2017

adelton commented Sep 6, 2017

php-coder commented Sep 6, 2017

openshift-bot commented Sep 6, 2017

openshift-merge-robot commented Sep 6, 2017

php-coder commented Jul 31, 2017 •

edited

Loading

php-coder commented Sep 4, 2017 •

edited

Loading

php-coder commented Sep 5, 2017 •

edited

Loading

adelton commented Sep 5, 2017 •

edited by php-coder

Loading

php-coder commented Sep 5, 2017 •

edited

Loading

liggitt commented Sep 5, 2017 •

edited

Loading