Add validation to warn about insecure SecurityContext defaults #13399

evankanderson · 2022-10-16T17:27:56Z

It would be nice if the Kubernetes defaults for SecurityContext and PodSecurityContext followed the
Pod Security Standards restricted profile, which
is "following current Pod hardening best practices", but they do not, presumably for historical reasons.

Since all the SecurityContext values which have insecure defaults can distinguish between "default value" and "declared insecure"
(either via pointer or empty-string vs dangerous string), it seems reasonable to warn about these defaults in Knative, and
default them to "safe" values in the future, since we've knowingly restricted our set of workloads to those that should be safer.

Proposed Changes

Add warning-level errors for containers whose effective SecurityContext (combining pod-level and container-level settings) are unset.
Explicitly setting values to their (insecure) defaults will disable the warning. This is intentional, we only want to alert people who
may not know the default values have higher privilege than necessary.
Mention in the warning that future Knative versions may default to safer values.

Release Note

Knative will now _warn_ (but not error) when creating or updating a PodSpec
where containers have additional privilege due to unset SecurityContext values.
Explicitly setting these values to any setting, including high-privilege ones,
will disable this warning.

These fields are:
* `runAsNonRoot` (empty means `false`)
* `allowPrivilegeEscalation` (empty means `true`)
* `seccompProfile.type` (empty string means `Unconfined`)
* `capabilities.drop` (default maintains privileges, use `ALL` to drop unneeded linux capabilities)

knative-prow · 2022-10-16T17:28:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson

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:

~~pkg/apis/OWNERS~~ [evankanderson]

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

evankanderson · 2022-10-16T17:41:09Z

I need to add an errLevel parameter to all the tests in pkg/apis/serving/v1

…default SecurityContexts dozens of times

evankanderson · 2022-10-16T19:09:11Z

@mattmoor -- it looks like "warning level errors" are translating into error return values from the generated clients e.g. https://github.com/knative/serving/blob/main/pkg/reconciler/configuration/configuration.go#L69-L82

    configuration_test.go:570: Missing status update for serving.knative.dev/v1, Kind=Configuration/foo/no-revisions-yet (-want, +prevState):
          &v1.Configuration{
          	TypeMeta:   {},
          	ObjectMeta: {Name: "no-revisions-yet", Namespace: "foo", Generation: 1234},
          	Spec:       {Template: {Spec: {PodSpec: {Containers: {{Name: "user-container", Image: "busybox", Resources: {Limits: {}, Requests: {}}, ReadinessProbe: &{ProbeHandler: {TCPSocket: &{}}, SuccessThreshold: 1}, ...}}}, ContainerConcurrency: &0, TimeoutSeconds: &60}}},
          	Status: v1.ConfigurationStatus{
          		Status: v1.Status{
          			ObservedGeneration: 1234,
          			Conditions: v1.Conditions{
          				{
          					Type:     "Ready",
        - 					Status:   "Unknown",
        + 					Status:   "False",
          					Severity: "",
          					... // 1 ignored field
        - 					Reason:  "",
        + 					Reason:  "RevisionFailed",
        - 					Message: "",
        + 					Message: "Revision creation failed with message: Kubernetes default value is insecure, Knative may default this to secure in a future release: spec.securityContext.containers[1].allowPrivilegeEscalation, spec.securityContext.containers[1].capabilities, spec.security"...,
          				},
          			},
          			Annotations: nil,
          		},
          		ConfigurationStatusFields: v1.ConfigurationStatusFields{
          			LatestReadyRevisionName:   "",
        - 			LatestCreatedRevisionName: "no-revisions-yet-01234",
        + 			LatestCreatedRevisionName: "",
          		},
          	},
          }
    configuration_test.go:570: Unexpected event(-want, +got):
          strings.Join({
        - 	`Normal Created Created Revision "no-revisions-yet-01234"`,
        + 	"Warning CreationFailed Failed to create Revision: Kubernetes def",
        + 	"ault value is insecure, Knative may default this to secure in a ",
        + 	"future release: spec.securityContext.containers[1].allowPrivileg",
        + 	"eEscalation, spec.securityContext.containers[1].capabilities, sp",
        + 	"ec.securityContext.containers[1].runAsNonRoot, spec.securityCont",
        + 	"ext.containers[1].seccompProfile",
          }, "")

mattmoor · 2022-10-16T19:12:35Z

Ah no, that's because this was never taught about warnings:
https://github.com/knative/pkg/blob/b812affa38934994dd27d29b158cfe6eb61963ea/reconciler/testing/reactions.go#L49-L71

mattmoor · 2022-10-16T19:13:18Z

tl;dr we added reactors to simulate validation webhooks in our table tests by calling Validate()

evankanderson · 2022-10-16T22:19:03Z

Hah, I can do that, then. I didn't realize that this wasn't going through an actual apiserver in the test.

mattmoor · 2022-10-16T22:36:46Z

I definitely didn't want to bring in an API server as a requirement for unit testing, that seems rough 😅

…ecure-defaults-for-3-years

codecov · 2022-12-17T17:29:33Z

Codecov Report

Base: 86.43% // Head: 86.17% // Decreases project coverage by -0.26% ⚠️

Coverage data is based on head (749cf68) compared to base (35abde0).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13399      +/-   ##
==========================================
- Coverage   86.43%   86.17%   -0.26%     
==========================================
  Files         196      197       +1     
  Lines       14600    14723     +123     
==========================================
+ Hits        12619    12688      +69     
- Misses       1680     1733      +53     
- Partials      301      302       +1

Impacted Files	Coverage Δ
pkg/apis/serving/k8s_validation.go	`94.58% <100.00%> (+0.37%)`	⬆️
pkg/autoscaler/statforwarder/processor.go	`88.00% <0.00%> (-6.00%)`	⬇️
pkg/autoscaler/statforwarder/forwarder.go	`90.66% <0.00%> (-5.34%)`	⬇️
cmd/autoscaler/main.go	`9.47% <0.00%> (-0.42%)`	⬇️
pkg/apis/serving/v1/service_defaults.go	`100.00% <0.00%> (ø)`
pkg/apis/serving/v1/configuration_defaults.go	`100.00% <0.00%> (ø)`
pkg/reconciler/route/resources/certificate.go	`100.00% <0.00%> (ø)`
.../reconciler/domainmapping/resources/certificate.go	`100.00% <0.00%> (ø)`
cmd/autoscaler/leaderelection.go	`0.00% <0.00%> (ø)`
pkg/reconciler/route/route.go	`80.30% <0.00%> (+0.24%)`	⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…://github.com/evankanderson/serving into warning-warning-insecure-defaults-for-3-years

evankanderson

/assign @psschwei @dprotaso

I think this is ready for review, I'm going to get #13398 cleaned up over the next day or so to make the feature flag available (default off) to improve these security defaults for April.

evankanderson · 2022-12-30T23:26:29Z

/retest

psschwei

other than a nit on the comment, LGTM

psschwei · 2023-01-19T18:42:02Z

pkg/apis/serving/k8s_validation.go

+// warnDefaultContainerSecurityContext warns about Kubernetes default
+// SecurityContext values which are insecure (i.e. the "restricted" profile
+// forbids these values). Because securityContext values may also be set at
+// the Pod level, the container-level settings need to be considered alongside
+// the Pod-level settings.


nit: the release note is a little clearer in that the warning will sound when these values are unset. would be helpful to include that detail in the comment here, maybe something like:

Suggested change

// warnDefaultContainerSecurityContext warns about Kubernetes default

// SecurityContext values which are insecure (i.e. the "restricted" profile

// forbids these values). Because securityContext values may also be set at

// the Pod level, the container-level settings need to be considered alongside

// the Pod-level settings.

// warnDefaultContainerSecurityContext warns about Kubernetes default

// SecurityContext values which are unset and thus insecure (i.e. the "restricted" profile

// forbids these values). Because securityContext values may also be set at

// the Pod level, the container-level settings need to be considered alongside

// the Pod-level settings.

Done (and rewrapped, just 'cuz)

dprotaso · 2023-01-19T19:55:09Z

pkg/apis/serving/k8s_validation.go

+	for i := range ps.Containers {
+		errs = errs.Also(
+			warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.Containers[i].SecurityContext).
+				ViaFieldIndex("containers", 1).ViaField("securityContext"))


I think 1 should be i

Suggested change

ViaFieldIndex("containers", 1).ViaField("securityContext"))

ViaFieldIndex("containers", i).ViaField("securityContext"))

Ugh, I think I might have let AutoPilot write these... that's a crazy mistake.

Thanks!

ViaField and ViaFieldIndex were backwards here, too, which I noticed in the test output when I was fixing the [1] in the tests.

dprotaso · 2023-01-19T19:55:32Z

pkg/apis/serving/k8s_validation.go

+	for i := range ps.InitContainers {
+		errs = errs.Also(
+			warnDefaultContainerSecurityContext(ctx, ps.SecurityContext, ps.InitContainers[i].SecurityContext).
+				ViaFieldIndex("initContainers", 1).ViaField("securityContext"))


Likewise 1 should be i

Suggested change

ViaFieldIndex("initContainers", 1).ViaField("securityContext"))

ViaFieldIndex("initContainers", i).ViaField("securityContext"))

dprotaso · 2023-01-19T20:03:19Z

pkg/apis/serving/k8s_validation_test.go

+		ps       corev1.PodSpec
+		cfgOpts  []configOption
+		want     *apis.FieldError
+		errLevel apis.DiagnosticLevel


question: is the default value (0) == apis.ErrorLevel

looks like it is - https://github.com/knative/pkg/blob/247510c00e9dbb7f5a23b76e8ab76654de173a04/apis/field_error.go#L38

I think it would be more clear in the tests that don't assert warnings to just filter in the t.Run instead of adding the additional option to the test case

got = got.Filter(apis.ErrorLevel)

Yes, the default value is error for compatibility with all the existing code.

I think I was trying to be consistent here, but I can roll all these back if you prefer.

dprotaso · 2023-01-19T20:14:40Z

pkg/apis/serving/k8s_validation.go

+		errs = errs.Also(insecureDefault("capabilities"))
+	} else {
+		if sc.Capabilities.Drop == nil {
+			errs = errs.Also(insecureDefault("capabilities.drop"))


You might want to add a warning that all needs to be ALL - or error out on this even

Knative made this mistake and it was only recently fixed - #13327

Added a warning if arg 0 is "all", I think that should be sufficient for now.

evankanderson · 2023-01-20T00:17:17Z

(Still rolling back the validation_test changes, but wanted to get other bits pushed.)

evankanderson · 2023-01-20T00:37:34Z

Okay, this should be ready for another review.

dprotaso · 2023-01-20T15:46:29Z

/lgtm

Add validation to warn about insecure SecurityContext defaults

2fea408

knative-prow bot added area/API API objects and controllers size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2022

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2022

evankanderson mentioned this pull request Oct 16, 2022

Add secure-pod-defaults flag to default Pods to 'restricted' profile by default #13398

Merged

Ignore warning-level errors on existing tests -- better than filling …

82ed0c6

…default SecurityContexts dozens of times

This was referenced Oct 17, 2022

Filter reactor FieldErrors on validation to only error-level alerts. knative/pkg#2611

Merged

Run queue proxy with restricted profile #13376

Merged

evankanderson added 2 commits December 17, 2022 09:04

Merge remote-tracking branch 'upstream/main' into warning-warning-ins…

38d9357

…ecure-defaults-for-3-years

Merge remote-tracking branch 'upstream/main' into warning-warning-ins…

e1fe57e

…ecure-defaults-for-3-years

evankanderson added 2 commits December 17, 2022 09:37

Merge branch 'warning-warning-insecure-defaults-for-3-years' of https…

07df34c

…://github.com/evankanderson/serving into warning-warning-insecure-defaults-for-3-years

Fix warning on unused ctx param

87c67e6

evankanderson commented Dec 30, 2022

View reviewed changes

knative-prow bot assigned dprotaso and psschwei Dec 30, 2022

psschwei reviewed Jan 19, 2023

View reviewed changes

dprotaso reviewed Jan 19, 2023

View reviewed changes

Address psschwei and dprosato feedback

6a69932

Remove overzealous error parameterization, fix tests

749cf68

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2023

knative-prow bot merged commit e05aa3a into knative:main Jan 20, 2023

	ViaFieldIndex("containers", 1).ViaField("securityContext"))
	ViaFieldIndex("containers", i).ViaField("securityContext"))

	ViaFieldIndex("initContainers", 1).ViaField("securityContext"))
	ViaFieldIndex("initContainers", i).ViaField("securityContext"))

Add validation to warn about insecure SecurityContext defaults #13399

Add validation to warn about insecure SecurityContext defaults #13399

Uh oh!

Conversation

evankanderson commented Oct 16, 2022

Proposed Changes

Uh oh!

knative-prow bot commented Oct 16, 2022

Uh oh!

evankanderson commented Oct 16, 2022

Uh oh!

evankanderson commented Oct 16, 2022

Uh oh!

mattmoor commented Oct 16, 2022

Uh oh!

mattmoor commented Oct 16, 2022

Uh oh!

evankanderson commented Oct 16, 2022

Uh oh!

mattmoor commented Oct 16, 2022

Uh oh!

codecov bot commented Dec 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

evankanderson commented Dec 30, 2022

Uh oh!

psschwei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprotaso Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson commented Jan 20, 2023

Uh oh!

evankanderson commented Jan 20, 2023

Uh oh!

dprotaso commented Jan 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Dec 17, 2022 •

edited

Loading

dprotaso Jan 19, 2023 •

edited

Loading