Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update pod security standards to use PodOS field #35772

Merged

Conversation

ravisantoshgudimetla
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 7, 2022
@netlify
Copy link

netlify bot commented Aug 7, 2022

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit a1f6615
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/62fe97647e40e8000854980e

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language labels Aug 7, 2022
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Aug 7, 2022
@ravisantoshgudimetla
Copy link
Contributor Author

/cc @tallclair @liggitt

/sig auth
/sig windows

@k8s-ci-robot k8s-ci-robot requested a review from liggitt August 7, 2022 23:00
@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/windows Categorizes an issue or PR as relevant to SIG Windows. labels Aug 7, 2022
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

We try to write documentation to be timeless - see https://developers.google.com/style/timeless-documentation for an example.

Here are some suggestions with that in mind.

Comment on lines 409 to 410
Containers must drop <code>ALL</code> capabilities, and are only permitted to add back
the <code>NET_BIND_SERVICE</code> capability.
the <code>NET_BIND_SERVICE</code> capability. Starting v1.25, windows pods would be exempted from this control if pod.spec.os.Name is set to <code>windows</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Containers must drop <code>ALL</code> capabilities, and are only permitted to add back
the <code>NET_BIND_SERVICE</code> capability.
the <code>NET_BIND_SERVICE</code> capability. Starting v1.25, windows pods would be exempted from this control if pod.spec.os.Name is set to <code>windows</code>
For Linux pods, require that containers drop <em>all</em> capabilities:
<code>ALL</code>.
As a special case, containers are permitted to add back the
<code>NET_BIND_SERVICE</code> capability (and only this capability).
This control only applies when you set `spec.os` for the overall Pod to
<code>linux</code>, or if you do not specify a
Pod OS. Versions of Kubernetes before v1.25 enforce this control regardless of
operating system.

profile. However, the lack of enforcement means that there is no additional restriction, for Pods
that use Windows containers, compared to the baseline profile.

In v1.25, the `pod.spec.os` field has graduated to stable which helps identify the windows pods authoritatively. Pod Security admission plugin has been updated to use this field which allows certain relaxations for the windows pods. The restricted profile has been updated to ensure that the Linux-specific restrictions such as seccomp profile, disallowing privilege escalation, capabilities are ignored if the `pod.spec.os.Name` is set to `windows`.
Copy link
Contributor

@sftim sftim Aug 8, 2022

Choose a reason for hiding this comment

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

Suggested change
In v1.25, the `pod.spec.os` field has graduated to stable which helps identify the windows pods authoritatively. Pod Security admission plugin has been updated to use this field which allows certain relaxations for the windows pods. The restricted profile has been updated to ensure that the Linux-specific restrictions such as seccomp profile, disallowing privilege escalation, capabilities are ignored if the `pod.spec.os.Name` is set to `windows`.
For Kubernetes v{{< skew currentVersion >}} it is good practice to specify
the `.spec.os` field for every Pod. The `.spec.os` field is available in Kubernetes
v1.23 and later but was behind a
[feature gate](https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/)
for Kubernetes v1.23 and v1.24.
If you set the Pod OS field to `linux`, only Linux nodes try to run the Pod, and the enforced security
controls are appropriate to Linux.
Setting the Pod OS field to `windows` means that only Windows nodes would try to run the Pod, and
you don't need to apply Linux-specific restrictions. For example, a Windows Pod does not
need to explicitly drop all capabilities for each container, even if you are applying the `restricted`
Pod security standard.
If you don't specify the Pod OS, Kubernetes takes a precautionary approach and applies all the Pod
security measures that could be relevant.

I would move this section out of the FAQ and move it to a 2nd-level heading just above the FAQ.
The section heading I'd use would be:

## Security standards for different operating systems

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 am fine with @tallclair any concerns?

Copy link
Member

Choose a reason for hiding this comment

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

I agree. I was going to suggest the same thing.

Copy link
Contributor

@chrisnegus chrisnegus left a comment

Choose a reason for hiding this comment

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

I just have one capitalization suggestion that occurs several time.

@@ -407,7 +407,7 @@ fail validation.
<td>
<p>
Containers must drop <code>ALL</code> capabilities, and are only permitted to add back
the <code>NET_BIND_SERVICE</code> capability.
the <code>NET_BIND_SERVICE</code> capability. Starting v1.25, windows pods would be exempted from this control if pod.spec.os.Name is set to <code>windows</code>
Copy link
Contributor

Choose a reason for hiding this comment

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

Other k8s docs capitalize the "W" in "Windows pods". I suggest changing the lowercase "windows pods" instances in this document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will change it.

@reylejano
Copy link
Member

/milestone 1.25
/assign @cathchu
/cc @kcmartin

@k8s-ci-robot k8s-ci-robot added this to the 1.25 milestone Aug 9, 2022
@k8s-ci-robot k8s-ci-robot requested a review from kcmartin August 9, 2022 14:39
@tallclair
Copy link
Member

/assign

@@ -326,7 +326,7 @@ fail validation.
<tr>
<td style="white-space: nowrap">Privilege Escalation (v1.8+)</td>
<td>
<p>Privilege escalation (such as via set-user-ID or set-group-ID file mode) should not be allowed.</p>
<p>On Linux, do not allow privilege escalation (such as via set-user-ID or set-group-ID file mode). This control only applies when you set `spec.os` to <code>linux</code>, or if you do not specify a Pod OS. Versions of Kubernetes before v1.25 enforce this control regardless of operating system.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Rather than integrating this text in here, I'd suggest just having a standard header that you can add to all the linux-only fields. I prefer this approach because it makes it easier to identify the linux-only fields with a visual scan.

Maybe something like this:

<p><em><a href="#what-profiles-should-i-apply-to-my-windows-pods">Linux only</a> in v1.25+ (`spec.os != windows`)</em></p>

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit odd to hyperlink “Linux only” to #what-profiles-should-i-apply-to-my-windows-pods - maybe we could have a new page section that covers OS specifics, with a subheading for Linux and another subheading for Windows.

Copy link
Contributor Author

@ravisantoshgudimetla ravisantoshgudimetla Aug 15, 2022

Choose a reason for hiding this comment

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

The content for Windows is pretty small with only 3 policies changes in restricted profile for now. May be we can create a new page section when the content increases for Windows? How about just a note?

@@ -381,7 +381,7 @@ fail validation.
<tr>
<td style="white-space: nowrap">Seccomp (v1.19+)</td>
<td>
<p>Seccomp profile must be explicitly set to one of the allowed values. Both the <code>Unconfined</code> profile and the <em>absence</em> of a profile are prohibited.</p>
<p>On Linux, require that the seccomp profile is explicitly set to one of the allowed values. Both the <code>Unconfined</code> profile and the <em>absence</em> of a profile are prohibited. This control only applies when you set `spec.os` to <code>linux</code>, or if you do not specify a Pod OS. Versions of Kubernetes before v1.25 enforce this control regardless of operating system.</p>
Copy link
Member

Choose a reason for hiding this comment

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

... when you set spec.os to linux ...

It's actually coded the other way - skip the check when it's set to windows. A subtle difference, but it could matter if we added another OS.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if I should also file a feature request issue to invert the logic there?

Copy link
Member

Choose a reason for hiding this comment

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

exempting only for an explicitly windows pod is intentional, so we default in the safe direction for new hitherto unknown OSes

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 thought about it but did not want to include negations(skips) in the docs for skipping. However I agree since there is a chance of expanding OS'es later it is better to talk about skips.

@@ -381,7 +381,7 @@ fail validation.
<tr>
<td style="white-space: nowrap">Seccomp (v1.19+)</td>
<td>
<p>Seccomp profile must be explicitly set to one of the allowed values. Both the <code>Unconfined</code> profile and the <em>absence</em> of a profile are prohibited.</p>
<p>On Linux, require that the seccomp profile is explicitly set to one of the allowed values. Both the <code>Unconfined</code> profile and the <em>absence</em> of a profile are prohibited. This control only applies when you set `spec.os` to <code>linux</code>, or if you do not specify a Pod OS. Versions of Kubernetes before v1.25 enforce this control regardless of operating system.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately markdown syntax isn't processed in this table, so you need to use raw HTML tags.

Suggested change
<p>On Linux, require that the seccomp profile is explicitly set to one of the allowed values. Both the <code>Unconfined</code> profile and the <em>absence</em> of a profile are prohibited. This control only applies when you set `spec.os` to <code>linux</code>, or if you do not specify a Pod OS. Versions of Kubernetes before v1.25 enforce this control regardless of operating system.</p>
<p>On Linux, require that the seccomp profile is explicitly set to one of the allowed values. Both the <code>Unconfined</code> profile and the <em>absence</em> of a profile are prohibited. This control only applies when you set <code>spec.os</code> to <code>linux</code>, or if you do not specify a Pod OS. Versions of Kubernetes before v1.25 enforce this control regardless of operating system.</p>

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is it actually .spec.os.name?

profile. However, the lack of enforcement means that there is no additional restriction, for Pods
that use Windows containers, compared to the baseline profile.

In v1.25, the `pod.spec.os` field has graduated to stable which helps identify the windows pods authoritatively. Pod Security admission plugin has been updated to use this field which allows certain relaxations for the windows pods. The restricted profile has been updated to ensure that the Linux-specific restrictions such as seccomp profile, disallowing privilege escalation, capabilities are ignored if the `pod.spec.os.Name` is set to `windows`.
Copy link
Member

Choose a reason for hiding this comment

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

I agree. I was going to suggest the same thing.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 15, 2022
@ravisantoshgudimetla ravisantoshgudimetla force-pushed the psp-restricted branch 4 times, most recently from 1071f44 to ac09f46 Compare August 15, 2022 15:15
@ravisantoshgudimetla
Copy link
Contributor Author

ravisantoshgudimetla commented Aug 15, 2022

I slightly tweaked your suggestions @sftim and @tallclair. PTAL

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

I do recommend some more changes here.

to Linux.

### Restricted Pod Security Standard changes
Another important change starting Kubernetes v{{ < skew currentVersion >}} is that the pod restricted standard
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Another important change starting Kubernetes v{{ < skew currentVersion >}} is that the pod restricted standard
Another important change, made in Kubernetes v1.25, is that the _restricted_ Pod security standard

If you can, rewrite the text to define and document the current (oost-v1.25) behaviour, rather than the state change.

Copy link
Member

Choose a reason for hiding this comment

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

I was actually thinking it might be worth having an explicit changelog on this page. This is a bit different from documenting Kubernetes changes, as the older profiles are supported indefinitely, across Kubernetes versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tallclair - You mean a link to the changelog on this page?

workloads. Specifically, many of the Pod SecurityContext fields
[have no effect on Windows](/docs/concepts/windows/intro/#compatibility-v1-pod-spec-containers-securitycontext).

### Pod API validation changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Pod API validation changes
### Pod security checks based on Pod operating system

Document the behavior, not the change in behavior (that's for the release notes)/

Copy link
Member

Choose a reason for hiding this comment

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

I think we should drop this section. Pod API validation is not part of the Pod Security Standards.

Copy link
Contributor Author

@ravisantoshgudimetla ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

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

Yup. Those changes look good to me. Thank you for making them @sftim

workloads. Specifically, many of the Pod SecurityContext fields
[have no effect on Windows](/docs/concepts/windows/intro/#compatibility-v1-pod-spec-containers-securitycontext).

### Pod API validation changes
Copy link
Member

Choose a reason for hiding this comment

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

I think we should drop this section. Pod API validation is not part of the Pod Security Standards.

to Linux.

### Restricted Pod Security Standard changes
Another important change starting Kubernetes v{{ < skew currentVersion >}} is that the pod restricted standard
Copy link
Member

Choose a reason for hiding this comment

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

I was actually thinking it might be worth having an explicit changelog on this page. This is a bit different from documenting Kubernetes changes, as the older profiles are supported indefinitely, across Kubernetes versions.

Windows in Kubernetes has some limitations and differentiators from standard Linux-based
workloads. Specifically, many of the Pod SecurityContext fields
[have no effect on Windows](/docs/concepts/windows/intro/#compatibility-v1-pod-spec-containers-securitycontext).

Copy link
Member

Choose a reason for hiding this comment

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

Add a warning here about how Kubelets prior to v1.24 don't enforce the pod OS field, and if a cluster has nodes on versions earlier than v1.24 the restricted policies should be pinned to a version prior to v1.25.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reylejano
Copy link
Member

Hi @ravisantoshgudimetla , are you able to address the comments and feedback
The 1.25 Docs complete deadline passed and this PR needs to address the comments and feedback as soon as possible

Copy link
Contributor Author

@ravisantoshgudimetla ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestions @sftim and @tallclair

@reylejano - I am on PTO till the end of this week with limited access to the internet so I'm making changes as soon as I can. If someone can pick this PR up to push it along, I am fine with it.

@sftim
Copy link
Contributor

sftim commented Aug 22, 2022

Should we:

  • merge this as-is
  • postpone the v1.25 release?

@ravisantoshgudimetla
Copy link
Contributor Author

Is this the only outstanding comment on which we need to get feedback?

@reylejano
Copy link
Member

let's merge as-is and I'll create a follow-up PR to address comments and suggestions
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2022
@sftim
Copy link
Contributor

sftim commented Aug 22, 2022

/lgtm

We should review this after the release, as we're merging updates to docs about a security control, and we don't yet have a tech review signoff from the right SIGs.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2022
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 805ba4d3f310232a934431e96bf864d447a65d9b

@k8s-ci-robot k8s-ci-robot merged commit 735d499 into kubernetes:dev-1.25 Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants