Skip to content

Add SCC.AllowPrivilegeEscalation (aka no-new-privs)#18398

Closed
php-coder wants to merge 5 commits intoopenshift:masterfrom
php-coder:scc_allow_privilege_escalation_option
Closed

Add SCC.AllowPrivilegeEscalation (aka no-new-privs)#18398
php-coder wants to merge 5 commits intoopenshift:masterfrom
php-coder:scc_allow_privilege_escalation_option

Conversation

@php-coder
Copy link
Contributor

@php-coder php-coder commented Feb 1, 2018

This PR adds two new members to SecurityContextConstraints:

  • AllowPrivilegeEscalation -- controls whether a container may request privilege escalation or not (by setting SecurityContext.AllowPrivilegeEscalation: true). When SCC doesn't define this member, it defaulted to true for backward/Kubernetes compatibility reasons.
  • DefaultAllowPrivilegeEscalation -- sets default value for container's SecurityContext.AllowPrivilegeEscalation field when it wasn't explicitly specified.

Kubernetes already has such support for their PodSecurityPolicy (kubernetes/kubernetes#47019) and, with that PR, OpenShift cluster admins will also have ability to deny execution of SUID-binaries.

Trello: https://trello.com/c/slL9l4pw/29-3-no-new-privs

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2018
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 1, 2018
@openshift-merge-robot openshift-merge-robot added needs-api-review vendor-update Touching vendor dir or related files labels Feb 1, 2018
scc.Volumes = []securityapi.FSType{volumeTypes[c.Rand.Intn(len(volumeTypes))]}

// match to the defaulting logic that is used to be backward compatible
scc.AllowPrivilegeEscalation = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks off. Why doesn't false roundtrip?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that to the original object were applied defaults that always set this value to true? But I'm not sure. I just know that it works :(


// match to the defaulting logic that is used to be backward compatible
scc.AllowPrivilegeEscalation = true
scc.DefaultAllowPrivilegeEscalation = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks weird. Why can't it roundtrip for a value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably because this field has omitempty and after roundtrip it's always nil.

@php-coder
Copy link
Contributor Author

I (still) stuck on serialization_test.go that now is failing with:

oundtrip.go:216: 	round tripping to /v1, Kind=SecurityContextConstraintsList v1.SecurityContextConstraintsList
roundtrip.go:332: SecurityContextConstraintsList: diff: 
object.Items[0].AllowedFlexVolumes:
a: []security.AllowedFlexVolume{}
b: []security.AllowedFlexVolume(nil)
object.Items[0].AllowPrivilegeEscalation:
a: true
b: false

This means that defaults don't get applied to SecurityContextConstraintsList. I don't know why, all my attempts failed.

@php-coder
Copy link
Contributor Author

@deads2k any advice would be appreciated!

@php-coder
Copy link
Contributor Author

@enj Thanks, Mo! I fixed the protobuf definition (here is the commit: 795b807) but it doesn't help. :-(

@php-coder php-coder force-pushed the scc_allow_privilege_escalation_option branch from 795b807 to 1e496e1 Compare March 5, 2018 16:08
@php-coder
Copy link
Contributor Author

/retest

@enj
Copy link
Contributor

enj commented Mar 22, 2018

@php-coder see php-coder#3 to get your tests passing correctly.

@openshift-ci-robot openshift-ci-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 Mar 22, 2018
@php-coder php-coder force-pushed the scc_allow_privilege_escalation_option branch from e17205f to b0880f4 Compare March 28, 2018 15:57
@php-coder php-coder changed the title [WIP] SCC: add AllowPrivilegeEscalation/DefaultAllowPrivilegeEscalation (aka no-new-privs) SCC: add AllowPrivilegeEscalation/DefaultAllowPrivilegeEscalation (aka no-new-privs) Mar 28, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2018
@php-coder php-coder changed the title SCC: add AllowPrivilegeEscalation/DefaultAllowPrivilegeEscalation (aka no-new-privs) SCC: add AllowPrivilegeEscalation and DefaultAllowPrivilegeEscalation (aka no-new-privs) Mar 28, 2018
@php-coder php-coder changed the title SCC: add AllowPrivilegeEscalation and DefaultAllowPrivilegeEscalation (aka no-new-privs) SCC: add AllowPrivilegeEscalation (aka no-new-privs) Mar 28, 2018
@php-coder php-coder changed the title SCC: add AllowPrivilegeEscalation (aka no-new-privs) Add SCC.AllowPrivilegeEscalation (aka no-new-privs) Mar 28, 2018
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 26, 2018
@php-coder
Copy link
Contributor Author

Rebased on top of #19624

@php-coder
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2018
@php-coder php-coder force-pushed the scc_allow_privilege_escalation_option branch from 9c5674e to fb8505d Compare June 27, 2018 14:00

// now add DefaultAllowPrivilegeEscalation to the SecurityContextConstraints
scc.DefaultAllowPrivilegeEscalation = &yes
scc.AllowPrivilegeEscalation = &no
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given 5ca1e18#diff-0d673702b8c4f7d9b631d3bae9cc2d90R136, isn't this an invalid SCC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stlaz
Copy link
Contributor

stlaz commented Jun 28, 2018

Thank you @php-coder for fixing the test cases.
/retest
/lgtm
We still need to wait for #19624 to be merged, though.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: php-coder, stlaz
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mfojtik

Assign the PR to them by writing /assign @mfojtik in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@php-coder php-coder force-pushed the scc_allow_privilege_escalation_option branch from 42075fc to 82cadc2 Compare June 28, 2018 20:02
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2018
@php-coder php-coder force-pushed the scc_allow_privilege_escalation_option branch from 82cadc2 to 2a9b2d7 Compare June 28, 2018 20:41
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2018
@php-coder php-coder force-pushed the scc_allow_privilege_escalation_option branch from 2a9b2d7 to 0014ceb Compare June 29, 2018 08:51
@php-coder
Copy link
Contributor Author

php-coder commented Jun 29, 2018

Currently it fails with:

Running hack/lib/start.sh:291: executing 'oc get configmap kube-scheduler --namespace kube-system --config='/tmp/openshift/test-cmd/openshift.local.config/master/admin.kubeconfig'' expecting success; re-trying every 0.25s until completion or 160.000s...
SUCCESS after 0.308s: hack/lib/start.sh:291: executing 'oc get configmap kube-scheduler --namespace kube-system --config='/tmp/openshift/test-cmd/openshift.local.config/master/admin.kubeconfig'' expecting success; re-trying every 0.25s until completion or 160.000s
error: unable to decode "STDIN": no kind "List" is registered for the internal version of group "template.openshift.io"
error: no objects passed to create

@bparees @deads2k Could it be related to rebase?

Jan Wozniak and others added 4 commits June 29, 2018 14:48
…ions for controlling no-new-privs flag.

This commit adds two new members to SecurityContextConstraints:

* AllowPrivilegeEscalation -- controls whether a container may request privilege escalation or not
  (by setting SecurityContext.AllowPrivilegeEscalation: true). When SCC doesn't define this member,
  it defaulted to "true" for backward/Kubernetes compatibility reasons.

* DefaultAllowPrivilegeEscalation -- sets default value for container's
  SecurityContext.AllowPrivilegeEscalation field when it wasn't explicitly specified.
- github.com/blang/semver: b38d23b8782a487059e8fc8773e9a5b228a77cb6
- github.com/openshift/api: a93a22bdd4ff0e883bcc5a5417a80c47e2459b7f
@php-coder php-coder force-pushed the scc_allow_privilege_escalation_option branch from 8b48ad5 to dcf5568 Compare June 29, 2018 13:17
@php-coder
Copy link
Contributor Author

Currently it fails with:

This issue should be resolved now. Thanks to @liggitt :

<liggitt> your bump(*) commit is re-adding &corev1.List{} to github.com/openshift/api/template/v1/register.go
<liggitt> and reverting a fix made in the rebase and sitting in api#master
<liggitt> unpin openshift/api and set it back to master in glide.yaml

@bparees
Copy link
Contributor

bparees commented Jun 29, 2018

this is being handled in #20152

@bparees bparees closed this Jun 29, 2018
@openshift-ci-robot
Copy link

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

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_networking_minimal 154a797 link /test extended_networking_minimal
ci/prow/launch-gcp 42075fc link /test launch-gcp
ci/openshift-jenkins/cross 82cadc2 link /test cross
ci/openshift-jenkins/extended_image_ecosystem 82cadc2 link /test extended_image_ecosystem
ci/prow/verify dcf5568 link /test verify
ci/prow/unit dcf5568 link /test unit
ci/openshift-jenkins/extended_conformance_install dcf5568 link /test extended_conformance_install
ci/prow/gcp dcf5568 link /test gcp
ci/openshift-jenkins/cmd dcf5568 link /test cmd

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.

Details

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.

@php-coder php-coder deleted the scc_allow_privilege_escalation_option branch June 30, 2018 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-api-review sig/security size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. vendor-update Touching vendor dir or related files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants