Enrich the PSP templates with template and field-level descriptions #109

julianKatz · 2021-09-14T16:37:01Z

No description provided.

maxsmythe

added some starting comments, but couldn't get through the whole thing yet

library/pod-security-policy/apparmor/template.yaml

library/pod-security-policy/capabilities/template.yaml

library/pod-security-policy/flexvolume-drivers/template.yaml

maxsmythe · 2021-09-14T22:22:46Z

library/pod-security-policy/fsgroup/template.yaml

@@ -3,7 +3,7 @@ kind: ConstraintTemplate
 metadata:
  name: k8spspfsgroup
  annotations:
-    description: Controls allocating an FSGroup that owns the Pod's volumes.
+    description: "Controls allocating an FSGroup that owns the Pod's volumes.  Corresponds to the `fsGroup` field in a PodSecurityPolicy."


I wish there could be an "extended discription", so we can have a blurb and then more detailed instructions.

Yea I was thinking about that. Particularly something that would allow for some nice markdown syntax.

Yeah, interesting to think how it would interact with shorter descriptions and examples...

File an issue for a feature request?

Where does this feature request go? Isn't this part of JSONSchema? Or are you talking about extended description for the whole resource? Couldn't we just add another annotation for that?

The second (another annotation), just an FR against the library.

Issue created: #112

library/pod-security-policy/fsgroup/template.yaml

maxsmythe · 2021-09-14T22:24:08Z

library/pod-security-policy/host-filesystem/template.yaml

              items:
                type: object
                properties:
                  readOnly:
                    type: boolean
+                    description: "when set to true, will allow host volumes matching the pathPrefix only if all volume mounts are readOnly."


all volume mounts or all matching volume mounts?

So, hilariously, I copied this from the k8s libraries themselves.

all volume mounts or all matching volume mounts?

So this would be all of the volume mounts that match that prefix. So, if you had another allowed prefix with readOnly: false the mounts matching that prefix would not be required to be readOnly.

Another version of that description could be:

When set to true, all mounts covered by the pathPrefix must be mounted readOnly.

Thoughts?

Ah, def. don't just use PSP words:

Because we should at least cite them if we do

Because the Rego may not be written exactly like the PSP Go code... particularly if the behavior isn't documented as part of the API

Looking at the template, here's what it seems like is happening:

It's possible to use paths on the host node as volumes that can be mounted to the pods (spec.volumes)

in order to do this, you must specify that the volume is a host path type, and which host path you want to use as a volume, this path is what pathPrefix matches against

A volume can be mounted to one or more paths in the pod

Each volume mount must reference a volume defined in spec.volumes

Each volume mount specifies where that volume gets mounted inside the pod (this value is not checked)

Each volume mount can be mounted as read only

It appears that the readOnly parameter allows users to say "for every volume mount that references this volume must mount it as read only"

how about:

when set to true, any container volumeMounts matching the pathPrefix must include `readOnly: true`

library/pod-security-policy/proc-mount/template.yaml

maxsmythe

finished the review, added some more comments.

maxsmythe · 2021-09-15T02:06:05Z

library/pod-security-policy/selinux/template.yaml

@@ -3,7 +3,7 @@ kind: ConstraintTemplate
 metadata:
  name: k8spspselinuxv2
  annotations:
-    description: Controls the SELinux context of the container.
+    description: "Defines an allow-list of seLinuxOptions configurations for pod containers.  Corresponds to a PodSecurityPolicy with SELinux rule `MustRunAs` enabled and specific `seLinuxOptions` specified."


can we make these longer descriptions multi-lined via the | operator (like the rego field below). This should help readability for people using vim (or GitHub code review)

WRT text...

"and specific seLinuxOptions specified."

"specific" isn't very descriptive. Do you mean the options as provided to the parameters of the CT?

Will change all of the templates to use the | operator. It's definitely cleaner and more readable.

r.e. "specific, the idea I'm going for is the following:

If you use this template, you configure at least one set of seLinuxOptions that is defined as a legal configuration of options for a container to run with. Were you to apply a container with a different set of Linux Options, that container would be rejected by g8r.

To achieve the same effect with a PSP, you'd have to create an seLinux entry with rule MustRunAs and the specific set of allowed SELinux options included under it. That would look like:

spec: seLinux: rule: RunAsAny seLinuxOptions: [values]

So I'm really just trying to say that you'd have to configure that field with some specific values of your choosing. So I'm not being particularly specific when I say specific. I'm just trying to say that you have to configure.

How could I say it better?

One thing I forgot about is newlines... I think > is the correct character if you want to have newlines for readability but not for content:

https://yaml.org/spec/1.2.1/#style/block/folded

re: specific... TBH I wouldn't mention it. It's enough to say that it is "requiring SELinux configs"

Referencing the PSP policy could be helpful for users already familiar with PSP, to help guide them when choosing replacement policies, but we shouldn't do it to the point where it confuses anyone unfamiliar with PSP and reading the docs for the first time.

Are you saying I should get rid of the "Corresponds to a PodSecurityPolicy" parts of all the descriptions? @mkorfgoogle told me he liked those.

Also, newest version:

Defines an allow-list of seLinuxOptions configurations for pod containers. Corresponds to a PodSecurityPolicy requiring SELinux configs.

library/pod-security-policy/selinux/template.yaml

library/pod-security-policy/users/template.yaml

maxsmythe · 2021-09-15T02:11:20Z

library/pod-security-policy/users/template.yaml

                    properties:
                      min:
                        type: integer
                      max:
                        type: integer
            supplementalGroups:
              type: object
+              description: "Controls which groups are applied to the first process run in any pod container, in addition to the container's primary GID."


Is there a reason we're calling out the "first process"?

I guess "main process" would be a better way to describe this.

I adapted this from the kubernetes core code.

My understanding is that other processes are forked off of that first process, so they can only have groups that it would have? Is that right?

Please reword anything inspired from others' work.

Also, I do like "main process" better. You can do escalation via methods like su, so it's not strictly true that it's an inheritance tree, but I do think that's the default behavior.

New version:

description: "Controls which additional groups are applied to a pod container's main process beyond its primary GID."

After reading other comments, I think it's better to go a simpler route, where we don't try to teach the feature in the CT. Rather, we should say specifically what policy the CT enforces and no more.

maxsmythe · 2021-09-15T02:12:42Z

library/pod-security-policy/users/template.yaml

                    properties:
                      min:
                        type: integer
                      max:
                        type: integer
            fsGroup:
              type: object
+              description: "Controls the supplemental group applied to some volumes."


"some volumes": which volumes?

Also adapted from k8s docs.

Based on this blogpost and these docs, it looks like it sets both the volume mounted to the contianer AND any files created within it by the Pod containers have this GID.

This is complicated, and I'm not sure how to think about ROI on figuring out how it works in detail. Is there something that's kind of vague but not wrong that we could say? It seems like the k8s docs punted as well

Please reword anything inspired from others' work.

Also, we should be documenting these things based of the Rego written, for accuracy.

Finally, I'm asking "which volumes", because as a user, that would be my first question when I read something like "some volumes.

I think you make a good point in another comment that we shouldn't try to teach people these features, but rather how to restrict them via the CT.

That being said, I think it makes sense to change the descriptions in this CT to things like:

fsGroup: type: object description: "Controls the fsGroup values allowed in a Pod or container level SecurityContext."

This is simpler and is actually what we validate in the rego.

Going to go ahead and change these, let me know if I should revert.

library/pod-security-policy/volumes/template.yaml

library/pod-security-policy/selinux/template.yaml

maxsmythe

LGTM with a few nits!

library/pod-security-policy/users/template.yaml

library/pod-security-policy/fsgroup/template.yaml

willbeason · 2021-09-24T17:42:05Z

library/pod-security-policy/allow-privilege-escalation/template.yaml

-    description: Controls restricting escalation to root privileges.
+    description: >-
+      Controls restricting escalation to root privileges. Corresponds to the
+      `allowPrivilegeEscalation` field in a PodSecurityPolicy.


What do you think about using fully-qualified paths in comments? My preference is to use the longer ones to make the structure clearer to the people consuming the templates.

For PSP specifically, the main purpose is to provide guidance for people who are referencing the PSP fields/ documentation.

The PSP documentation references its fields by non-fully-qualified name:

https://kubernetes.io/docs/concepts/policy/pod-security-policy/

So we should follow their conventions.

Since PSP is deprecated, we may choose to remove these references entirely once most/all users are no longer in a transitional phase.

If we are talking about related fields within the same constraint template, that seems reasonable to me, though I'd be a bit concerned about verbosity. Maybe make all paths relative to parameters?

I think @willbeason is talking about saying:

Corresponds to the `spec.allowPrivilegeEscalation` field in a PodSecurityPolicy

in the description.

Personally, I'm down either way. My first move when using a PSP Constraint Template would be to look at an example PSP, so the full paths wouldn't be a huge difference maker (they're basically all spec.[fieldname]). That said, adding them requires little effort so I'm happy to do so if you feel strongly @willbeason . Are you against this, @maxsmythe ?

I'm fine either way.

I lean more towards matching the PSP docs (meaning what's written is fine as-is)

library/pod-security-policy/apparmor/template.yaml

library/pod-security-policy/capabilities/template.yaml

library/pod-security-policy/forbidden-sysctls/template.yaml

library/pod-security-policy/fsgroup/template.yaml

library/pod-security-policy/host-filesystem/template.yaml

library/pod-security-policy/selinux/template.yaml

library/pod-security-policy/allow-privilege-escalation/template.yaml

library/pod-security-policy/proc-mount/template.yaml

library/pod-security-policy/read-only-root-filesystem/template.yaml

library/pod-security-policy/users/template.yaml

Signed-off-by: juliankatz <juliankatz@google.com>

…ary into psp-field-descriptions Signed-off-by: juliankatz <juliankatz@google.com>

library/pod-security-policy/allow-privilege-escalation/template.yaml

…ileged-containers Signed-off-by: juliankatz <juliankatz@google.com>

sozercan

LGTM

…pen-policy-agent#109) Signed-off-by: juliankatz <juliankatz@google.com> Signed-off-by: Mark van Holsteijn <mark.van.holsteijn@gmail.com>

julianKatz requested a review from maxsmythe September 14, 2021 16:37

julianKatz force-pushed the psp-field-descriptions branch from d7bdde4 to e6d2113 Compare September 14, 2021 17:30

maxsmythe reviewed Sep 14, 2021

View reviewed changes

maxsmythe requested changes Sep 15, 2021

View reviewed changes

julianKatz force-pushed the psp-field-descriptions branch 2 times, most recently from 0c8328b to 63a047c Compare September 16, 2021 22:48

maxsmythe reviewed Sep 17, 2021

View reviewed changes

library/pod-security-policy/selinux/template.yaml Outdated Show resolved Hide resolved

maxsmythe approved these changes Sep 23, 2021

View reviewed changes

library/pod-security-policy/users/template.yaml Outdated Show resolved Hide resolved

library/pod-security-policy/fsgroup/template.yaml Show resolved Hide resolved

julianKatz requested a review from willbeason September 24, 2021 17:30

willbeason reviewed Sep 24, 2021

View reviewed changes

mkorfgoogle reviewed Sep 24, 2021

View reviewed changes

Add description info for the PodSecurityPolicy ConstraintTemplates

8dad32d

Signed-off-by: juliankatz <juliankatz@google.com>

julianKatz force-pushed the psp-field-descriptions branch from dcdd096 to 8dad32d Compare September 28, 2021 02:22

Copy annotations description into validation section

f7e7099

Signed-off-by: juliankatz <juliankatz@google.com>

willbeason approved these changes Sep 28, 2021

View reviewed changes

julianKatz changed the title ~~First stab at field descriptions for the PSP templates~~ Enrich the PSP templates with template and field-level descriptions Sep 28, 2021

julianKatz requested review from ritazh, shomron and sozercan September 28, 2021 19:10

Merge branch 'master' of github.com:open-policy-agent/gatekeeper-libr…

3ec2808

…ary into psp-field-descriptions Signed-off-by: juliankatz <juliankatz@google.com>

ritazh reviewed Oct 4, 2021

View reviewed changes

library/pod-security-policy/allow-privilege-escalation/template.yaml Outdated Show resolved Hide resolved

Removed unnecessary comments form allow-privilege-escalation and priv…

1c9cde0

…ileged-containers Signed-off-by: juliankatz <juliankatz@google.com>

sozercan approved these changes Oct 5, 2021

View reviewed changes

ritazh approved these changes Oct 5, 2021

View reviewed changes

julianKatz merged commit 8be1fcf into open-policy-agent:master Oct 6, 2021

julianKatz deleted the psp-field-descriptions branch October 6, 2021 18:27

mac-chaffee mentioned this pull request Dec 8, 2021

spec.validation.openAPIV3Schema.properties[spec].properties[parameters].type: Required value: must not be empty for specified object fields #152

Closed

Enrich the PSP templates with template and field-level descriptions #109

Enrich the PSP templates with template and field-level descriptions #109

Conversation

julianKatz commented Sep 14, 2021

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianKatz Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianKatz Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianKatz Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianKatz Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan left a comment

Choose a reason for hiding this comment

julianKatz Sep 17, 2021 •

edited

Loading

julianKatz Sep 22, 2021 •

edited

Loading

julianKatz Sep 17, 2021 •

edited

Loading

julianKatz Sep 28, 2021 •

edited

Loading