Modify privileged SCC to allow to use all capabilities #12875

php-coder · 2017-02-08T19:10:37Z

pweil-

Thanks @php-coder this is what I was expecting. I think you can clean up and add unit tests for the strategies/regen if @smarterclayton is good with this enhancement.

pweil- · 2017-02-08T19:18:48Z

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

@@ -1256,6 +1256,9 @@ const (
 // Capability represent POSIX capabilities type
 type Capability string

+// CapabilityAll represent all POSIX capabilities types.
+var CapabilityAll Capability = "*"


There should be changes in here and v1 types to note on the description that * is an available wildcard

And to add CapabilityAll to the v1 version 😄

to note on the description that * is an available wildcard

@pweil- Do you mean that I should add such a comment to the Capability type above?

The comment should go on the SCC's AllowedCapabilities description so it shows up in generated documentation.

smarterclayton · 2017-02-08T21:08:42Z

Looks good to me barring paul's comments.

php-coder · 2017-02-09T17:11:21Z

I think you can clean up and add unit tests for the strategies/regen

@pweil- I'm sorry but it's not clear to me what exactly clean up I should do. Also could you elaborate on unit tests for the strategies/regen?

pweil- · 2017-02-09T17:33:38Z

There should be a mustrunas_test.go that needs to be updated for the new * support in addition to the admission test that you updated. By regen I mean you should run the generate* scripts so that your new comment update appears.

php-coder · 2017-02-23T18:34:50Z

PTAL again @pweil-

There is only one known issue: oc edit scc privileged accepts invalid configuration when allowedCapabilities: * and requiredDropCapabilities: FOO. Is it a bug, isn't it? Do you want to fix it as part of this PR or we can merge PR and fix issue later?

pweil- · 2017-02-23T18:37:58Z

🐛 let's fix it now

php-coder · 2017-02-27T14:47:40Z

It's ready to be merged. PTAL @pweil-

pweil- · 2017-02-27T14:56:51Z

vendor/k8s.io/kubernetes/pkg/securitycontextconstraints/capabilities/mustrunas.go

+	allowedAdd := makeCapSet(s.allowedCaps)
+	allowAllCaps := allowedAdd.Has(string(api.CapabilityAll))
+	if allowAllCaps {
+		if len(s.requiredDropCapabilities) > 0 {


this validation doesn't belong here. s here represents a strategy created from an SCC api object. You have validation that ensures that this case should never happen in the api object itself.

This should be for testing the pod/container requests against the ability to add everything.

If anything, that validation should be in the New... method since it is validation against the invariants. Assuming you're trying to protect against misconfiguration of the parameters. Can that even happen? Do we need a migration of an invalid strategy?

I've pushed the code that is addressing your first comment.

Do we need a migration of an invalid strategy?

While technically it was possible to create a SCC with * as the allowedCapabilities value, most likely no one uses it because it didn't have a special meaning.

… Constraints.

pweil- · 2017-02-28T19:12:04Z

Sgtm

…

On Mon, Feb 27, 2017 at 10:39 Vyacheslav Semushin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In vendor/k8s.io/kubernetes/pkg/securitycontextconstraints/capabilities/mustrunas.go <#12875 (comment)>: > @@ -101,9 +101,21 @@ func (s *defaultCapabilities) Validate(pod *api.Pod, container *api.Container) f return allErrs } + allowedAdd := makeCapSet(s.allowedCaps) + allowAllCaps := allowedAdd.Has(string(api.CapabilityAll)) + if allowAllCaps { + if len(s.requiredDropCapabilities) > 0 { I've pushed the code that is addressing your first comment. Do we need a migration of an invalid strategy? While technically it was possible to create a SCC with * as the allowedCapabilities value, most likely no one uses it because it didn't have a special meaning. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12875 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE_PU9WBfXQGYFDWuCZPsqGqR97riJapks5rgu44gaJpZM4L7O4I> .

pweil-

LGTM

pweil- · 2017-03-08T20:42:00Z

[test] - @php-coder can you get a BZ or trello card for this, it needs added to the QE test cases. Merging on green.

openshift-bot · 2017-03-08T20:45:27Z

Evaluated for origin test up to 25ade46

php-coder · 2017-03-08T21:54:33Z

@pweil- I'm not aware of any existing Trello/BZ. Do you want me to create one?

openshift-bot · 2017-03-08T22:06:04Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/47/) (Base Commit: aff7b5b)

php-coder · 2017-03-08T22:12:02Z

Something wrong with our infrastructure:

16:19:50 RUNNING HANDLER [openshift_node : restart node] ********************************
16:19:51 
fatal: [localhost]: FAILED! => {
16:19:51     "changed": false, 
16:19:51     "failed": true
16:19:51 }
16:19:51 
16:19:51 MSG:
16:19:51 
16:19:51 Unable to start service origin-node: Job for origin-node.service failed because the control process exited with error code. See "systemctl status origin-node.service" and "journalctl -xe" for details.
16:19:51 
16:19:51 
16:19:51 PLAY RECAP *********************************************************************
16:19:51  [WARNING]: Could not create retry file '/usr/share/ansible/openshift-
16:19:51 ansible/playbooks/byo/config.retry'.         [Errno 13] Permission denied:
16:19:51 u'/usr/share/ansible/openshift-ansible/playbooks/byo/config.retry'
16:19:51 localhost                  : ok=484  changed=132  unreachable=0    failed=1

The full log is here: https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_install/34/consoleFull

@stevekuznetsov Is it a known problem? Should I create an issue? Is it a test-flake or permanent failure?

pweil- · 2017-03-09T12:59:25Z

Do you want me to create one?

yes

stevekuznetsov · 2017-03-09T13:03:26Z

Other jobs have been succeeding -- are you sure that is not something from this PR?

php-coder · 2017-03-09T13:34:45Z

Other jobs have been succeeding -- are you sure that is not something from this PR?

@stevekuznetsov I see that another PR has just failed with the same error: openshift/openshift-ansible#3554 (comment) Log is here: https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_openshift_ansible/2/consoleFull

stevekuznetsov · 2017-03-09T13:40:02Z

I see. Please make an issue -- I'll look into it now.

php-coder · 2017-03-16T10:38:50Z

test flake #13318

sdodson · 2017-03-16T13:19:44Z

@smarterclayton QE saw this once[1] in a particular environment but never again. The suggested workaround is to add set the timeout=30 in /etc/ansible.cfg. There's a few upstream issues[2][3].

[defaults]
timeout = 30

1- https://bugzilla.redhat.com/show_bug.cgi?id=1427025
2- ansible/ansible#14426
3 - ansible/ansible#13278

smarterclayton · 2017-03-16T15:10:19Z

I'll set that in origin-gce, thanks [merge]

…

On Thu, Mar 16, 2017 at 9:19 AM, Scott Dodson ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton> QE saw this once[1] in a particular environment but never again. The suggested workaround is to add set the timeout=30 in /etc/ansible.cfg. There's a few upstream issues[2][3]. [defaults] timeout = 30 1- https://bugzilla.redhat.com/show_bug.cgi?id=1427025 2- ansible/ansible#14426 <ansible/ansible#14426> 3 - ansible/ansible#13278 <ansible/ansible#13278> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12875 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p5M4k6pPo1cJxziyV3mDqtYMero_ks5rmTbygaJpZM4L7O4I> .

php-coder · 2017-03-16T17:00:23Z

That's something new:

TASK [validate-public : Validate the public address] ***************************
Thursday 16 March 2017  16:05:43 +0000 (0:00:00.420)       0:15:29.851 ******** 
fatal: [localhost]: FAILED! => {"changed": false, "content": "", "failed": true, "msg": "Status code was not [200]: An unknown error occurred: ''", "redirected": false, "status": -1, "url": "https://api.prtest-5a37c28-320.origin-ci-int-gce.dev.rhcloud.com:443/healthz/ready"}

php-coder · 2017-03-16T17:03:18Z

Should I fill an issue for that?

stevekuznetsov · 2017-03-16T17:21:06Z

There's one here: openshift/origin-gce#15

re[merge]

openshift-bot · 2017-03-16T17:21:21Z

Evaluated for origin merge up to 25ade46

openshift-bot · 2017-03-16T18:37:38Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/111/) (Base Commit: b9ff323) (Image: devenv-rhel7_6072)

jcperezamin · 2017-05-05T18:49:29Z

When is this change is going to be available and which version? I trying to setup elasticsearch 5.3.2 on Openshift Origin 1.5.0 and is not working out because I need to add Capabilities like IPC_LOCK and SYS_RESOURCE, in Kubernetes this is working fine. Please fix it!. Thanks

stevekuznetsov · 2017-05-05T18:54:57Z

@jcperezamin this should be available in the following releases:

$ git tag --contains edb08ab5b46be11a18603b7450039fc11c102bee
v3.6.0-alpha.0
v3.6.0-alpha.1

php-coder · 2017-05-05T20:42:59Z

@jcperezamin On old versions you may modify privileged SCC and allow these capabilities.

php-coder · 2017-05-05T20:44:56Z

P.S. Something like this should do the trick:
oc patch scc/privileged -p '{"allowedCapabilities": ["IPC_LOCK", "SYS_RESOURCE"]}'

jcperezamin · 2017-05-06T01:35:09Z

Thanks, is working out now!

deads2k · 2017-07-06T15:46:41Z

@smarterclayton @liggitt I think this pull needs to be reverted. It changes the meaning of a kube API in an incompatible way, which prevents unification and makes our podspecs incompatible with kube podspecs. We still have time before 3.6 ships.

deads2k · 2017-07-06T15:52:04Z

Unless the comment is lying (which I suppose it may be).

php-coder · 2017-07-06T18:11:31Z

@deads2k I don't see how "It changes the meaning of a kube API". It modifies SecurityContextContraints which as you know is OpenShift-specific API and has nothing related to Kube API.

.. and makes our podspecs incompatible with kube podspecs

It doesn't affect pod spec. Otherwise, could you provide an example?

Unless the comment is lying (which I suppose it may be).

Which one comment?

deads2k · 2017-07-06T18:50:36Z

@php-coder you say "To allow all capabilities you may use '*'." If that is the "normal" behavior upstream, then you should create the constant upstream. Assuming you didn't do that because that is not the upstream behavior, that means that value of "*" only works properly when submitting a PodSpec.SecurityContext.Capabilities against an openshift server.

smarterclayton · 2017-07-06T19:08:43Z

Should only apply to SecurityContextConstraints

deads2k · 2017-07-06T19:16:41Z

Should only apply to SecurityContextConstraints

Ok, that would be good. That would mean the doc and constant are in the wrong spot, but we could keep it otherwise. It also suggests that we may want to separate types.

php-coder · 2017-07-07T16:09:12Z

That would mean the doc and constant are in the wrong spot, but we could keep it otherwise.

@deads2k I can move it to a proper place. Where it should be? Just above SecurityContextConstraints type definition?

deads2k · 2017-07-07T16:26:58Z

@deads2k I can move it to a proper place. Where it should be? Just above SecurityContextConstraints type definition?

Yeah, and the comment about usage goes inside of the SCC capabilities values since it only applies there

@deads2k

Automatic merge from submit-queue Move CapabilityAll from k8s types and rename it to AllowAllCapabilities Move `CapabilityAll` from k8s types to openshift, close to the SCC definition and also rename it to `AllowAllCapabilities`. Addresses #12875 (comment) and fixes #15273 PTAL @deads2k @pweil- CC @simo5

@simo5

Automatic merge from submit-queue (batch tested with PRs 51337, 47080, 52646, 52635, 52666). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.. PodSecurityPolicy.allowedCapabilities: add support for * to allow to request any capabilities **What this PR does / why we need it**: Prior this change there was no way to allow to pods to request any capabilities. Cluster admin had always specify a full list of capabilities explicitly. Because there are many of them, it gets tedious. This PR makes possible to use `*` to allow all possible capabilities. Non-paranoid (and lazy) cluster admins can use it. Those who are super strict and paranoid of course won't use it because `*` allows capabilities that don't exist today but may be introduced in the future. "privileged" PSP in examples was modified to allow privileged users to use this feature. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #50055 **Special notes for your reviewer**: This functional is already present in OpenShift: openshift/origin#12875 and openshift/origin#15135 **Release note**: ```release-note PSP: add support for using `*` as a value in `allowedCapabilities` to allow to request any capabilities ``` CC @simo5 @pweil- @gyliu513 @liqlin2015

pweil- suggested changes Feb 8, 2017

View reviewed changes

smarterclayton added the api-approved label Feb 8, 2017

php-coder force-pushed the privileged_should_allow_all_capabilities branch from f6fd7a4 to 0b67a9c Compare February 10, 2017 14:15

php-coder changed the title ~~Modify privileged SCC to allow to use all capabilities~~ [WIP] Modify privileged SCC to allow to use all capabilities Feb 10, 2017

php-coder force-pushed the privileged_should_allow_all_capabilities branch 2 times, most recently from 73b6e03 to 057df4a Compare February 23, 2017 18:19

php-coder changed the title ~~[WIP] Modify privileged SCC to allow to use all capabilities~~ Modify privileged SCC to allow to use all capabilities Feb 23, 2017

php-coder force-pushed the privileged_should_allow_all_capabilities branch from 057df4a to d9f28a9 Compare February 27, 2017 14:39

pweil- reviewed Feb 27, 2017

View reviewed changes

php-coder added 3 commits February 27, 2017 16:47

UPSTREAM: <carry>: allow to use * as a capability in Security Context…

1b2aacb

… Constraints.

Modify privileged SCC to allow to use all capabilities.

edb08ab

Update autogenerated files.

25ade46

php-coder force-pushed the privileged_should_allow_all_capabilities branch from 11cad7f to 25ade46 Compare February 27, 2017 15:47

pweil- approved these changes Mar 8, 2017

View reviewed changes

openshift-bot merged commit 1a39743 into openshift:master Mar 16, 2017

php-coder deleted the privileged_should_allow_all_capabilities branch March 16, 2017 20:48

php-coder mentioned this pull request Mar 20, 2017

Update output of oc get scc command openshift/openshift-docs#3972

Merged

php-coder mentioned this pull request Jun 14, 2017

Improvementes to the SCC documentation openshift/openshift-docs#4554

Merged

php-coder mentioned this pull request Jul 11, 2017

Move CapabilityAll from k8s types and rename it to AllowAllCapabilities #15135

Merged

php-coder mentioned this pull request Aug 3, 2017

PSP: provide a simple way to allow all Linux capabilities kubernetes/kubernetes#50055

Closed

php-coder mentioned this pull request Aug 25, 2017

PodSecurityPolicy.allowedCapabilities: add support for * to allow to request any capabilities kubernetes/kubernetes#51337

Merged

Modify privileged SCC to allow to use all capabilities #12875

Modify privileged SCC to allow to use all capabilities #12875

Conversation

php-coder commented Feb 8, 2017 • edited Loading

pweil- left a comment

Choose a reason for hiding this comment

pweil- Feb 8, 2017

Choose a reason for hiding this comment

pweil- Feb 8, 2017

Choose a reason for hiding this comment

php-coder Feb 9, 2017

Choose a reason for hiding this comment

pweil- Feb 9, 2017

Choose a reason for hiding this comment

smarterclayton commented Feb 8, 2017

php-coder commented Feb 9, 2017

pweil- commented Feb 9, 2017

php-coder commented Feb 23, 2017

pweil- commented Feb 23, 2017

php-coder commented Feb 27, 2017

pweil- Feb 27, 2017

Choose a reason for hiding this comment

pweil- Feb 27, 2017

Choose a reason for hiding this comment

php-coder Feb 27, 2017

Choose a reason for hiding this comment

pweil- commented Feb 28, 2017 via email

pweil- left a comment

Choose a reason for hiding this comment

pweil- commented Mar 8, 2017

openshift-bot commented Mar 8, 2017

php-coder commented Mar 8, 2017

openshift-bot commented Mar 8, 2017

php-coder commented Mar 8, 2017

pweil- commented Mar 9, 2017

stevekuznetsov commented Mar 9, 2017

php-coder commented Mar 9, 2017

stevekuznetsov commented Mar 9, 2017

php-coder commented Mar 16, 2017

sdodson commented Mar 16, 2017

smarterclayton commented Mar 16, 2017 via email

php-coder commented Mar 16, 2017

php-coder commented Mar 16, 2017

stevekuznetsov commented Mar 16, 2017

openshift-bot commented Mar 16, 2017

openshift-bot commented Mar 16, 2017 • edited Loading

jcperezamin commented May 5, 2017

stevekuznetsov commented May 5, 2017

php-coder commented May 5, 2017

php-coder commented May 5, 2017

jcperezamin commented May 6, 2017

deads2k commented Jul 6, 2017

deads2k commented Jul 6, 2017

php-coder commented Jul 6, 2017

deads2k commented Jul 6, 2017 • edited Loading

smarterclayton commented Jul 6, 2017

deads2k commented Jul 6, 2017

php-coder commented Jul 7, 2017

deads2k commented Jul 7, 2017

php-coder commented Feb 8, 2017 •

edited

Loading

openshift-bot commented Mar 16, 2017 •

edited

Loading

deads2k commented Jul 6, 2017 •

edited

Loading