feat(chart): Improved default security context #7279

stevehipwell · 2024-10-25T15:53:20Z

Fixes #N/A

Description
This PR improves the default security context posture and adds support for setting container values which should be user defined.

How was this change tested?
The Helm chart was templated with the new values.

Does this change impact docs?

Yes, PR includes docs updates
Yes, issue opened: #
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

netlify · 2024-10-25T15:53:37Z

✅ Deploy Preview for karpenter-docs-prod canceled.

Name	Link
🔨 Latest commit	`6d4e9fa`
🔍 Latest deploy log	https://app.netlify.com/sites/karpenter-docs-prod/deploys/67c8288a90c62100088c745a

stevehipwell · 2024-10-25T15:54:04Z

CC @jonathan-innis

stevehipwell · 2024-11-18T09:48:26Z

@tzneal could you take a look at this please?

github-actions · 2024-12-02T12:06:33Z

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

stevehipwell · 2024-12-02T17:04:41Z

@jonathan-innis @tzneal could someone please take a look at this?

stevehipwell · 2024-12-12T10:43:54Z

@jonathan-innis @tzneal could someone please take a look at this?

saurav-agarwalla · 2024-12-13T17:09:48Z

I'll take a look at this.

github-actions · 2024-12-28T12:04:59Z

This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity.

stevehipwell · 2025-01-02T13:04:30Z

/not-stale

stevehipwell · 2025-01-14T15:40:06Z

@saurav-agarwalla did you manage to take a look at this?

saurav-agarwalla · 2025-01-14T21:40:16Z

@stevehipwell apologies for the delay, I started reviewing this but then got side-tracked with the holidays and other things. Planning to get back to it this week.

saurav-agarwalla · 2025-01-02T20:57:44Z

charts/karpenter/README.md

@@ -72,7 +75,7 @@ cosign verify public.ecr.aws/karpenter/karpenter:1.1.1 \
 | podDisruptionBudget.maxUnavailable | int | `1` |  |
 | podDisruptionBudget.name | string | `"karpenter"` |  |
 | podLabels | object | `{}` | Additional labels for the pod. |
-| podSecurityContext | object | `{"fsGroup":65532}` | SecurityContext for the pod. |
+| podSecurityContext | object | `{"fsGroup":65532,"runAsNonRoot":true,"seccompProfile":{"type":"RuntimeDefault"}}` | SecurityContext for the pod. |


Why do we need to change the defaults?

Some systems need this making explicit for policy purposes.

Same comment as the other one below. As per the documentation, we're changing the desired behavior by setting these values so I am not sure if we should be setting this for everyone:

Indicates that the container must run as a non-root user. If true, the Kubelet will validate the image at runtime to ensure that it does not run as UID 0 (root) and fail to start the container if it does. If unset or false, no such validation will be performed. May also be set in SecurityContext. If set in both SecurityContext and PodSecurityContext, the value specified in SecurityContext takes precedence.

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.32/#securitycontext-v1-core

Karpenter runs as non-root so this can be set with no implications. It is 100% a best practice to set this.

@stevehipwell My question was more around if/why this needs to be a breaking change (although I don't disagree that this is a best practice).

It's not a breaking change, Karpenter already runs as non-root!

saurav-agarwalla · 2025-01-02T20:58:16Z

charts/karpenter/README.md

@@ -90,7 +93,6 @@ cosign verify public.ecr.aws/karpenter/karpenter:1.1.1 \
 | settings.clusterCABundle | string | `""` | Cluster CA bundle for TLS configuration of provisioned nodes. If not set, this is taken from the controller's TLS configuration for the API server. |
 | settings.clusterEndpoint | string | `""` | Cluster endpoint. If not set, will be discovered during startup (EKS only) |
 | settings.clusterName | string | `""` | Cluster name. |
-| settings.eksControlPlane | bool | `false` | Marking this true means that your cluster is running with an EKS control plane and Karpenter should attempt to discover cluster details from the DescribeCluster API |


Any reason this is being removed?

I'm not sure, I guess it might be a merge artifact.

Can you add it back? I am seeing this in the latest main so I don't think we want to remove it: https://github.com/aws/karpenter-provider-aws/blob/main/charts/karpenter/README.md?plain=1#L93.

I'll add this back.

So it turns out I didn't remove anything; whoever added this didn't configure the helm-docs comment correctly and then manually updated the README to work around it. When I ran helm-docs for this PR it was removed.

saurav-agarwalla · 2025-01-02T21:00:36Z

charts/karpenter/templates/deployment.yaml

@@ -62,16 +62,29 @@ spec:
      containers:
        - name: {{ .Values.controller.containerName | default "controller" }}
          securityContext:
+            privileged: false


Is this something you wanted to add? If so, should we parameterize it for backwards compatibility like the others?

There is no need to make this optional as Karpenter doesn't need it. Some systems want it explicitly set for policy purposes.

From the docs:

privileged boolean
Run container in privileged mode. Processes in privileged containers are essentially equivalent to root on the host. Defaults to false. Note that this field cannot be set when spec.os.name is windows.

So I don't think we can set it without parameterizing it.

The default is un privileged, we're just making it explicit as some org policies require it to be so. Karpenter will never need this so there is no valid reason to set it otherwise. Helm charts should only offer customisation where required, first party Helm charts even more so.

The default is un privileged, we're just making it explicit as some org policies require it to be so.

But setting it will break windows like I linked above. So why not offer it as a parameter for anyone who wants to set it?

@saurav-agarwalla Karpenter is linux only!

coveralls · 2025-01-17T19:34:59Z

Pull Request Test Coverage Report for Build 12771160071

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 64.959%

Totals
Change from base Build 12754714725:	0.02%
Covered Lines:	5782
Relevant Lines:	8901

💛 - Coveralls

stevehipwell · 2025-01-23T12:00:08Z

@saurav-agarwalla I think there is some confusion over what a first party Helm chart should be. A first party Helm chart should be opinionated and follow both industry and project best practices. As it owns the constraints it doesn't need to open everything up for customization.

Karpenter's constraints are that it runs on Linux, as a non-root user, and doesn't need any additional OS permissions; therefore there is no valid reason to allow an end-user to lower this security posture.

CC @jonathan-innis @tzneal @bwagner5

charts/karpenter/templates/deployment.yaml

jonathan-innis · 2025-02-07T06:29:09Z

charts/karpenter/values.yaml

@@ -51,7 +51,10 @@ podDisruptionBudget:
  maxUnavailable: 1
 # -- SecurityContext for the pod.
 podSecurityContext:
+  runAsNonRoot: true


Can we do this? I think we explicitly avoided doing this since it would change the security context of sidecar containers that may not support the more locked-down configuration

This sets the default which can then be overridden by the consumer. Karpenter would be a high value target and this makes it really clear when adding a sidecar that running as root impacts the whole pod and not just the container.

We did this and then pulled it back because multiple people ran into issues with it -- I'm fine having a more open discussion on this in something like a working group but I'm against breaking users in this way after the number of issues that were opened when we first tried to make this change (see #4681)

I'm pretty weary of this, too since this is a breaking change to the chart post-v1 and I believe that folks will miss this during the upgrade process

@jonathan-innis let's take this to a separate discussion, I'll change this to false for the PR.

That said I am very interested why it was reversed in the first place? It significantly increases the attack surface for all users, but off the top of my head I can't think of many valid uses for root sidecars? Furthermore anyone using sidecars as root should have the K8s contextual knowledge to be proactively checking the pod security spec of the targets.

charts/karpenter/values.yaml

jonathan-innis · 2025-02-07T06:30:17Z

Hey @stevehipwell -- sorry that we've been slow on this -- I just took a look, most of the changes look reasonable -- just a few questions about potential breaking changes and why you need certain levels of customization

stevehipwell · 2025-02-07T14:21:16Z

@jonathan-innis I've replied to your comments and rebased.

stevehipwell · 2025-02-14T10:23:59Z

@jonathan-innis I've reverted the pod non-root default.

Signed-off-by: Steve Hipwell <[email protected]>

stevehipwell · 2025-03-05T10:34:34Z

@jonathan-innis I've rebased this PR again and it'd be great to get it merged.

stevehipwell requested a review from a team as a code owner October 25, 2024 15:53

stevehipwell requested a review from tzneal October 25, 2024 15:53

stevehipwell force-pushed the chart-security-context branch from e211a98 to 0e90ebf Compare November 4, 2024 15:42

stevehipwell force-pushed the chart-security-context branch from 0e90ebf to dacfc30 Compare November 18, 2024 09:47

github-actions bot added the lifecycle/stale label Dec 2, 2024

stevehipwell force-pushed the chart-security-context branch from dacfc30 to 5cc1c72 Compare December 2, 2024 17:03

github-actions bot removed the lifecycle/stale label Dec 3, 2024

stevehipwell force-pushed the chart-security-context branch from 5cc1c72 to be44880 Compare December 12, 2024 10:43

jonathan-innis assigned saurav-agarwalla Dec 13, 2024

github-actions bot added the lifecycle/stale label Dec 28, 2024

stevehipwell force-pushed the chart-security-context branch from be44880 to 144aa53 Compare January 2, 2025 13:03

github-actions bot removed the lifecycle/stale label Jan 3, 2025

stevehipwell force-pushed the chart-security-context branch from 144aa53 to e351477 Compare January 14, 2025 15:39

saurav-agarwalla reviewed Jan 17, 2025

View reviewed changes

stevehipwell force-pushed the chart-security-context branch 4 times, most recently from a81a0d6 to 35471b8 Compare January 20, 2025 11:12

jonathan-innis reviewed Feb 7, 2025

View reviewed changes

stevehipwell force-pushed the chart-security-context branch from 35471b8 to 934ae1b Compare February 7, 2025 14:20

stevehipwell force-pushed the chart-security-context branch 2 times, most recently from b66b744 to 86b5dd8 Compare February 14, 2025 10:23

feat(chart): Improved default security context

6d4e9fa

Signed-off-by: Steve Hipwell <[email protected]>

stevehipwell force-pushed the chart-security-context branch from 86b5dd8 to 6d4e9fa Compare March 5, 2025 10:33

feat(chart): Improved default security context #7279

Are you sure you want to change the base?

feat(chart): Improved default security context #7279

Conversation

stevehipwell commented Oct 25, 2024

netlify bot commented Oct 25, 2024 • edited Loading

✅ Deploy Preview for karpenter-docs-prod canceled.

stevehipwell commented Oct 25, 2024

stevehipwell commented Nov 18, 2024

github-actions bot commented Dec 2, 2024

stevehipwell commented Dec 2, 2024

stevehipwell commented Dec 12, 2024

saurav-agarwalla commented Dec 13, 2024

github-actions bot commented Dec 28, 2024

stevehipwell commented Jan 2, 2025

stevehipwell commented Jan 14, 2025

saurav-agarwalla commented Jan 14, 2025

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

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

coveralls commented Jan 17, 2025

Pull Request Test Coverage Report for Build 12771160071

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

stevehipwell commented Jan 23, 2025

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

jonathan-innis commented Feb 7, 2025

stevehipwell commented Feb 7, 2025

stevehipwell commented Feb 14, 2025

stevehipwell commented Mar 5, 2025

netlify bot commented Oct 25, 2024 •

edited

Loading