Updates to KEP for Windows Security Options by ddebroy · Pull Request #975 · kubernetes/enhancements

ddebroy · 2019-04-19T00:33:03Z

Addressed review comments from PR KEP for Windows security context #972
Added sample pod specs with the fields populated
Clarified test case

ddebroy · 2019-04-19T00:34:36Z

/assign @PatrickLang @michmike @liggitt

ddebroy · 2019-04-22T16:23:24Z

/kind api-change

fejta-bot · 2019-04-22T17:00:27Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

liggitt · 2019-04-24T13:45:50Z

keps/sig-windows/20190418-windows-security-context.md

 ## Proposal

-In this KEP we propose a new field named `WindowsOptions` in the `PodSecurityContext` struct (associated with a pod spec) and the `SecurityContext` struct (associated with each container in a pod). `WindowsOptions` will be a pointer to a new struct of type `WindowsSecurityOptions`. The `WindowsSecurityOptions` struct will contain Windows OS specific security attributes scoped either at the pod level (applicable to all containers in the pod) or at the individual container level (that can override the pod level specification). Initially, the `WindowsSecurityOptions` struct will have the following fields:
+In this KEP we propose a new field named `WindowsOptions` in the `PodSecurityContext` struct (associated with a pod spec) and the `SecurityContext` struct (associated with each container in a pod). `WindowsOptions` will be a pointer to a new struct of type `WindowsOptions`. Having the field name and struct type name being the same has [precedence] (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go). The `WindowsOptions` struct will contain Windows OS specific security attributes scoped either at the pod level (applicable to all containers in the pod) or at the individual container level (that can override the pod level specification). This is inspired by the existing `SELinuxOptions` field that groups Linux specific SELinux options. Initially, the `WindowsOptions` struct will have the following fields:


suggest the type name be WindowsSecurityContextOptions, to indicate the context in which it is used

liggitt · 2019-04-24T13:46:37Z

keps/sig-windows/20190418-windows-security-context.md

+  securityContext:
+    windowsOptions:
+      gmsaCredentialSpecName: webapp1-credspec
+      runAsUserName: "NT AUTHORITY\NETWORK SERVICE"


nit for the example, would need \\ because it's in a yaml string (applies to all)

liggitt · 2019-04-24T17:17:24Z

fill in the ### Upgrade / Downgrade Strategy section with the timing of the feature gate promotion (xref https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#adding-unstable-features-to-stable-versions)

ddebroy · 2019-04-25T00:51:17Z

Addressed the comments so far from the meeting and on the PR (so far). Ready for another round of review @liggitt @PatrickLang

liggitt · 2019-04-26T05:33:04Z

keps/sig-windows/20190418-windows-security-context.md

+The fields within the `WindowsSecurityContextOptions` struct will be validated as follows:
+- GMSACredentialSpecName: This field will be the name of a custom resource. It should follow the rules associated with [naming of resources](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names) in Kubernetes. Only the length will be checked to make sure it is below 64 characters.
+- GMSACredentialSpec: The size of the CredentialSpec JSON blob should be limited to avoid abuse. We will limit it to 64K which allows for a lot of room to specify extremely complicated credential specs. Typically this JSON blob is not expected to be more than 1K based on experience so far.
+- RunAsUserName: This field needs to allow for a valid set of usernames allowed for Windows containers. Currently the OCI spec or Windows documentation does not specify any clear restrictions around the length of this parameter or restrictions around usage of special characters when passed to the container runtime and eventually to Windows HCS. So Kubernetes validation won't enforce any character set validation. A maximum length of 64 characters will be allowed to prevent abuse.


I thought we discussed this as 256 characters?

we don't want to make this stricter than what windows would accept, is there a clear limit specified there?

Agree. But I am not able to find a clear limit specified anywhere in MSDN or moby/moby today around the above. @PatrickLang any thoughts on ^ ?

liggitt · 2019-04-26T05:35:22Z

keps/sig-windows/20190418-windows-security-context.md

+
+The fields within the `WindowsSecurityContextOptions` struct will be validated as follows:
+- GMSACredentialSpecName: This field will be the name of a custom resource. It should follow the rules associated with [naming of resources](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names) in Kubernetes. Only the length will be checked to make sure it is below 64 characters.
+- GMSACredentialSpec: The size of the CredentialSpec JSON blob should be limited to avoid abuse. We will limit it to 64K which allows for a lot of room to specify extremely complicated credential specs. Typically this JSON blob is not expected to be more than 1K based on experience so far.


it amuses me to no end that we literally have our own "64k ought to be enough for anyone" :)

liggitt · 2019-04-26T05:48:49Z

keps/sig-windows/20190418-windows-security-context.md

+#### Validation of fields
+
+The fields within the `WindowsSecurityContextOptions` struct will be validated as follows:
+- GMSACredentialSpecName: This field will be the name of a custom resource. It should follow the rules associated with [naming of resources](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names) in Kubernetes. Only the length will be checked to make sure it is below 64 characters.


clarify this will validate the string is a GMSACredentialSpec name (length and format)

liggitt · 2019-04-26T05:49:05Z

a couple comments about the validation, lgtm otherwise

As outlined in the KEP at https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190418-windows-security-context.md and improvements on it at kubernetes/enhancements#975 For now this struct is left empty, as discussed in the KEP (see above) and as previously discussed with Jordan Liggitt. It will allow adding GMSA and options as well as `RunAsUserName` options; both of which have already been pre-implemented respectively at kubernetes#75459 and kubernetes#73609; and both of which will need to be re-based to make use of the new struct. Signed-off-by: Jean Rouge <rougej+github@gmail.com>

ddebroy · 2019-04-29T17:29:39Z

Clarified some of the validation as requested.

liggitt · 2019-04-29T17:43:58Z

lgtm, can squash once other reviewers are happy

PatrickLang · 2019-04-29T21:46:49Z

keps/sig-windows/20190418-windows-security-context.md


-Overall test plans for GMSA are covered in the GMSA KEP.
+An e2e test case with `[sig-windows]` label will be added to the Windows test runs to exercise and verify RunAsUserName is correctly taking effect. The test will try to bring up a pod with a container based on NanoServer, default user `ContainerUser` and the entrypoint trying to execute a privileged operation like changing the routing table inside the container. The pod spec will specify `ContainerAdministrator` as the RunAsUserName to verify the privileged operation within the container can successfully execute. Without RunAsUserName working correctly, the default user in the container will fail to execute the privileged operation and the container will exit prematurely.
+


^^ :) Glad we don't need a custom image. I forgot nanoserver already had both users defined.

…ptions Signed-off-by: Deep Debroy <ddebroy@docker.com>

liggitt · 2019-04-30T02:42:43Z

/lgtm

PatrickLang · 2019-04-30T16:17:24Z

/lgtm

k8s-ci-robot · 2019-04-30T16:17:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, liggitt, PatrickLang

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:

~~keps/sig-windows/OWNERS~~ [PatrickLang]

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

As outlined in the KEP at https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190418-windows-security-context.md and improvements on it at kubernetes/enhancements#975 For now this struct is left empty, as discussed in the KEP (see above) and as previously discussed with Jordan Liggitt. It will allow adding GMSA and options as well as `RunAsUserName` options; both of which have already been pre-implemented respectively at kubernetes/kubernetes#75459 and kubernetes/kubernetes#73609; and both of which will need to be re-based to make use of the new struct. Signed-off-by: Jean Rouge <rougej+github@gmail.com> Kubernetes-commit: d7aa31858e1734861131d2e8d67f94c766f9b577

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 19, 2019

k8s-ci-robot requested review from PatrickLang and michmike April 19, 2019 00:33

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/windows Categorizes an issue or PR as relevant to SIG Windows. labels Apr 19, 2019

ddebroy changed the title ~~Add test case and sample pod specs~~ Add test case and sample pod specs to KEP for Windows Security Options Apr 19, 2019

k8s-ci-robot assigned liggitt, michmike and PatrickLang Apr 19, 2019

ddebroy mentioned this pull request Apr 19, 2019

KEP for Windows security context #972

Merged

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 22, 2019

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 22, 2019

liggitt reviewed Apr 24, 2019

View reviewed changes

ddebroy changed the title ~~Add test case and sample pod specs to KEP for Windows Security Options~~ Updates to KEP for Windows Security Options Apr 25, 2019

liggitt reviewed Apr 26, 2019

View reviewed changes

wk8 mentioned this pull request Apr 26, 2019

Adding a new WindowsSecurityOptions struct kubernetes/kubernetes#77147

Merged

PatrickLang reviewed Apr 29, 2019

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2019

Clarify field validation logic and version skew handling for WindowsO…

606997b

…ptions Signed-off-by: Deep Debroy <ddebroy@docker.com>

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2019

k8s-ci-robot merged commit 1ac44a2 into kubernetes:master Apr 30, 2019


		Overall test plans for GMSA are covered in the GMSA KEP.
		An e2e test case with `[sig-windows]` label will be added to the Windows test runs to exercise and verify RunAsUserName is correctly taking effect. The test will try to bring up a pod with a container based on NanoServer, default user `ContainerUser` and the entrypoint trying to execute a privileged operation like changing the routing table inside the container. The pod spec will specify `ContainerAdministrator` as the RunAsUserName to verify the privileged operation within the container can successfully execute. Without RunAsUserName working correctly, the default user in the container will fail to execute the privileged operation and the container will exit prematurely.

Conversation

ddebroy commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddebroy commented Apr 19, 2019

Uh oh!

ddebroy commented Apr 22, 2019

Uh oh!

fejta-bot commented Apr 22, 2019

Uh oh!

liggitt Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

liggitt Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Apr 24, 2019

Uh oh!

ddebroy commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

liggitt Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddebroy Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

liggitt Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

liggitt Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

liggitt commented Apr 26, 2019

Uh oh!

ddebroy commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt commented Apr 29, 2019

Uh oh!

PatrickLang Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

liggitt commented Apr 30, 2019

Uh oh!

PatrickLang commented Apr 30, 2019

Uh oh!

k8s-ci-robot commented Apr 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ddebroy commented Apr 19, 2019 •

edited

Loading

liggitt Apr 24, 2019 •

edited

Loading

ddebroy commented Apr 25, 2019 •

edited

Loading

liggitt Apr 26, 2019 •

edited

Loading

ddebroy commented Apr 29, 2019 •

edited

Loading