✨ Apply PodSecurityStandard in Secure Cluster Class #6390

chrischdi · 2022-04-07T12:54:28Z

What this PR does / why we need it:

Adds a baseline pod security standard which gets enforced at cluster level.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Resolves parts of #6329

Open for discussion for me:

Wether or not add it to the default ClusterClass or add a separate ClusterClass e.g. called capi-quickstart-secure.
A second ClusterClass would have the con of duplicating the normal quick-start ClusterClass with lots of duplicated content.

If adding the PSS to the default cluster class results into issues we could also work the other way around and add a "insecure" ClusterClass named capi-quickstart-secure fur users to fallback to (clusterctl generate cluster capi-quickstart --flavor development-topology-insecure ...).

TODO:

squash commits

linux-foundation-easycla · 2022-04-07T12:54:36Z

The committers listed above are authorized under a signed CLA.

✅ login: chrischdi / name: Christian Schlotter (7c704c5)

killianmuldoon · 2022-04-07T16:47:42Z

Awesome - this is working for me:

The Pod "busybox" is invalid: spec: Forbidden: pod updates may not change fields other than `spec.containers[*].image`, `spec.initContainers[*].image`, `spec.activeDeadlineSeconds` or `spec.tolerations` (only additions to existing tolerations)
  core.PodSpec{
        Volumes:        {{Name: "kube-api-access-p9jbd", VolumeSource: {Projected: &{Sources: {{ServiceAccountToken: &{ExpirationSeconds: 3607, Path: "token"}}, {ConfigMap: &{LocalObjectReference: {Name: "kube-root-ca.crt"}, Items: {{Key: "ca.crt", Path: "ca.crt"}}}}, {DownwardAPI: &{Items: {{Path: "namespace", FieldRef: &{APIVersion: "v1", FieldPath: "metadata.namespace"}}}}}}, DefaultMode: &420}}}},
        InitContainers: nil,
        Containers: []core.Container{
                {
                        ... // 16 identical fields
                        TerminationMessagePolicy: "File",
                        ImagePullPolicy:          "IfNotPresent",
-                       SecurityContext:          &core.SecurityContext{Privileged: &true},
+                       SecurityContext:          nil,
                        Stdin:                    false,
                        StdinOnce:                false,
                        TTY:                      false,
                },
        },
        EphemeralContainers: nil,
        RestartPolicy:       "Always",
        ... // 5 identical fields
        AutomountServiceAccountToken: nil,
        NodeName:                     "cloister-linux-workers-ppp8v-775fffb45b-kdp7k",
        SecurityContext: &core.PodSecurityContext{
-               HostNetwork: true,
+               HostNetwork: false,
                HostPID:     false,
                HostIPC:     false,
                ... // 11 identical fields
        },
        ImagePullSecrets: nil,
        Hostname:         "",
        ... // 15 identical fields
  }

We should run the full e2e suite on this before merging to make sure it doesn't break anything (as this modifies the ClusterClass that underpins all of our tests). I don't think there should be an issue, but just in case.

killianmuldoon · 2022-04-07T16:49:20Z

/test pull-cluster-api-e2e-full-main

PushkarJ · 2022-04-07T20:59:13Z

@chrischdi Thank you so much for getting around to opening this PR so quickly!!!!

PushkarJ · 2022-04-07T21:09:41Z

If adding the PSS to the default cluster class results into issues

What would be the definition of "no issues detected by this change?". If it is only about e2e test passing then looks like we have achieved that already!! 🎉

we could also work the other way around and add a "insecure" ClusterClass named capi-quickstart-insecure for users to fallback to (clusterctl generate cluster capi-quickstart --flavor development-topology-insecure ...)

Yeah that makes sense to me because it meets second and third guideline on secure defaults in cloud native apps mentioned here: https://github.com/cncf/tag-security/blob/main/security-whitepaper/secure-defaults-cloud-native-8.md

Next step for me sounds like figuring out how we can enable this across all CAPI cloud providers.

killianmuldoon · 2022-04-08T08:15:32Z

I think we should be happy with adding this as is for now as the main use for this ClusterClass is e2e testing and a reference implementation.

/lgtm

chrischdi · 2022-04-08T08:42:57Z

For the cloud providers there currently are no pre-generated/pre-defined cluster classes to use (I checked the latest releases of CAPA, CAPZ and CAPG).

From a clusterctl perspective: Currently it is only consuming the released yaml files of the infrastructure providers (in case of this PR the docker infrastructure provider).
The docker infrastructure provider is the only one currently providing cluster class manifests (selected via the flag --flavor development-topology).

One thing we could do to improve here would be to add a security guidelines section to the book as proposed by fabrizzio here.

Edit: maybe we should move this discussion then back to the issue #6329 :-)

fabriziopandini · 2022-04-08T13:08:05Z

Whether or not add it to the default ClusterClass or add a separate ClusterClass e.g. called capi-quickstart-secure.
A second ClusterClass would have the con of duplicating the normal quick-start ClusterClass with lots of duplicated content.

If adding the PSS to the default cluster class results into issues we could also work the other way around and add a "insecure" ClusterClass named capi-quickstart-secure fur users to fallback to (clusterctl generate cluster capi-quickstart --flavor development-topology-insecure ...).

What if we add a variable, named enable-PodSecurityStandard defaulted to true, and we use this var to add/remove the changes in this PR via patches (instead of changing editing templates sources).

Pushing this a little bit further, we can make this variable a struct allowing users to define:

Pod Security standard name (restricted, baseline, privileged)
Applicable mode (enforce, warn, audit)
Applicable version (usually default)
(this can be also a follow up iteration)

Also, what about extending the scope of this PR including the creation of a security guidelines section n the book (probably under tasks) with a short description to how to enable pod-security-standards + an inline version of the above variable/patches and a link to the CAPD example

chrischdi · 2022-04-12T12:59:43Z

Whether or not add it to the default ClusterClass or add a separate ClusterClass e.g. called capi-quickstart-secure.
A second ClusterClass would have the con of duplicating the normal quick-start ClusterClass with lots of duplicated content.

If adding the PSS to the default cluster class results into issues we could also work the other way around and add a "insecure" ClusterClass named capi-quickstart-secure fur users to fallback to (clusterctl generate cluster capi-quickstart --flavor development-topology-insecure ...).

What if we add a variable, named enable-PodSecurityStandard defaulted to true, and we use this var to add/remove the changes in this PR via patches (instead of changing editing templates sources).

Pushing this a little bit further, we can make this variable a struct allowing users to define:

Pod Security standard name (restricted, baseline, privileged)

Applicable mode (enforce, warn, audit)

Applicable version (usually default)
(this can be also a follow up iteration)

Also, what about extending the scope of this PR including the creation of a security guidelines section n the book (probably under tasks) with a short description to how to enable pod-security-standards + an inline version of the above variable/patches and a link to the CAPD example

I did some refactoring of the PR :-)

I added a todo at the top-level comment so I don't forget to squash commits
Refactored the configuration to use patches via cluster class instead which allows configuring the levels and disabling the patches (keeping the default enabled)
book/quick-start: added a short part to mention how PSS could be disabled for the docker quickstart
book/writing cluster class: added a Limitation section to note the unability to append to empty slices via json patches
book/security-guidelines: added a section which as of now only contains adding pod security policy to a cluster class

PushkarJ

Few suggestions on docs for clarity. But overall looks good!!

docs/book/src/security-guidelines/index.md

docs/book/src/security-guidelines/pod-security-standards.md

docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md

docs/book/src/security-guidelines/pod-security-standards.md

PushkarJ · 2022-04-14T19:23:28Z

Would it make sense to add the content from #6152 in to the security guidelines section created in this PR?

Edit: To be clear, Not suggesting to add it in this PR but in follow up PRs :)

PushkarJ · 2022-04-14T21:57:33Z

/retitle Apply PodSecurityStandard in Secure Cluster Class

(Since the scope of the PR has changed thought this might be a better reflection of the work being done. Please revert/edit as needed)

killianmuldoon · 2022-04-15T11:28:23Z

/retitle ✨ Apply PodSecurityStandard in Secure Cluster Class

Just adding a prefix to make this work for our release note generation process 🙂

chrischdi · 2022-04-19T08:12:22Z

Thanks for the detailed review 👍

Would it make sense to add the content from #6152 in to the security guidelines section created in this PR?

Edit: To be clear, Not suggesting to add it in this PR but in follow up PRs :)

Should be fine to add the content of #6152 in a separate PR.

fabriziopandini

Last round of nits but really looking forward to get this merged

docs/book/src/security-guidelines/index.md

docs/book/src/security-guidelines/pod-security-standards.md

docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md

chrischdi · 2022-04-19T15:18:20Z

Addressed comments and rebased for squashing :-)

chrischdi · 2022-04-19T15:19:11Z

/test help

k8s-ci-robot · 2022-04-19T15:19:12Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-23-latest-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-informing-ipv6-main
pull-cluster-api-e2e-informing-main
pull-cluster-api-e2e-main
pull-cluster-api-test-main
pull-cluster-api-test-mink8s-main
pull-cluster-api-verify-main

In response to this:

/test help

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.

chrischdi · 2022-04-20T07:33:06Z

cert-manager failed. Should be unrelated

/test pull-cluster-api-e2e-main

fabriziopandini · 2022-04-20T10:40:34Z

/lgtm

PushkarJ · 2022-04-20T16:38:51Z

/lgtm

(Thank you so much for working on this @chrischdi !)

fabriziopandini · 2022-04-21T17:03:51Z

/test pull-cluster-api-test-full-main

k8s-ci-robot · 2022-04-21T17:03:52Z

@fabriziopandini: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main
/test pull-cluster-api-e2e-full-main
/test pull-cluster-api-e2e-informing-ipv6-main
/test pull-cluster-api-e2e-informing-main
/test pull-cluster-api-e2e-workload-upgrade-1-23-latest-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-informing-ipv6-main
pull-cluster-api-e2e-informing-main
pull-cluster-api-e2e-main
pull-cluster-api-test-main
pull-cluster-api-test-mink8s-main
pull-cluster-api-verify-main

In response to this:

/test pull-cluster-api-test-full-main

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.

fabriziopandini · 2022-04-21T17:04:14Z

/test pull-cluster-api-e2e-full-main

sbueringer

Very nice

I assume the current state is manually tested.

It would be really nice to see a follow-up PR to include those changes in our e2e test quickstart ClusterClass to:

find out if this is compatible with our e2e tests
to keep the diff between tested templates/ClusterClass and released template as small as possible

docs/book/src/security/pod-security-standards.md

docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md

* use parametrizable patches via ClusterClass instead of statically adding the Pod Security Standard * book: introduce security guidelines section * book: add security guidelines section about Pod Security Standard * book: add section about limitation of cluster class json patches Signed-off-by: Christian Schlotter <[email protected]>

chrischdi · 2022-04-27T18:33:02Z

Thanks for the detailed review. I applied changes according the following patch:

--- a/docs/book/src/security/pod-security-standards.md
+++ b/docs/book/src/security/pod-security-standards.md
@@ -3,7 +3,7 @@
 Pod Security Admission allows applying [Pod Security Standards] during creation of pods at the cluster level.

 The flavor `development-topology` for the docker provider used in [Quick Start](../user/quick-start.md) already includes a basic Pod Security Standard configuration.
-It is using variables and patches via ClusterClass to inject the configuration.
+It is using ClusterClass variables and patches to inject the configuration.

 ## Adding a basic Pod Security Standards configuration to a ClusterClass

@@ -51,7 +51,7 @@ spec:

 The following snippet contains the patch to be added to the ClusterClass.

-Due to [limitations of ClusterClass with patches](../tasks/experimental-features/cluster-class/write-clusterclass.md#limitations-of-clusterclass-with-patches) there are two versions for this patch.
+Due to [limitations of ClusterClass with patches](../tasks/experimental-features/cluster-class/write-clusterclass.md#json-patches-tips--tricks) there are two versions for this patch.

 {{#tabs name:"tab-configuration-patches" tabs:"Add to existing slice,Create slice"}}
 {{#tab Append}}
@@ -76,7 +76,7 @@ spec:
         matchResources:
           controlPlane: true
       jsonPatches:
-       - op: add
+      - op: add
         path: "/spec/template/spec/kubeadmConfigSpec/clusterConfiguration/apiServer/extraArgs"
         value:
           admission-control-config-file: "/etc/kubernetes/kube-apiserver-admission-pss.yaml"
@@ -133,7 +133,7 @@ kind: ClusterClass
 spec:
   ...
   patches:
-  - name: admissionPodSecurityPolicy
+  - name: podSecurityStandard
     description: "Adds an admission configuration for PodSecurity to the kube-apiserver."
     definitions:
     - selector:
@@ -190,7 +190,7 @@ spec:

 ### Create a secure Cluster using the ClusterClass

-After adding the variables and patdocs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.mdches the Pod Security Standards would be applied by default.
+After adding the variables and patches the Pod Security Standards would be applied by default.
 It is also possible to disable this patch or configure different levels for the configuration
 using variables.

diff --git a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md
index 32b6a1dc6..4d62bf6c7 100644
--- a/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md
+++ b/docs/book/src/tasks/experimental-features/cluster-class/write-clusterclass.md
@@ -685,12 +685,11 @@ add operation must exist.
 As a consequence ClusterClass authors should pay special attention when the following
 conditions apply in order to prevent errors when a patch is applied:

-* the patch is a JSON patch
-* the patch tries to `add` a value to a **slice**
+* the patch tries to `add` a value to an **array** (which is a **slice** in the corresponding go struct)
 * the slice was defined with `omitempty`
 * the slice currently does not exist

-A workaround in this particular case is to create the slice in the patch instead of adding it to the non-existing one.
+A workaround in this particular case is to create the array in the patch instead of adding to the non-existing one.
 When creating the slice, existing values would be overwritten so this should only be used when it does not exist.

 The following example shows both cases to consider while writing a patch for adding a value to a slice.

I will verify again the last open point tomorrow.

Edit: verified and works as outlined in the doc

sbueringer · 2022-04-27T18:41:21Z

Check, looks good so far

sbueringer · 2022-04-27T19:37:29Z

Thank you!
/lgtm

fabriziopandini

/lgtm
/approve

k8s-ci-robot · 2022-04-28T09:52:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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:

~~OWNERS~~ [fabriziopandini]

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 added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 7, 2022

k8s-ci-robot requested review from JoelSpeed and stmcginnis April 7, 2022 12:54

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 7, 2022

k8s-ci-robot assigned killianmuldoon Apr 8, 2022

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

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 12, 2022

PushkarJ reviewed Apr 14, 2022

View reviewed changes

k8s-ci-robot changed the title ~~✨ CAPD: add PodSecurityStandard to quickstart topology for security by default~~ Apply PodSecurityStandard in Secure Cluster Class Apr 14, 2022

k8s-ci-robot changed the title ~~Apply PodSecurityStandard in Secure Cluster Class~~ ✨ Apply PodSecurityStandard in Secure Cluster Class Apr 15, 2022

chrischdi force-pushed the issue-6329-add-admission-pss branch from 6b38f19 to ca3eb82 Compare April 19, 2022 08:09

fabriziopandini reviewed Apr 19, 2022

View reviewed changes

chrischdi force-pushed the issue-6329-add-admission-pss branch from ca3eb82 to 96ed69d Compare April 19, 2022 15:17

k8s-ci-robot assigned fabriziopandini Apr 20, 2022

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

k8s-ci-robot assigned PushkarJ Apr 20, 2022

sbueringer reviewed Apr 27, 2022

View reviewed changes

chrischdi force-pushed the issue-6329-add-admission-pss branch from 96ed69d to 9d3927f Compare April 27, 2022 18:32

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2022

k8s-ci-robot assigned sbueringer Apr 27, 2022

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

fabriziopandini reviewed Apr 28, 2022

View reviewed changes

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

k8s-ci-robot merged commit 813e702 into kubernetes-sigs:main Apr 28, 2022

k8s-ci-robot added this to the v1.2 milestone Apr 28, 2022

chrischdi deleted the issue-6329-add-admission-pss branch June 30, 2022 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Apply PodSecurityStandard in Secure Cluster Class #6390

✨ Apply PodSecurityStandard in Secure Cluster Class #6390

chrischdi commented Apr 7, 2022 •

edited

Loading

linux-foundation-easycla bot commented Apr 7, 2022 •

edited

Loading

killianmuldoon commented Apr 7, 2022 •

edited

Loading

killianmuldoon commented Apr 7, 2022

PushkarJ commented Apr 7, 2022

PushkarJ commented Apr 7, 2022 •

edited

Loading

killianmuldoon commented Apr 8, 2022

chrischdi commented Apr 8, 2022 •

edited

Loading

fabriziopandini commented Apr 8, 2022 •

edited

Loading

chrischdi commented Apr 12, 2022

PushkarJ left a comment

PushkarJ commented Apr 14, 2022 •

edited

Loading

PushkarJ commented Apr 14, 2022

killianmuldoon commented Apr 15, 2022

chrischdi commented Apr 19, 2022

fabriziopandini left a comment

chrischdi commented Apr 19, 2022

chrischdi commented Apr 19, 2022

k8s-ci-robot commented Apr 19, 2022

chrischdi commented Apr 20, 2022

fabriziopandini commented Apr 20, 2022

PushkarJ commented Apr 20, 2022

fabriziopandini commented Apr 21, 2022

k8s-ci-robot commented Apr 21, 2022

fabriziopandini commented Apr 21, 2022

sbueringer left a comment

chrischdi commented Apr 27, 2022 •

edited

Loading

sbueringer commented Apr 27, 2022

sbueringer commented Apr 27, 2022 •

edited

Loading

fabriziopandini left a comment

k8s-ci-robot commented Apr 28, 2022

✨ Apply PodSecurityStandard in Secure Cluster Class #6390

✨ Apply PodSecurityStandard in Secure Cluster Class #6390

Conversation

chrischdi commented Apr 7, 2022 • edited Loading

linux-foundation-easycla bot commented Apr 7, 2022 • edited Loading

killianmuldoon commented Apr 7, 2022 • edited Loading

killianmuldoon commented Apr 7, 2022

PushkarJ commented Apr 7, 2022

PushkarJ commented Apr 7, 2022 • edited Loading

killianmuldoon commented Apr 8, 2022

chrischdi commented Apr 8, 2022 • edited Loading

fabriziopandini commented Apr 8, 2022 • edited Loading

chrischdi commented Apr 12, 2022

PushkarJ left a comment

Choose a reason for hiding this comment

PushkarJ commented Apr 14, 2022 • edited Loading

PushkarJ commented Apr 14, 2022

killianmuldoon commented Apr 15, 2022

chrischdi commented Apr 19, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

chrischdi commented Apr 19, 2022

chrischdi commented Apr 19, 2022

k8s-ci-robot commented Apr 19, 2022

chrischdi commented Apr 20, 2022

fabriziopandini commented Apr 20, 2022

PushkarJ commented Apr 20, 2022

fabriziopandini commented Apr 21, 2022

k8s-ci-robot commented Apr 21, 2022

fabriziopandini commented Apr 21, 2022

sbueringer left a comment

Choose a reason for hiding this comment

chrischdi commented Apr 27, 2022 • edited Loading

sbueringer commented Apr 27, 2022

sbueringer commented Apr 27, 2022 • edited Loading

fabriziopandini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 28, 2022

chrischdi commented Apr 7, 2022 •

edited

Loading

linux-foundation-easycla bot commented Apr 7, 2022 •

edited

Loading

killianmuldoon commented Apr 7, 2022 •

edited

Loading

PushkarJ commented Apr 7, 2022 •

edited

Loading

chrischdi commented Apr 8, 2022 •

edited

Loading

fabriziopandini commented Apr 8, 2022 •

edited

Loading

PushkarJ commented Apr 14, 2022 •

edited

Loading

chrischdi commented Apr 27, 2022 •

edited

Loading

sbueringer commented Apr 27, 2022 •

edited

Loading