Skip to content

Conversation

@KauzClay
Copy link
Contributor

@KauzClay KauzClay commented Sep 13, 2023

Fixes #14365

Related to:

Proposed Changes

  • Add seccompProfile type to queue proxy security context.
  • Add runAsNonRoot to secure-pod-defaults
  • I believe this PR added much of the security context sections. But when I install a Knative Service on a cluster that enforces Pod Security Standards, and I enabled secure-pod-defaults, I still get errors.
 status:
  conditions:
  - lastTransitionTime: "2023-09-13T15:13:42Z"
    lastUpdateTime: "2023-09-13T15:13:42Z"
    message: Created new replica set "tomato-00001-deployment-5db9b9f88d"
    reason: NewReplicaSetCreated
    status: "True"
    type: Progressing
  - lastTransitionTime: "2023-09-13T15:13:42Z"
    lastUpdateTime: "2023-09-13T15:13:42Z"
    message: Deployment does not have minimum availability.
    reason: MinimumReplicasUnavailable
    status: "False"
    type: Available
  - lastTransitionTime: "2023-09-13T15:13:42Z"
    lastUpdateTime: "2023-09-13T15:13:42Z"
    message: 'admission webhook "pod-security-webhook.kubernetes.io" denied the request:
      pods "tomato-00001-deployment-5db9b9f88d-w8gd4" is forbidden: violates PodSecurity
      "restricted:latest": runAsNonRoot != true (pod or container "user-container"
      must set securityContext.runAsNonRoot=true), seccompProfile (pod or container
      "queue-proxy" must set securityContext.seccompProfile.type to "RuntimeDefault"
      or "Localhost")'
    reason: FailedCreate
    status: "True"
    type: ReplicaFailure

Release Note

Fix secure 'secure-pod-defaults' to work with restricted namespaces

@knative-prow
Copy link

knative-prow bot commented Sep 13, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2023
@knative-prow knative-prow bot requested review from kauana and krsna-m September 13, 2023 15:16
@knative-prow knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/API API objects and controllers labels Sep 13, 2023
@KauzClay KauzClay changed the title fix security Context for Knative Service Pod (user-container and queue-proxy) fix securityContext for Knative Service Pod (user-container and queue-proxy) Sep 13, 2023
@KauzClay KauzClay force-pushed the ck-add-seccompprofile branch from 11c7925 to 58a7ebc Compare September 13, 2023 16:59
@dprotaso
Copy link
Member

To confirm - Does the changes in this PR imply that SecurePodDefaults doesn't work? Since the queue proxy and RevisionSpec are missing those explicit settings?

@KauzClay KauzClay force-pushed the ck-add-seccompprofile branch from 58a7ebc to 72613d2 Compare September 13, 2023 20:02
@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 13, 2023
@KauzClay
Copy link
Contributor Author

To confirm - Does the changes in this PR imply that SecurePodDefaults doesn't work? Since the queue proxy and RevisionSpec are missing those explicit settings?

yeah, that is what I'm seeing. This issue (#14365) has an example of the errors I see on my deployment

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (4dddf9f) 86.12% compared to head (2cf6abf) 86.15%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14363      +/-   ##
==========================================
+ Coverage   86.12%   86.15%   +0.02%     
==========================================
  Files         196      196              
  Lines       14787    14790       +3     
==========================================
+ Hits        12735    12742       +7     
+ Misses       1744     1743       -1     
+ Partials      308      305       -3     
Files Changed Coverage Δ
pkg/reconciler/revision/resources/queue.go 98.42% <ø> (ø)
pkg/apis/serving/v1/revision_defaults.go 97.48% <100.00%> (+0.04%) ⬆️

... and 5 files with indirect coverage changes

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

@KauzClay KauzClay marked this pull request as ready for review September 13, 2023 21:10
@knative-prow 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 Sep 13, 2023
@knative-prow knative-prow bot requested review from jsanin-vmw and nak3 September 13, 2023 21:10
@dprotaso
Copy link
Member

/lgtm
/approve
/cherry-pick release-1.11

@knative-prow-robot
Copy link
Contributor

@dprotaso: once the present PR merges, I will cherry-pick it on top of release-1.11 in a new PR and assign it to you.

In response to this:

/lgtm
/approve
/cherry-pick release-1.11

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.

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 14, 2023
@knative-prow
Copy link

knative-prow bot commented Sep 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, KauzClay

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:

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2023
@dprotaso
Copy link
Member

/cherry-pick release-1.10

@knative-prow-robot
Copy link
Contributor

@dprotaso: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.10

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.

@knative-prow-robot
Copy link
Contributor

@dprotaso: new pull request created: #14377

In response to this:

/cherry-pick release-1.10

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.

@knative-prow-robot
Copy link
Contributor

@dprotaso: new pull request created: #14378

In response to this:

/lgtm
/approve
/cherry-pick release-1.11

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.

@BobyMCbobs
Copy link
Member

Woohoo! This is a good change

@dprotaso
Copy link
Member

Surprisingly it means no one has used the secure-pod-defaults feature on a restricted profile :/

krsna-m pushed a commit to krsna-m/serving that referenced this pull request Nov 6, 2023
…-proxy) (knative#14363)

* add seccompProfile to queue container security context

* run as non root by default

* update tests to expect new default run as nonroot
wmfgerrit pushed a commit to wikimedia/operations-docker-images-production-images that referenced this pull request Feb 27, 2025
The pull request is needed to force Knative to set runAsNonRoot
when secure-pod-defaults is true. The option forces knative to
create the user pods with all the safest security options to pass
the PSS restriction policy, but at the moment it is lacking
runAsNonRoot forced to true.

Note: I haven't backported the whole patch since part of it overlapped
with a previous one for queue.go, where seccomp's defaults were
set. The same code was committed in different pull requests, so I just
removed the bit already there to allow patch to avoid error/warnings.

Bug: T369493
Change-Id: Iceb1ac2d83f298ef2a834e24a8fdc8a6f1df4a28
wmfgerrit pushed a commit to wikimedia/operations-docker-images-production-images that referenced this pull request Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

secure-pod-defaults does not work when enforcing restricted Pod Security Standards

4 participants