Introduce new SecurePodDefaults options

[nader-ziada]

Fixes #14029

Proposed Changes

• SecurePodDefaults adds a new more secure options, but keeps default as disabled for now

Release Note

    - create a new default value for secure-pod-defaults: AllowRootBounded
    - default is still disabled
    - when AllowRootBounded, defaults SeccompProfile and Capabilities if nil
    - when enabled sets RunAsNonRoot to true if not already specified

[codecov]

Codecov Report

❌ Patch coverage is 54.54545% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.02%. Comparing base (561f348) to head (c7a1ea4).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/testing/v1/revision.go	0.00%	20 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16042      +/-   ##
==========================================
- Coverage   80.07%   80.02%   -0.05%     
==========================================
  Files         214      214              
  Lines       16907    16932      +25     
==========================================
+ Hits        13538    13550      +12     
- Misses       3013     3021       +8     
- Partials      356      361       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nader-ziada]

/test upgrade-tests

[dprotaso]

Thanks for the release notes in the PR body.

One expected side-effect of this change is that certain images will break with the new default.

eg. nginx will no longer run since it tries to do a chown syscall on startup

nginx: [emerg] chown("/var/cache/nginx/client_temp", 101) failed (1: Operation not permitted)

caddy (which runs as root) runs on on the default profile now. It fails on the restricted one which is expected.

As a follow up (separate PR) I notice the caddy image error doesn't propagate up properly to the Revision properly. The Pod has

      state:
        waiting:
          message: 'container has runAsNonRoot and image will run as root (pod: "hello-00001-deployment-5c77b7fb5b-w7r74_default(271b9aaa-340d-4073-914c-2144c8560273)",
            container: user-container)'
          reason: CreateContainerConfigError

[dprotaso]

/assign @evankanderson for a security WG review

[evankanderson]

I think we should also plan to flag changing any defaults here as a breaking change if we decide to do so, along with clear instructions about what the breakage looks like, and how to switch back to the current behavior.

In particular, I think nginx is probably a great example to show / test with, since many people may use it to serve static content.

From a security point of view, I think getting towards safer defaults without breaking too many things for users is a net win, so I think it makes sense from a security perspective.

[evankanderson]

This feels like it's doing something different, since RunAsNonRoot is a tri-state (*bool).

Before, if SecurePodDefaults was enabled, this would set a missing RunAsNonRoot to true . If RunAsNonRoot was set to false, it would leave it as false. The new flag will unconditionally set RunAsNonRoot, which feels like it might be a value which is always wrong.

What about adding an root-okay value between disabled and enabled, and then aiming to change the default to root-okay in an upcoming release (it could be 1.20 or 1.21)?

[nader-ziada]

Can we make RootAllowed as the default and that would work the same as you described where it allows RunAsRoot to true if empty or false if the user set it to that.
and keep Restricted but not set it as a default until a later release?

[nader-ziada]

it could also be called RootEnabled

[evankanderson]

Yes, I think RootAllowed is probably a good name for this intermediate setting, and I think it's reasonable to make it the new default in the same release; upgrade instructions should flag that setting the config to disabled prior to upgrading will preserve existing behavior.

Since this is an admission webhook, I think it will only affect new Revisions, correct?

[nader-ziada]

Everything else in the config is called enabled, should we use that for the meaning of root-allowed and mention it in the description? or do you actually want it to be called root-allowed and remove enabled

Since this is an admission webhook, I think it will only affect new Revisions, correct?

yes I tested with an existing cluster and the existing revisions were fine, only applied on create

[nader-ziada]

@evankanderson wdyt?

[evankanderson]

I would like the enum values to clearly indicate where on the security continuum the value falls. We currently have:

• disabled -- least secure
• enabled -- some security, low chance of incompatibility
• restricted -- highest security, medium chance of incompatibility

The problem is that on a quick read, it's not clear to me if enabled > restricted or restricted > enabled in terms of security. I'd sort of expect that enabled means as much security as possible, so I'd like things to look like:

• disabled -- least secure
• $BETTER -- some security, low chance of incompatibility
• enabled -- highest security, medium chance of incompatibility

We could call $BETTER something like transition or root-allowed, but it seems odd for enabled to be an intermediate step, and for users who've already turned on enabled in 1.19 (where it means "highest security") to have a security backslide until they change to restricted in 1.20 (which would not be allowed in 1.19). On the other hand, if we add $BETTER in the middle as a default, users already on enabled don't need to change anything, users who want disabled can set it explicitly before upgrade, and users who don't care can simply accept the upgrade (and possibly set the value afterwards).

Sorry for the late reply -- hope this makes sense.

[nader-ziada]

yes, makes perfect sense, thank you for the clarification. Will make the necessary updates and let you know

[nader-ziada]

@evankanderson @dprotaso

here is a summary of the changes:

• create a new default value for secure-pod-defaults: SecureDefaultsOverridable
• applies secure defaults but honors RunAsNonRoot
• sets RunAsNonRoot to true if not already specified
• when secure-pod-defaults = 'enabled', set RunAsNonRoot to true
• sets kubernetes.podspec-securitycontext to "enabled"

[evankanderson]

@evankanderson @dprotaso

here is a summary of the changes:

• create a new default value for secure-pod-defaults: SecureDefaultsOverridable
• applies secure defaults but honors RunAsNonRoot
• sets RunAsNonRoot to true if not already specified
• when secure-pod-defaults = 'enabled', set RunAsNonRoot to true
• sets kubernetes.podspec-securitycontext to "enabled"

Sorry to be going around on this, but what's the user benefit to setting secure-pod-defaults = enabled instead of using the pod-security.kubernetes.io/enforce: restricted label on a namespace? The latter enforces the settings filled in by secure-pod-defaults, but also applies to non-Knative pods in the same namespace.

The previous purpose of secure-pod-defaults was to prevent users from accidentally wandering into the "less secure" space that Kubernetes uses as a historical default, without requiring every Knative Service manifest to specify a bunch of securityContexts.

runAsNonRoot is a weird, special flag on the PodSpec, because it acts as a "spoiler" on kubelet running the pod after admission and scheduling. A pod may be accepted with RunAsNonRoot=true and runAsUser=nil, and then fail to run if the container image has USER 0, USER root, or some other such content. runAsNonRoot=true' (but not runAsUser`) is required by the Pod Security Standards "Restricted" profile. This is further complicated by history:

• Docker defaults to building containers with USER 0 if you don't issue a USER directive.
• Docker supports both USER <uid> and USER <name> formats; the latter requires pulling the image layer contents for the highest layer which contains /etc/passwd.
  • Layers don't declare which files they contain...
• A number of popular containers (in particular, nginx) run as root and may be used to (for example) serve static content. Knative would naturally be a place for that.

The "correct" solution is to rebuild the container and/or set runAsUser to a non-zero value, possibly also adding the NET_BIND_SERVICE capability. This will fix compatibily for some containers without getting into the "my pod launched, but then doesn't run" failure state that I suspect will lead to user frustration with the feature.

My suggestion would be to change secure-defaults-overrideable to no-strict-nonroot, and then change lines 250-264 of revision_defaults.go to:

	// RunAsNonRoot breaks more containers than other settings, see discussion in #16042
	if cfg.Features.SecurePodDefaults == config.Enabled {
		if psc.RunAsNonRoot == nil {
			updatedSC.RunAsNonRoot = ptr.Bool(true)
		}
	}

In either case, it feels like the enforcement of RunAsNonRoot would be better handled using either Pod Security Admission on the application namespaces or ValidatingAdmissionPolicy.

Of course, it's also possible that I'm missing a use-case here, but this was mostly intended to be a "better defaults than Kubernetes" feature in the same way that we do for probes and network setup.

[nader-ziada]

My suggestion would be to change secure-defaults-overrideable to no-strict-nonroot, and then change lines 250-264 of revision_defaults.go to:

	// RunAsNonRoot breaks more containers than other settings, see discussion in #16042
	if cfg.Features.SecurePodDefaults == config.Enabled {
		if psc.RunAsNonRoot == nil {
			updatedSC.RunAsNonRoot = ptr.Bool(true)
		}
	}

In either case, it feels like the enforcement of RunAsNonRoot would be better handled using either Pod Security Admission on the application namespaces or ValidatingAdmissionPolicy.

Of course, it's also possible that I'm missing a use-case here, but this was mostly intended to be a "better defaults than Kubernetes" feature in the same way that we do for probes and network setup.

apologies for maybe not understand, but if securePodDefaults is set enabled, we still don't need to set the RunAsNonRoot to true? I though the point of this issue is to eventually make RunAsNonRoot as true the default behaviour. The code snippt above only sets it to true if its nil, but doesn't that allows the user to set it to false, which is the same as secure-defaults-overrideable or no-strict-nonroot

[dprotaso]

In either case, it feels like the enforcement of RunAsNonRoot would be better handled using either Pod Security Admission on the application namespaces or ValidatingAdmissionPolicy.

Of course, it's also possible that I'm missing a use-case here, but this was mostly intended to be a "better defaults than Kubernetes" feature in the same way that we do for probes and network setup.

Interesting - I didn't realize there was an K8s admission controller that was already doing defaulting on the pods. I was under the impression there was a validation controller that would prevent pods from running.

@evankanderson I guess what do we want to do here? Do we want to remove this feature entirely (and the warnings) and just provide docs about 'securing workloads' and linking to the K8s Pod Security Admission ?

[dprotaso]

For some historical context the original PR (#13398) was done in Oct 2022 which is roughly a month or so after K8s 1.25 was released with security pod admission going stable - https://kubernetes.io/docs/concepts/security/pod-security-admission/

Given we trail K8s releases by some time we probably introduced the secure defaulting because it wasn't available at the time.

[evankanderson]

Kubernetes does not have secure defaulting; it does have policies to enforce (block) pods which don't set the fields to approved values.

My concern is the behavior of a pod which uses a user container which only functions when running as the root user (for example, the binary or shared libraries are in a 0700 directory owned by root).

If a user submits the pod with runAsNonRoot=false (explicit opt-out), overriding this value will simply move around the error if the underlying pod security admission policy is set. (Instead of the Pod falling admission, it will be created and then later fail to run after scheduling.) Worse, if a user submits the pod in a namespace without the restricted pod security admission annotation, overriding runAsNonRoot will cause us to change a working explicit podspec to a failing explicit podspec, which seems particularly frustrating.

I'm fine with setting a default if the value was previously unset (the Kubernetes behavior here values backwards compatibility too highly in my opinion), but forcibly overriding user intent seems hostile.

[nader-ziada]

@evankanderson @dprotaso what do you think about this:

My understanding is that the concern is the current code changes in this PR would override the user RunAsNonRoot=false when the secure-pod-defaults is enabled

the way I implemented the feature in this PR is:

• disabled: do nothing
• [default]secure-defaults-overrideable or no-strict-nonroot: sets RunAsNonRoot to true unless a value is already specified by the user.
• enabled: force setting RunAsNonRoot to true

here are some suggestions for how we can move forward:

• instead of forcing the value of RunAsNonRoot when enabled, reject the pod with an error if it's set to false similar to how Pod Security Admission works
• only have disabled and enabled where enabled will be the default and sets RunAsNonRoot to true unless a value is already specified by the user.
• keep the current implementation but improve docs about how to make it more secure, maybe enhance the warning message

[evankanderson]

My thoughts about this (though I'm happy to be overruled):

• We shouldn't duplicate controls that are already present in Kubernetes. Some of what we do could be fine in policy frameworks like OPA gatekeeper or Kyverno, but those are not universally deployed (unlike Kubernetes, which is a hard dependency).

• The default Kubernetes and Docker defaults here aren't very good, and they sort of combine for a security "sharp edge":

  • Docker defaults to running as UID 0, and makes it hard to easily determine the effective container UID under casual inspection
  • The ecosystem relies on a number of containers which are built to operate as UID 0 (hello, nginx my old friend)
  • Kubernetes early defaults were built for adoption, so they tended to prefer "weak security" over "need to adjust a container to get it to work"
  • Kubernetes has the possibility of stronger security settings now, but the default matches the historical trend, unfortunately

What I would like to see here is:

1. The required spec for a Knative Service doesn't get longer. The fact that you can fit a Knative Service YAML in 200 characters is a good example of how we simplify the development experience. Forcing users to add a bunch of security annotations to use Knative out of the box (e.g. shipping with "secure by default" by adding work to every user) feels like a miss.

2. A racheting mechanism whereby cluster operators can move towards higher security and prevent backsliding feels like a win. Pod Security Admission has this with the warn vs enforce settings. I'd be okay with using those annotations to trigger these updates if we want to go that way. Kubernetes doesn't do this today, but it could.

   • The current on/off settings are okay, but changing the current default from "disabled" to "enabled" will probably break nginx and other containers more than we want. In the field telemetry could convince me that this is wrong, but I didn't know that we have a mechanism to collect that...
   • My guess is that an in-between rachet setting that defaults all the unspecified security settings except runAsNonRoot would probably break a minimal number of containers.
     My intuition is that setting the default to the current "enabled" will break about 2% of deployments, and the in-between would break 0.2% of deployments, but those are unscientific gut numbers. Big security wins from in-between would probably be from setting the seccomp profile to RuntimeDefault, instead of the default value of Unconstrained.

[nader-ziada]

would this match what we want to see and provide better secure defaults?

• disabled: nothing
• medium: (default) sets seccomp profile to RuntimeDefault by default, but doesn't set RunAsNonRoot
• enabled: sets seccomp profile to RuntimeDefault by default, and RunAsNonRoot to true unless the user set it to false

[dprotaso]

I was wrong about k8s defaulting settings.

Given the naming issue of the setting - I'm wondering if we should use the profile names of Pod Security Standards

Then Knative's obligation is to 'default' settings appropriate to satisfy the profile.

I'm also wondering if we shouldn't enable this by default in this release (sorry) - or maybe we introduce that as a new feature and deprecate this one

[evankanderson]

/lgtm
/approve

/hold if you want to make the other slices.Contains changes (I don't care strongly)

[nader-ziada]

/lgtm /approve

/hold if you want to make the other slices.Contains changes (I don't care strongly)

thanks for all the reviews and help @evankanderson

I added the rest of the slice.Contains, looks and read much better than the long if

[evankanderson]

/lgtm
/hold cancel

[evankanderson]

/approve

[evankanderson]

/assign @dprotaso

[dprotaso]

/lgtm
/approve

🎉 thanks for the improvements

@evankanderson @nader-ziada Are we ok with flipping the default in 1.21 release? Assuming in 1.20 we notify people about the upcoming change

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, evankanderson, nader-ziada

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [dprotaso]
• pkg/apis/OWNERS [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[evankanderson]

/lgtm
/approve

🎉 thanks for the improvements

@evankanderson @nader-ziada Are we ok with flipping the default in 1.21 release? Assuming in 1.20 we notify people about the upcoming change

Yes, I think flipping the default to AllowRootConstrained in an upcoming release is entirely reasonable (such as 1.21 specifically).

[dprotaso]

@nader-ziada can you add docs to the website around this secure-pod-defaults?

[nader-ziada]

@nader-ziada can you add docs to the website around this secure-pod-defaults?

knative/docs#6461