Run queue proxy with restricted profile #13376

skonto · 2022-10-10T18:34:31Z

Fixes partially #13308

Proposed Changes

Allows to run a ksvc in a namespace with restricted profile enabled. Without this patch we get the following issue on 1.25 (see instructions next):

Warning FailedCreate replicaset/helloworld-go-00001-deployment-584759b77b (combined from similar events): Error creating: pods "helloworld-go-00001-deployment-584759b77b-tmxxd" is forbidden: violates PodSecurity "restricted:latest": allowPrivilegeEscalation != false (container "user-container" must set securityContext.allowPrivilegeEscalation=false), unrestricted capabilities (container "queue-proxy" must set securityContext.capabilities.drop=["ALL"]), seccompProfile (pod or containers "user-container", "queue-proxy" must set securityContext.seccompProfile.type to "RuntimeDefault" or "Localhost")

To test follow instructions here.
Discussed here.

Release Note

Queue proxy explicit set `SeccompProfile` to `RunTimeDefault` to be able to run under restricted PSP policy by default.

skonto · 2022-10-10T18:38:25Z

pkg/reconciler/revision/resources/deploy.go

@@ -230,6 +230,12 @@ func makeContainer(container corev1.Container, rev *v1.Revision) corev1.Containe
 	container.Lifecycle = userLifecycle
 	container.Env = append(container.Env, getKnativeEnvVar(rev)...)

+	// Set PSP requirements explicitly to avoid failures in case `pod-security.kubernetes.io/enforce=restricted` is used
+	// at the user workload namespace
+	container.SecurityContext.AllowPrivilegeEscalation = ptr.Bool(false)


We don't allow the user to set the following two properties. So we enforce the defaults here to make it pass the PSP auditing.

@mattmoor just made allowPrivilegeEscalation pass-through in #13395 , though we could force it to false as we do here instead.

I sent #13401 to allow setting this in the Revision Spec (e.g. in Service).

codecov · 2022-10-10T19:19:25Z

Codecov Report

Base: 86.47% // Head: 86.47% // No change to project coverage 👍

Coverage data is based on head (f3360cf) compared to base (6264c1b).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13376   +/-   ##
=======================================
  Coverage   86.47%   86.47%           
=======================================
  Files         196      196           
  Lines       14551    14551           
=======================================
  Hits        12583    12583           
  Misses       1669     1669           
  Partials      299      299

Impacted Files	Coverage Δ
pkg/reconciler/revision/resources/queue.go	`98.23% <ø> (ø)`

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.

psschwei

Couple of minor things, but otherwise this looks good to me:

could you add a release note?
this won't work with kourier (it doesn't set runAsNotRoot = true)... don't think that blocks us here, just wanted to make a note of it

skonto · 2022-10-13T08:58:56Z

this won't work with kourier (it doesn't set runAsNotRoot = true)... don't think that blocks us here, just wanted to make a note of it

If you mean the gateway yes. I will take a look to fix it. Downstream we already run without root.

skonto · 2022-10-13T12:40:14Z

@psschwei gentle ping, added the release note and created knative-extensions/net-kourier#934.

psschwei

/lgtm
/approve
/hold

to give @dprotaso a chance to weigh in

knative-prow · 2022-10-13T16:30:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: psschwei, skonto

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:

~~pkg/OWNERS~~ [psschwei]

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

evankanderson

I think we may want to pass-through the allowPrivilegeEscalation and seccompProfile fields, and then add a feature flag to default them to safe values if not provided.

I hadn't hit on this until the other day seeming @mattmoor 's #13395 , but I think I'd feel most happy long term with "didn't set anything" meaning secure, but allowing people to explicitly be insecure when needed (maybe with warnings)

evankanderson · 2022-10-16T04:21:39Z

pkg/reconciler/revision/resources/deploy.go

@@ -230,6 +230,12 @@ func makeContainer(container corev1.Container, rev *v1.Revision) corev1.Containe
 	container.Lifecycle = userLifecycle
 	container.Env = append(container.Env, getKnativeEnvVar(rev)...)

+	// Set PSP requirements explicitly to avoid failures in case `pod-security.kubernetes.io/enforce=restricted` is used
+	// at the user workload namespace
+	container.SecurityContext.AllowPrivilegeEscalation = ptr.Bool(false)


@mattmoor just made allowPrivilegeEscalation pass-through in #13395 , though we could force it to false as we do here instead.

skonto · 2022-10-17T09:25:36Z

@evankanderson I also mentioned on slack that allowing users to configure stuff on their own was more user friendly than having everything explicitly set without ability to change on demand. Also as you said users can choose their security level. For some properties though you can be opinionated and set explicitly the defaults instead of optionally applying them as in #13398 as long as users can remove them. This can be done, given our experience of what users need in practice eg. usually you dont want a service to escalate privileges. Another approach is to define security application as a downstream issue only. We have options depending on the strategy, do we want to be secure by default? My understanding is that this concept applies to other features like internal-encryption etc. Btw I am not sure if @mattmoor was aware of this PR or discussion. Should I close this PR and enable all stuff in #13398 or keep this PR with the queue proxy changes only?

(maybe with warnings)

You get the warning anyways from psp no need to add more imho.

skonto · 2022-10-17T11:10:01Z

pkg/reconciler/revision/resources/queue.go

-			Drop: []corev1.Capability{"all"},
+			Drop: []corev1.Capability{"ALL"},
+		},
+		SeccompProfile: &corev1.SeccompProfile{


I guess this takes precedence compared to podsecurityContext.SeccompProfile.

Yes, it does.

evankanderson · 2022-10-17T14:15:54Z

Can users see the PSP / PSA warnings when they apply a Knative Service? If so, then I'd agree about not needing a second set of warnings that settings are possibly dangerous.

In #13399, I tried to add a warning because our future behavior might change, which could break a small number of applications.

Upon reflection, I think giving the defaulting of security properties time to sit rather than rushing it for release is probably the right idea, particularly given the issues hit by #13399.

evankanderson · 2022-10-17T14:16:51Z

I'm still willing to approve the queue-proxy side of this, but I don't want to add a breaking behavioral change without some sort of "out" for users.

skonto · 2022-10-17T21:25:10Z

/test istio-latest-no-mesh

psschwei

FYI @skonto I updated the PR title / release note since these changes are just on QP now.
/lgtm

skonto · 2022-10-18T09:50:57Z

@psschwei thank you, ok to unhold?

psschwei · 2022-10-18T12:09:32Z

/hold cancel

* allow user workloads to run with restricted profile * only change queue proxy

* Run queue proxy with restricted profile (knative#13376) * allow user workloads to run with restricted profile * only change queue proxy * remove seccomp

…ve#1283) * Run queue proxy with restricted profile (knative#13376) * allow user workloads to run with restricted profile * only change queue proxy * remove seccomp

…ve#1283) (#13) * Run queue proxy with restricted profile (knative#13376) * allow user workloads to run with restricted profile * only change queue proxy * remove seccomp

…hift#1283) * Run queue proxy with restricted profile (knative#13376) * allow user workloads to run with restricted profile * only change queue proxy * remove seccomp

…ve#1283) (#19) * Run queue proxy with restricted profile (knative#13376) * allow user workloads to run with restricted profile * only change queue proxy * remove seccomp Co-authored-by: Stavros Kontopoulos <skontopo@redhat.com>

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/API API objects and controllers labels Oct 10, 2022

skonto commented Oct 10, 2022

View reviewed changes

skonto force-pushed the fix_psp_queue branch from 199ce6c to 6cf9ef1 Compare October 10, 2022 19:11

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 10, 2022

skonto changed the title ~~[wip] Allow user workloads to run with restricted profile~~ Allow user workloads to run with restricted profile Oct 11, 2022

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 11, 2022

psschwei reviewed Oct 12, 2022

View reviewed changes

psschwei reviewed Oct 13, 2022

View reviewed changes

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 13, 2022

knative-prow bot assigned psschwei Oct 13, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2022

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

psschwei mentioned this pull request Oct 15, 2022

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

Merged

evankanderson reviewed Oct 16, 2022

View reviewed changes

evankanderson mentioned this pull request Oct 16, 2022

Allow setting seccompProfile to enable using restricted security profile #13401

Merged

skonto commented Oct 17, 2022

View reviewed changes

skonto added 2 commits October 17, 2022 23:02

allow user workloads to run with restricted profile

d2304cc

only change queue proxy

f3360cf

skonto force-pushed the fix_psp_queue branch from 6cf9ef1 to f3360cf Compare October 17, 2022 20:03

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2022

knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 17, 2022

psschwei changed the title ~~Allow user workloads to run with restricted profile~~ Run queue proxy with restricted profile Oct 18, 2022

psschwei reviewed Oct 18, 2022

View reviewed changes

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2022

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2022

knative-prow bot merged commit 388128b into knative:main Oct 18, 2022

skonto added a commit to skonto/serving that referenced this pull request Oct 19, 2022

Run queue proxy with restricted profile (knative#13376)

705787a

* allow user workloads to run with restricted profile * only change queue proxy

daraghlowe mentioned this pull request Nov 15, 2022

Seccomp profile in queue-proxy incompatible with gvisor #13471

Closed

k4leung4 mentioned this pull request Dec 5, 2022

bump knative serving 0.35.1 chainguard-dev/hakn#52

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run queue proxy with restricted profile #13376

Run queue proxy with restricted profile #13376

skonto commented Oct 10, 2022 •

edited by psschwei

Loading

skonto Oct 10, 2022 •

edited

Loading

evankanderson Oct 16, 2022

evankanderson Oct 16, 2022

codecov bot commented Oct 10, 2022 •

edited

Loading

psschwei left a comment

skonto commented Oct 13, 2022 •

edited

Loading

skonto commented Oct 13, 2022 •

edited

Loading

psschwei left a comment

knative-prow bot commented Oct 13, 2022

evankanderson left a comment

evankanderson Oct 16, 2022

skonto commented Oct 17, 2022 •

edited

Loading

skonto Oct 17, 2022 •

edited

Loading

evankanderson Oct 17, 2022

evankanderson commented Oct 17, 2022

evankanderson commented Oct 17, 2022

skonto commented Oct 17, 2022

psschwei left a comment

skonto commented Oct 18, 2022

psschwei commented Oct 18, 2022

Run queue proxy with restricted profile #13376

Run queue proxy with restricted profile #13376

Conversation

skonto commented Oct 10, 2022 • edited by psschwei Loading

Proposed Changes

skonto Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

evankanderson Oct 16, 2022

Choose a reason for hiding this comment

evankanderson Oct 16, 2022

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2022 • edited Loading

Codecov Report

psschwei left a comment

Choose a reason for hiding this comment

skonto commented Oct 13, 2022 • edited Loading

skonto commented Oct 13, 2022 • edited Loading

psschwei left a comment

Choose a reason for hiding this comment

knative-prow bot commented Oct 13, 2022

evankanderson left a comment

Choose a reason for hiding this comment

evankanderson Oct 16, 2022

Choose a reason for hiding this comment

skonto commented Oct 17, 2022 • edited Loading

skonto Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

evankanderson Oct 17, 2022

Choose a reason for hiding this comment

evankanderson commented Oct 17, 2022

evankanderson commented Oct 17, 2022

skonto commented Oct 17, 2022

psschwei left a comment

Choose a reason for hiding this comment

skonto commented Oct 18, 2022

psschwei commented Oct 18, 2022

skonto commented Oct 10, 2022 •

edited by psschwei

Loading

skonto Oct 10, 2022 •

edited

Loading

codecov bot commented Oct 10, 2022 •

edited

Loading

skonto commented Oct 13, 2022 •

edited

Loading

skonto commented Oct 13, 2022 •

edited

Loading

skonto commented Oct 17, 2022 •

edited

Loading

skonto Oct 17, 2022 •

edited

Loading