Skip to content

Conversation

@skonto
Copy link

@skonto skonto commented Dec 6, 2022

In pod 3scale-kourier-gateway-56bfb7497f-lfjzm
    sh-4.4$  ./amicontained
    Container Runtime: kube
    Has Namespaces:
            pid: true
            user: false
    AppArmor Profile: system_u:system_r:container_t:s0:c9,c27
    Capabilities:
    Seccomp: filtering
    Blocked Syscalls (45):
            MSGRCV SYSLOG SETUID SETSID SETREUID SETGROUPS SETRESUID USELIB USTAT SYSFS VHANGUP PIVOT_ROOT CHROOT ACCT SETTIMEOFDAY UMOUNT2 SWAPON SWAPOFF REBOOT SETHOSTNAME SETDOMAINNAME IOPL IOPERM INIT_MODULE DELETE_MODULE QUERY_MODULE QUOTACTL NFSSERVCTL LOOKUP_DCOOKIE CLOCK_SETTIME KEXEC_LOAD MIGRATE_PAGES FUTIMESAT VMSPLICE MOVE_PAGES UTIMENSAT PE

This means TestShouldRunAsUserContainerDefault will not work. Tried it with the ksvc here.
The downside here is that we still see the warning in the controller (other fields are fine) :

{"severity":"WARNING","timestamp":"2022-12-05T22:08:11.415195138Z","logger":"controller","caller":"logging/warning_handler.go:32","message":"API Warning: would violate PodSecurity \"restricted:v1.24\": seccompProfile (pod or containers \"user-container\", \"queue-proxy\" must set securityContext.seccompProfile.type to \"RuntimeDefault\" or \"Localhost\")","commit":"8835ff5","knative.dev/pod":"controller-6ff988dcc4-txvz8"}

The warning seems weird (invalid?) because it is reported here that:

V2 requires you to either leave SeccompProfile empty or set it to runtime/default
** Empty is compatible with v1 and works on OCP versions < 4.11

For this PR using runAsUser works as expected by setting:
oc adm policy add-scc-to-user anyuid -z default -n {ns} if there is no label pod-security.kubernetes.io/enforce=restricted. Tested with ksvc and by executin id in pod's containers.

If such a namespace label exists then user needs to set: oc adm policy add-scc-to-user nonroot-v2 -z default -n {ns}

  • In case we choose to eliminate all warnings by setting seccompProfile too by default we will have somehow to label a revision with a special key/pair to be able to skip it in scenarios like TestShouldRunAsUserContainerDefault.
    That would deviate from unfinished upstream work and also would require docs for that case.
    However even if we set all fields we still get a different warning because seccompProfile is not available in crds:
{"severity":"WARNING","timestamp":"2022-12-05T14:54:55.718187034Z","logger":"controller","caller":"logging/warning_handler.go:32","message":"API Warning: unknown field \"spec.template.spec.containers[0].securityContext.seccompProfile\"","commit":"8835ff5","knative.dev/pod":"controller-c98d8498c-schv6"}
  • I also tried unconfined or "" for the seccompProfile as defined here (trying to figure out what empty means and it was supposed to be turned into runtime/default on OCP but it does not work or I am missing something):
LAST SEEN   TYPE      REASON            OBJECT                         MESSAGE
3s          Warning   InternalError     revision/helloworld-go-00001   failed to create deployment "helloworld-go-00001-deployment": Deployment.apps "helloworld-go-00001-deployment" is invalid: [spec.template.spec.containers[0].securityContext.seccompProfile.type: Required value: type is required when seccompProfile is set, spec.template.spec.containers[1].securityContext.seccompProfile.type: Required value: type is required when seccompProfile is set]

@openshift-ci
Copy link

openshift-ci bot commented Dec 6, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skonto

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:

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

@openshift-ci openshift-ci bot added the approved label Dec 6, 2022
@skonto
Copy link
Author

skonto commented Dec 6, 2022

@mgencur @maschmid let's discuss options here as listed in the description. WDYTH?

@skonto
Copy link
Author

skonto commented Dec 6, 2022

/assign @mgencur @maschmid

@mgencur
Copy link

mgencur commented Dec 6, 2022

@skonto Thanks for the detailed analysis. If I understand it correctly setting the defaults as currently implemented in this PR will still produce a warning. It is slightly different warning but still it's there.
I'm wondering if we could set the SeccompProfile by default in this PR and when the user needs to have functionality from TestShouldRunAsUserContainerDefault they could override the SecompProfile by configuring the Knative Service. I know it still requires some configuration from the user but the use case is more narrow. I.e. most users would not need to do anything and wouldn't get warnings but users that need to run as container default will need to put a short config by themselves. This would have to be documented.
Later we could probably converge to the upstream solution. WDYT?

@skonto
Copy link
Author

skonto commented Dec 6, 2022

@mgencur

I'm wondering if we could set the SeccompProfile by default in this PR and when the user needs to have functionality from TestShouldRunAsUserContainerDefault they could override the SecompProfile by configuring the Knative Service

Container sc seccompProfile is not configurable at all. User can only set seccompProfile at the podsecurity sc.
That requires to enable the feature kubernetes.podspec-securitycontex. Even in that case the container's security context (sc) takes precedence. The upstream code sets the seccomProfile by default only if

 psc.SeccompProfile == nil || psc.SeccompProfile.Type == "" 

TestShouldRunAsUserContainerDefault requires not to set the seccompProfile. Not sure we can distinguish between a normal ksvc and one that has anyuid set for its sa that is why I proposed to have a label.
Btw you can't use "" to signal a special case because it is not acceptable as a value:

**4s          Warning   InternalError     revision/helloworld-go-00001   failed to create deployment "helloworld-go-00001-deployment": Deployment.apps "helloworld-go-00001-deployment" is invalid: spec.template.spec.securityContext.seccompProfile.type: Required value: type is required when seccompProfile is set
**

@skonto skonto changed the title [WIP] [RELEASE-1.6][SRVKS-934] Add revision security defaults [WIP] [RELEASE-1.6][SRVKS-985] Add revision security defaults Jan 11, 2023
@skonto
Copy link
Author

skonto commented Jan 19, 2023

closing in favor of #132

@skonto skonto closed this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants