-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
Rewrite PodSecurityPolicy guide #6378
Conversation
@tallclair: GitHub didn't allow me to request PR reviews from the following users: php-coder. Note that only kubernetes members can review this PR, and authors cannot review their own PRs. In response to this:
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. |
Deploy preview ready! Built with commit 5d39cfe https://deploy-preview-6378--kubernetes-io-master-staging.netlify.com |
+100 |
|
||
### FSGroup | ||
Once a PodSecurityPolicy resource has been created, it does nothing. In order to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
”Once“ -> ”Initially when" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with when
First, a `Role` or `ClusterRole` needs to grant access to `use` the desired | ||
policies. The rules to grant access look like this: | ||
|
||
```yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better make this snippet a complete manifest for users, i.e. add apiVersion, kind, metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
used. | ||
2. Otherwise, the first valid policy in alphabetical order is used. | ||
|
||
## Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the example presented here is a full story by itself, I'd suggest separating it out from the "concepts" page by creating a "task".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the value in that. Personally I find it much more useful to have a example alongside the documentation, so that I have access to both when I click the first search result.
If you're worried about the length of this page, I could be convinced to separate out the reference section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly okay with simple examples e.g. a manifest showing users how things look like.
This "example" is a little bit long. It is actually an exercise for users. They will need to create a separate namespace.
This is just my own feelings. I understand people have different reading habits. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what other's think about this. Is your goal to keep tutorials & reference docs separate, or just to shrink this page? I personally like having a thorough example alongside the reference docs, but we should go for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea but I'd suggest to postpone it and merge this PR as-is.
|
||
```shell | ||
$ kubectl create namespace psp-example | ||
$ kubectl create serviceaccount -n psp-example fake-user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename 'fake-user' to 'fake-sa' or 'fake-account' for clarity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's a real service account, and a fake user :)
I think fake-user communicates the intent better, which is that we are using this account to act as an unprivileged (fake) user.
defaults to allowed. | ||
|
||
**DefaultAllowPrivilegeEscalation** - Sets the default for the | ||
`AllowPrivilegeEscalation` option. The default behavior is to allow privilege |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better clarify the AllowPrivilegeEscalation
here. Does it mean the field of PodSecurityPolicy or the field 'pod.spec.container[i].securityContext.allowPrivilegeEscalation' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's explained above the fields, since it applies to both fields. Would you prefer I give the full path instead of container option
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"sets the default for the AllowPrivilegeEscalation
option" <-- when reading this, I was a bit confused.
Let me try again. This field works only when both the following conditions are met. The admission controller mutates the Pod spec by injecting the value of this field as the allowPrivilegeEscalation
property for the container's security context.
- the
AllowPrivilegeEscalation
field of the PodSecurityPolicy is unspecified - the container's security context has left
allowPrivilegeEscalation
unspecified
Sorry if I failed making it clear again... taxes to pay for a non-native speaker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to clarify it. Better?
the AllowPrivilegeEscalation field of the PodSecurityPolicy is unspecified
If it's set to true
, this field can still be used to default it to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, comments addressed.
|
||
### FSGroup | ||
Once a PodSecurityPolicy resource has been created, it does nothing. In order to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with when
First, a `Role` or `ClusterRole` needs to grant access to `use` the desired | ||
policies. The rules to grant access look like this: | ||
|
||
```yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
used. | ||
2. Otherwise, the first valid policy in alphabetical order is used. | ||
|
||
## Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the value in that. Personally I find it much more useful to have a example alongside the documentation, so that I have access to both when I click the first search result.
If you're worried about the length of this page, I could be convinced to separate out the reference section.
|
||
```shell | ||
$ kubectl create namespace psp-example | ||
$ kubectl create serviceaccount -n psp-example fake-user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's a real service account, and a fake user :)
I think fake-user communicates the intent better, which is that we are using this account to act as an unprivileged (fake) user.
defaults to allowed. | ||
|
||
**DefaultAllowPrivilegeEscalation** - Sets the default for the | ||
`AllowPrivilegeEscalation` option. The default behavior is to allow privilege |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's explained above the fields, since it applies to both fields. Would you prefer I give the full path instead of container option
?
$ kubectl-admin delete ns psp-example | ||
namespace "psp-example" deleted | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be concise, I was suggesting moving line 145-321 into a task. The example by itself has 180 lines in total.
defaults to allowed. | ||
|
||
**DefaultAllowPrivilegeEscalation** - Sets the default for the | ||
`AllowPrivilegeEscalation` option. The default behavior is to allow privilege |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"sets the default for the AllowPrivilegeEscalation
option" <-- when reading this, I was a bit confused.
Let me try again. This field works only when both the following conditions are met. The admission controller mutates the Pod spec by injecting the value of this field as the allowPrivilegeEscalation
property for the container's security context.
- the
AllowPrivilegeEscalation
field of the PodSecurityPolicy is unspecified - the container's security context has left
allowPrivilegeEscalation
unspecified
Sorry if I failed making it clear again... taxes to pay for a non-native speaker.
@@ -11,10 +13,5 @@ spec: | |||
rule: RunAsAny | |||
fsGroup: | |||
rule: RunAsAny | |||
hostPorts: | |||
- min: 8000 | |||
max: 8080 | |||
volumes: | |||
- '*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That make it still permissive (as it allows hostPath
volume type). Consider limiting the number of allowed types to fixed number (AFAIR we had a recommended list somewhere..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to keep this policy as minimal as possible, just to demonstrate that it is being applied in the example. The restricted policy (restricted-psp.yaml) is the one that actually applies the recommended best practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"unprivileged" still makes it sound like it will be restrictive... not sure a better name for what this does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with just "example".
- min: 1 | ||
max: 65535 | ||
fsGroup: | ||
rule: 'RunAsAny' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we disallow adding the root group in supplemental groups and permit this group here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, fixed locally & forgot to push. Done.
BTW - I filed kubernetes/kubernetes#56173 for this case. Unfortunately these rules as written default to applying the 1
GID to every container.
As you have renamed Lines 237 to 239 in a0d1143
P.S. This should fix build on TravisCI:
|
The last time when I tried to document them, we had no strong opinion about whether we should document annotations or we shouldn't. I'd be glad to hear that our vision has been changed. If we're going to document them, I'd suggest to also include |
| Allocating an FSGroup that owns the pod's volumes | [`fsGroup`](#volumes-and-file-systems) | | ||
| Requiring the use of a read only root file system | [`readOnlyRootFilesystem`](#volumes-and-file-systems) | | ||
| The user and group IDs of the container | [`runAsUser`, `supplementalGroups`](#users-and-groups) | | ||
| Restricting root privileges | [`allowPrivilegeEscalation`, `defaultAllowPrivilegeEscalation`](#privilege-escalation) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, it's better to rephrase "Restricting root privileges" to emphasize the fact that it not about root but about getting root privs.
What's about "Restricting obtaining of root privileges", "Restricting getting the root privileges"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
`seLinuxOptions`. | ||
- *RunAsAny* - No default provided. Allows any `seLinuxOptions` to be | ||
specified. | ||
Pod security policy control is implemented as an optional [admission |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised that admission plugin is optional. How the control will work without it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand your question. If the plugin is not enabled, then PodSecurityPolicy won't be enforced, but the cluster will still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it means, that there will be no control and all pods will be accepted.
I'd rather remove "optional" word from here because in reality nothing will work without admission plugin as we expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the phrasing is more that it is optional but highly encouraged for production environments. You don't have to run it, you just have to accept the risk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not enabled by default, it's in beta, and until recently wasn't tested in any e2e environments. I don't think we can claim it's not optional, but I added a note that it's recommended.
|
||
{% include code.html language="yaml" file="example-psp.yaml" ghlink="/docs/concepts/policy/example-psp.yaml" %} | ||
|
||
And create it with the standard `kubectl` command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/the standard kubectl
command/our kubectl
wrapper/ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just 'create it with kubectl:`
|
||
Now, as the unprivileged user, try to create a simple pod: | ||
|
||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add ```shell here to highlight the fragment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
1m 2m 15 pause-7774d79b5 ReplicaSet Warning FailedCreate replicaset-controller Error creating: pods "pause-7774d79b5-" is forbidden: no providers available to validate pod request | ||
``` | ||
|
||
**What happened?** We already bound the psp:unprivileged role for our fake-user, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we need to stand out psp:unprivileged
and fake-user
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "stand out". Did you mean add the inline-code backticks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I did.
### Host namespaces | ||
|
||
**HostPID** - Controls whether the pod containers can share the host process ID | ||
namespace. Note that when paired with ptrace this can be used to escalate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about ptrace!
But in order to use ptrace
we need to have CAP_SYS_PTRACE
capability, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Clarified that it's not enabled by default.
traditionally associated with the superuser. Some of these capabilities can be | ||
used to escalate privileges or for container breakout, and may be restricted by | ||
the PodSecurityPolicy. For more details on Linux capabilities, see | ||
[Capabilities(7)](http://man7.org/linux/man-pages/man7/capabilities.7.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Capabilities(7)/capabilities(7)/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
This command will open a default text editor where you will be able to modify policy. | ||
- *MustRunAs* - Requires `seLinuxOptions` to be configured if not using | ||
pre-allocated values. Uses `seLinuxOptions` as the default. Validates against |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK there is no "pre-allocated values" and this sentence is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not familiar with SELinux support at all. This was copied from the previous version of this page. Can you suggest a replacement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**AllowedCapabilities** - Provides a whitelist of capabilities that may be added | ||
to a container. The default set of capabilities are implicitly allowed. The | ||
empty set means that no additional capabilities may be added beyond the default | ||
set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we also could mention about *
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
defines a host path to match. The value cannot be "`*`" though. | ||
An example is shown below: | ||
**AllowedHostPaths** - This specifies a whitelist of host paths that are allowed | ||
to be used by HostPath volumes. An empty list means there is no restriction on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider replacing HostPath
by hostPath
in this paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@@ -125,141 +399,153 @@ to the volume sources that are defined when creating a volume: | |||
The recommended minimum set of allowed volumes for new PSPs are | |||
configMap, downwardAPI, emptyDir, persistentVolumeClaim, secret, and projected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd highlight the recommended volume types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Define the example PodSecurityPolicy object in a file. This is a policy that | ||
simply prevents the creation of privileged pods. | ||
|
||
{% include code.html language="yaml" file="example-psp.yaml" ghlink="/docs/concepts/policy/example-psp.yaml" %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
things I didn't know we could do
|
||
- *MustRunAs* - Requires at least one range to be specified. Uses the | ||
minimum value of the first range as the default. Validates against the | ||
first ID in the first range. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought validation required the groups to fall within one of the specified ranges. Where are you seeing validation use the first id in the first range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from the old docs, but I think it was a typo. Changed so MustRunAs
docs are consistent.
of the range as the default. Validates against the configured range. | ||
- *MustRunAsNonRoot* - Requires that the pod be submitted with a non-zero | ||
`runAsUser` or have the `USER` directive defined (using a numeric UID) in the | ||
image. No default provided. Setting `AllowPrivilegeEscalation=false` is strongly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allowPrivilegeEscalation to match API json field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
### Privilege Escalation | ||
|
||
These options control the `AllowPrivilegeEscalation` container option. This bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allowPrivilegeEscalation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, misunderstood... thought this was the pod API field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
against [the secured API port](/docs/admin/accessing-the-api/), and must not | ||
have superuser permissions. Otherwise requests would bypass authentication and | ||
authorization modules, all PodSecurityPolicy objects would be allowed, | ||
and user will be able to create privileged containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/user will/users would/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
- The [Controller Manager](/docs/admin/kube-controller-manager/) must be run | ||
against [the secured API port](/docs/admin/accessing-the-api/), and must not | ||
have superuser permissions. Otherwise requests would bypass authentication and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can mention --use-service-account-credentials
in combination with RBAC as a good way of making the controller loops run with bounded permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
above, plus `*` to allow all profiles. Absence of this annotation means that the | ||
default cannot be changed. | ||
|
||
## Troubleshooting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably hoist this up into the authorizing policies section... reading all the way to the bottom to see the "here's how to make sure you're not accidentally insecure" isn't great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
# This is redundant with non-root + disallow privilege escalation, | ||
# but we can provide it for defense in depth. | ||
requiredDropCapabilities: | ||
- ALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is dropping all capabilities best practice? does that cause problems for most images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment above. Running as non-root drops all capabilities from the effective set (I don't think there's a way around this?) and no_new_privs disables file capabilities, so there is no way to access the bounding set. This is what I meant about it being redundant, but the capabilities might as well be dropped (e.g. if a kernel bug lead to a root privilege escalation, maybe it would still have 0 capabilities).
``` | ||
|
||
## Admission | ||
**What happened?** Although the PodSecurityPolicy was created, neither the | ||
created pod nor `fake-user` have permission to use the new policy: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a little confusing since pods do not get permission to use the policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clarified
|
||
**AllowPrivilegeEscalation** - Gates whether or not a user is allowed to set the | ||
security context of a container to `allowPrivilegeEscalation=true`. This | ||
defaults to allowed. When set to false, the container's `allPrivilegeEscalation` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allPrivilegeEscalation
-> allowPrivilegeEscalation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments addressed. Thanks!
|
||
- *MustRunAs* - Requires at least one range to be specified. Uses the | ||
minimum value of the first range as the default. Validates against the | ||
first ID in the first range. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from the old docs, but I think it was a typo. Changed so MustRunAs
docs are consistent.
@@ -11,10 +13,5 @@ spec: | |||
rule: RunAsAny | |||
fsGroup: | |||
rule: RunAsAny | |||
hostPorts: | |||
- min: 8000 | |||
max: 8080 | |||
volumes: | |||
- '*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with just "example".
minimum value of the first range as the default. Validates against the | ||
first ID in the first range. | ||
- *RunAsAny* - No default provided. Allows any `fsGroup` ID to be specified. | ||
Most Kubernetes pods are not created directly by user. Instead, they are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
[ReplicaSet](/docs/concepts/workloads/controllers/replicaset/), or other | ||
templated controller via the controller manager. Granting the controller access | ||
to the policy would grant access for *all* pods created by that the controller, | ||
so the preferable method for authorizing policies is to grant access to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
In addition to restricting pod creation and update, pod security policies can | ||
also be used to provide default values for many of the fields that it | ||
controls. However, this presents a problem when multiple policies are available |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
above, plus `*` to allow all profiles. Absence of this annotation means that the | ||
default cannot be changed. | ||
|
||
## Troubleshooting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
- The [Controller Manager](/docs/admin/kube-controller-manager/) must be run | ||
against [the secured API port](/docs/admin/accessing-the-api/), and must not | ||
have superuser permissions. Otherwise requests would bypass authentication and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
against [the secured API port](/docs/admin/accessing-the-api/), and must not | ||
have superuser permissions. Otherwise requests would bypass authentication and | ||
authorization modules, all PodSecurityPolicy objects would be allowed, | ||
and user will be able to create privileged containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
# This is redundant with non-root + disallow privilege escalation, | ||
# but we can provide it for defense in depth. | ||
requiredDropCapabilities: | ||
- ALL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment above. Running as non-root drops all capabilities from the effective set (I don't think there's a way around this?) and no_new_privs disables file capabilities, so there is no way to access the bounding set. This is what I meant about it being redundant, but the capabilities might as well be dropped (e.g. if a kernel bug lead to a root privilege escalation, maybe it would still have 0 capabilities).
`seLinuxOptions`. | ||
- *RunAsAny* - No default provided. Allows any `seLinuxOptions` to be | ||
specified. | ||
Pod security policy control is implemented as an optional [admission |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not enabled by default, it's in beta, and until recently wasn't tested in any e2e environments. I don't think we can claim it's not optional, but I added a note that it's recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Review status: 0 of 5 files reviewed at latest revision, 37 unresolved discussions, some commit checks failed. docs/concepts/policy/pod-security-policy.md, line 154 at r4 (raw file):
Comments from Reviewable |
Reviewed 2 of 5 files at r1, 1 of 2 files at r3, 3 of 3 files at r4. Comments from Reviewable |
Review status: all files reviewed at latest revision, 37 unresolved discussions, some commit checks failed. docs/concepts/policy/pod-security-policy.md, line 154 at r4 (raw file): Previously, aasmall (Aaron Small) wrote…
This fix was explicitly made to make it deterministic (it used to actually be non-deterministc, which broke things). I would say that at this point the alpbabetical ordering is a part of the API, and won't change (unless the API version changes too). We may add explicit priorities in the future that would override alphabetic, but I think alphabetic would still be the tie-breaker. Comments from Reviewable |
Review status: all files reviewed at latest revision, 37 unresolved discussions, some commit checks failed. docs/concepts/policy/pod-security-policy.md, line 154 at r4 (raw file): Previously, tallclair (Tim Allclair (St. Clair)) wrote…
OK Comments from Reviewable |
- name: pause | ||
image: gcr.io/google-containers/pause | ||
EOF | ||
Error from server (Forbidden): error when creating "STDIN": pods "pause" is forbidden: no providers available to validate pod request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW the error message has been changed:
s/no providers available to validate pod request/unable to validate against any pod security policy: []/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Squashed & rebased. |
|
||
PodSecurityPolicy authorization uses the union of all policies available to the | ||
user creating the pod and | ||
[the service account specified on the pod](/docs/tasks/configure-pod-container/configure-service-account/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this information is very useful, and the subtlety of how this works was lost in this rewrite. Can we add it back in somehow?
For pods created on behalf of a user, in most cases by Controller Manager, | ||
access should be given to the service account specified on the pod spec | ||
template. Examples of resources that create pods on behalf of a user are | ||
Deployments, ReplicaSets, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this information is very useful, and the subtlety of how this works was lost in this rewrite. Can we add it back in somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tammersaleh I think this is covered by the second paragraph under authorizing policies. Do you disagree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tallclair Reading through that again, I want to say it's sufficient. But I'd read that document a few times as I was figuring this stuff out, and still somehow missed it. I'm happy to chalk this up to just inattention on my part.
This is an almost complete rewrite. Summary of changes:
/cc @liggitt @php-coder
This change is