Security context - types, kubelet, admission #7343

pweil- · 2015-04-27T03:03:46Z

This PR is a subset of the original code in #6287 and covers the following for SecurityContexts:

New sub resource
Kubelet support for setting flags based on SecurityContext Settings
An always admit and always deny admission plugin
Validation that defaults settings in the SecurityContext based on Container settings and vice versa

@erictune, some items I'd like to call out

the SecurityContext is not inlined as recommended in WIP: Security Context #6287 (comment) but I was unsure about the example for ResourceList that you mentioned. If this implementation is incorrect I could use some guidance on where to look for the ResourceList defaulting
I interpreted the "in the kubelet allow..." statements (WIP: Security Context #6287 (comment)) as the kubelet always allows a SecurityContext if it makes it to the kubelet in a pod config. Since I have not run a pod straight from a manifest I will confirm this on Monday in case I'm incorrect.
I went ahead and used a provider as designed in https://github.com/GoogleCloudPlatform/kubernetes/blob/master/docs/design/security_context.md#security-context-provider but it doesn't contain all the SecurityConstraint items as previously submitted. If you'd still like to stay away from a formal provider and just have the logic in the kubelet I can move it.

I need to do some more testing on the functionality to make sure I carried it all over correctly but I wanted to get this under review while I did.

@erictune @smarterclayton @pmorie @liggitt @derekwaynecarr PTAL

History: #6287

googlebot · 2015-04-27T03:03:47Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

erictune · 2015-04-27T14:43:43Z

Suggest:

internal representation should only have container.securityContext.privileged, not container.privileged, and so on.
v1beta3, etc, have both
conversion from v1beta3 to internal fails on mismatch.
conversion from internal to v1beta3 fills in both.
conversion and check happen in pkg/api/v1beta*/conversion.go
in a future version > v1beta3, we drop the container.privileged field.

@bgrant0607 check above statements if you have time.

pweil- · 2015-04-27T14:53:28Z

10-4 - on it

bgrant0607 · 2015-04-27T17:54:30Z

@erictune's proposal is correct. See container resources for an example where we used this approach.

erictune · 2015-04-28T22:53:01Z

Conversion happens before admission control, and conversion sets corresponding values in SecurityContext if pod.privileged or pod.capabilities is set. SecurityContextDeny disallows pods with any values set in SecurityContext. So, are all pods with pod.privileged or pod.capabilities denied? I think that is okay, but just wanted to clarify my understanding.

erictune · 2015-04-28T22:56:06Z

pkg/api/types.go

+// both the Container AND the SecurityContext will result in an error.
+type SecurityContext struct {
+	// Capabilities are the capabilities to add/drop when running the container
+	// DUPLICATE OF CONTAINER FIELD


replace DUPLICATE OF CONTAINER FIELD with Must match Container.Privileged or be unset.

erictune · 2015-04-29T00:00:30Z

Almost there...

pweil- · 2015-04-29T01:38:23Z

whew - git reflog saved me.

@erictune the suggestion to move the check to the conversion really cleaned this up. The defaulting happens in the default.go of each v1beta* now. If an SC is already set then it won't be changed. The conversion errors if the container fields don't match the SC fields that exist. Otherwise a normal conversion occurs. Validation is left to do normal validation-y checks.

Descriptions have been added to all fields in the feedback commit.

@yifan-gu, @jonboulle - I removed the direct references to the docker options in the comments for everything except disabled. Perhaps disabled doesn't even need to be there, if you aren't specifying labels then maybe we just take no action. If you have thoughts on how to express this better please let me know.

Thanks for the feedback so far.

pweil- · 2015-04-29T02:09:07Z

Conversion happens before admission control, and conversion sets corresponding values in SecurityContext if pod.privileged or pod.capabilities is set. SecurityContextDeny disallows pods with any values set in SecurityContext. So, are all pods with pod.privileged or pod.capabilities denied? I think that is okay, but just wanted to clarify my understanding.

As it stand right now, yes, it would. It creates a conflict I didn't think of, the internal api is now completely based on SecurityContext but anything with SecurityContext is denied. I'll fix that first thing tomorrow. I'll allow a SecurityContext if it only has Privileged and Caps set. Good catch.

pmorie · 2015-05-05T16:17:24Z

This LGTM but needs to be squashed. Until #7779 goes in, we'll have to do the swagger changes as a separate PR.

pmorie · 2015-05-05T17:32:20Z

Okay, even with #7779 merged, there is still an upstream go-restful bug that makes the validation tests break after the swagger spec is regenerated. We need that fixed before we can regen the swagger spec after this.

pmorie · 2015-05-05T17:43:39Z

Actually, I think I was wrong. I ran the script locally and it produced the correct output.

pmorie · 2015-05-05T18:03:39Z

LGTM. I will merge on green.

pmorie · 2015-05-05T18:05:13Z

api/swagger-spec/v1beta1.json

@@ -7623,7 +7627,8 @@
      "description": "restart policy for all containers within the pod; one of RestartPolicyAlways, RestartPolicyOnFailure, RestartPolicyNever"
     },
     "terminationGracePeriodSeconds": {
-      "$ref": "int64",
+      "type": "integer",


@bgrant0607 I'm not sure why this wasn't changed in #7779, but it looks like the pointers are being handled correctly now.

Security context - types, kubelet, admission

pmorie · 2015-05-05T20:26:37Z

Merged

zmerlynn · 2015-05-05T22:35:51Z

This was reverted, and was the cause of #7805. See there for more context.

pmorie · 2015-05-06T00:11:50Z

For the record, the revert was itself reverted.

smarterclayton · 2015-05-06T00:32:11Z

It was verted.

On May 5, 2015, at 8:12 PM, Paul Morie notifications@github.com wrote:

For the record, the revert was itself reverted.

—
Reply to this email directly or view it on GitHub.

googlebot added the cla: no label Apr 27, 2015

pweil- mentioned this pull request Apr 27, 2015

WIP: Security Context #6287

Closed

satnam6502 assigned erictune Apr 27, 2015

pweil- force-pushed the security-context-types branch 3 times, most recently from 4784de1 to 63b8e2b Compare April 27, 2015 14:06

erictune mentioned this pull request Apr 28, 2015

Demo service account to replace kubernetes-ro use cases in demos #7444

Closed

erictune reviewed Apr 28, 2015
View reviewed changes

pweil- force-pushed the security-context-types branch from 63b8e2b to c183492 Compare April 29, 2015 01:05

pweil- force-pushed the security-context-types branch from bec3167 to 52da39e Compare April 29, 2015 01:48

pweil- force-pushed the security-context-types branch from 52da39e to 5cf28c8 Compare April 29, 2015 12:59

thockin mentioned this pull request May 5, 2015

spec: security and isolators conflated? appc/spec#349

Open

pweil- force-pushed the security-context-types branch from 69e69bc to fa20bae Compare May 5, 2015 13:24

pweil- force-pushed the security-context-types branch 2 times, most recently from 5527d3c to 8c4c4f6 Compare May 5, 2015 17:23

pweil- force-pushed the security-context-types branch from 8c4c4f6 to 4ea84c3 Compare May 5, 2015 17:34

pweil- added 2 commits May 5, 2015 13:46

security context initial implementation - squash

982bf19

update swagger

5e5b904

pweil- force-pushed the security-context-types branch from 4ea84c3 to 5e5b904 Compare May 5, 2015 17:53

pmorie reviewed May 5, 2015
View reviewed changes

pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 5, 2015

pmorie added a commit that referenced this pull request May 5, 2015

Merge pull request #7343 from pweil-/security-context-types

1625e23

Security context - types, kubelet, admission

pmorie merged commit 1625e23 into kubernetes:master May 5, 2015

pweil- mentioned this pull request May 5, 2015

SecurityContext admission clean up #7792

Merged

This was referenced May 5, 2015

Kubelet: Add rkt as a runtime option #7743

Merged

Revert "Security context - types, kubelet, admission" #7809

Merged

smarterclayton mentioned this pull request May 5, 2015

#7743 introduced weird glibc dep #7805

Closed

zmerlynn mentioned this pull request May 5, 2015

Revert "Revert "Security context - types, kubelet, admission"" #7814

Merged

mbforbes mentioned this pull request May 6, 2015

Support manually-created and generated conversion functions #7832

Merged

pmorie mentioned this pull request May 7, 2015

Security contexts and volumes #7925

Closed

pweil- mentioned this pull request May 10, 2015

Update Security Context Documentation #7987

Merged

caesarxuchao mentioned this pull request Jun 2, 2015

add v1 to hack/test-go; let default value of Container.Security Context to be nil #9101

Merged

mtardy mentioned this pull request Jul 28, 2022

Deprecation and removal of the SecurityContextDeny admission plugin #111516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security context - types, kubelet, admission #7343

Security context - types, kubelet, admission #7343

pweil- commented Apr 27, 2015

googlebot commented Apr 27, 2015

erictune commented Apr 27, 2015

pweil- commented Apr 27, 2015

bgrant0607 commented Apr 27, 2015

erictune commented Apr 28, 2015

erictune Apr 28, 2015

erictune commented Apr 29, 2015

pweil- commented Apr 29, 2015

pweil- commented Apr 29, 2015

pmorie commented May 5, 2015

pmorie commented May 5, 2015

pmorie commented May 5, 2015

pmorie commented May 5, 2015

pmorie May 5, 2015

pmorie commented May 5, 2015

zmerlynn commented May 5, 2015

pmorie commented May 6, 2015

smarterclayton commented May 6, 2015

Security context - types, kubelet, admission #7343

Security context - types, kubelet, admission #7343

Conversation

pweil- commented Apr 27, 2015

googlebot commented Apr 27, 2015

erictune commented Apr 27, 2015

pweil- commented Apr 27, 2015

bgrant0607 commented Apr 27, 2015

erictune commented Apr 28, 2015

erictune Apr 28, 2015

Choose a reason for hiding this comment

erictune commented Apr 29, 2015

pweil- commented Apr 29, 2015

pweil- commented Apr 29, 2015

pmorie commented May 5, 2015

pmorie commented May 5, 2015

pmorie commented May 5, 2015

pmorie commented May 5, 2015

pmorie May 5, 2015

Choose a reason for hiding this comment

pmorie commented May 5, 2015

zmerlynn commented May 5, 2015

pmorie commented May 6, 2015

smarterclayton commented May 6, 2015