[RELEASE-1.7][SRVKS-985] Add revision security defaults #186

skonto · 2023-02-28T08:30:30Z

Adapted/backported from Add secure-pod-defaults flag to default Pods to 'restricted' profile by default knative/serving#13398
We will enable the feature by default at the S-O side on OCP 4.11+ as we did with deprecated apis.
The feature flag is: secure-pod-defaults.
Tested on 4.13 for audit warnings. To test follow the process here (you need the latest oc version to avoid failures):

oc adm must-gather -- /usr/bin/gather_audit_logs
cd must-gather.local....
gunzip -r *
find ./ -type f -exec grep -l "violation" {} \; | xargs cat | grep violation

Introduces "serving.knative.openshift.io/skipSeccompProfile" as a revision annotation that allows to skip setting seccomProfile for a service so it can run with the default image user. Since we support versions < 4.11 SeccompProfile is only set by default in versions > 4.10.

skonto · 2023-02-28T08:32:29Z

/hold

skonto · 2023-02-28T11:08:00Z

Tests passed. Must gather failed:

  * could not run steps: step e2e-aws-ocp-411 failed: "e2e-aws-ocp-411" post steps failed: "e2e-aws-ocp-411" pod "e2e-aws-ocp-411-gather-audit-logs" failed: the pod ci-op-ctmrhy2b/e2e-aws-ocp-411-gather-audit-logs failed after 5s (failed containers: place-entrypoint): ContainerFailed one or more containers exited
  * ```

matzew · 2023-02-28T13:45:05Z

pkg/apis/config/features.go

 		PodSpecInitContainers:            Disabled,
 		PodSpecDNSPolicy:                 Disabled,
 		PodSpecDNSConfig:                 Disabled,
+		SecurePodDefaults:                Disabled,


isn't this something we can do upstream?

This is done upstream. There is a link in the description. The PR here is a backport of the upstream work.

matzew · 2023-02-28T13:46:29Z

pkg/apis/serving/fieldmask.go

+	if config.FromContextOrDefaults(ctx).Features.SecurePodDefaults == config.Enabled {
+		// Allow to opt out of more-secure defaults if SecurePodDefaults is enabled.
+		// This aligns with defaultSecurityContext in revision_defaults.go.
+		if in.SeccompProfile != nil {


We need it here, since serving is creating/owing those, and not the serverless operator?

Yes this is for knative services. Eventing might need this for some user workloads too.

matzew · 2023-02-28T13:48:27Z

In serverless operator will the feature be enabled, via code, when a certain platform match was detected, @skonto ?

skonto · 2023-02-28T13:54:44Z

In serverless operator will the feature be enabled, via code, when a certain platform match was detected, @skonto ?

Correct here is the PR at the S-O that I am using for testing:openshift-knative/serverless-operator#1862.

skonto · 2023-02-28T13:57:25Z

/test 49-e2e-aws-ocp-49

matzew · 2023-02-28T16:38:11Z

vendor/knative.dev/pkg/injection/sharedmain/main.go

 	CheckK8sClientMinimumVersionOrDie(ctx, logger)
+	// HACK: should go away when we move away from < 4.11 releases
+	if err := CheckMinimumKubeVersion(kubeclient.Get(ctx).Discovery(), "1.24.0"); err == nil {
+		os.Setenv("OCP_SECCOMP_PROFILE_WITHOUT_SCC", "true")


is that our env-var? If so, I'd prefix it slightly different, to not confuse w/ actual OCP bits.

This is internal not meant to be exposed.

matzew · 2023-02-28T16:38:45Z

vendor/knative.dev/pkg/injection/sharedmain/main.go

+
+// CheckMinimumKubeVersion checks if current K8s version we are on is higher than the one passed.
+// If an error is returned then the version is not higher than the minimum
+func CheckMinimumKubeVersion(versioner discovery.ServerVersionInterface, version string) error {


why adding the func on this file?

the pkg/version has already the normalizeVersion func

Does not do what we want so we adapted it, we also have the same at the S-O side.

matzew · 2023-02-28T16:39:22Z

vendor/knative.dev/pkg/injection/sharedmain/main.go

+		return err
+	}
+
+	minimumVersion, err := semver.Make(normalizeVersion(version))


we could (for us) just hard-code here against 1.24, instead of passing in a version?

We could but it is not much different.

matzew · 2023-03-01T08:13:25Z

pkg/apis/serving/v1/revision_defaults.go

+		updatedSC.AllowPrivilegeEscalation = ptr.Bool(false)
+	}
+
+	if _, ok := os.LookupEnv("OCP_SECCOMP_PROFILE_WITHOUT_SCC"); ok && !skipSeccompProfile(ctx) { // Only apply the profile in 4.11+


@skonto in 4.11 we only get warnings if the SecurityContext values are missing, right?

Yes if they are missing you get warnings in audit logs. We also got warnings in the controller logs since we do k8s api calls to create deployments.

skonto · 2023-03-02T07:58:34Z

/test 48-e2e-aws-ocp-48
/test 412-e2e-aws-ocp-412

skonto · 2023-03-02T12:18:20Z

/assign @ReToCode

/hold not sure if we are going to merge it, check discussion on the related jira.

skonto · 2023-03-02T12:19:20Z

/assign @mgencur

ReToCode

👍

openshift/e2e-common.sh

ReToCode · 2023-03-02T13:02:07Z

openshift/e2e-common.sh

  # Apply persistent volume claim needed, needed for the related e2e test.
  oc apply -f ./test/config/pvc/pvc.yaml

-  oc adm policy add-scc-to-user privileged -z default -n serving-tests


We merge that change, even if we do not merge the whole thing.

vendor/knative.dev/pkg/injection/sharedmain/main.go

openshift-ci · 2023-03-02T13:09:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ReToCode, 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:

~~OWNERS~~ [skonto]

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

ReToCode · 2023-03-06T08:33:12Z

Closing in favour of #194, but we are keeping the changes around in case we need it later.

openshift-ci bot requested review from alanfx and mgencur February 28, 2023 08:30

openshift-ci bot added the approved label Feb 28, 2023

openshift-ci bot added the do-not-merge/hold label Feb 28, 2023

skonto force-pushed the add_sec_default_1.7 branch from 2e2cb4c to c3b229e Compare February 28, 2023 12:03

matzew reviewed Feb 28, 2023

View reviewed changes

skonto mentioned this pull request Feb 28, 2023

[WIP][SRVKS-934] Enable pod security defaults openshift-knative/serverless-operator#1862

Closed

matzew reviewed Mar 1, 2023

View reviewed changes

skonto added 4 commits March 2, 2023 14:01

add revision security defaults

300ec89

restore ci yaml

b1b5e17

remove privileged for most tests

f64a215

rebase and cleanup

b0bfd4d

skonto force-pushed the add_sec_default_1.7 branch from 105d6f8 to b0bfd4d Compare March 2, 2023 12:16

openshift-ci bot assigned ReToCode Mar 2, 2023

openshift-ci bot assigned mgencur Mar 2, 2023

ReToCode approved these changes Mar 2, 2023

View reviewed changes

skonto mentioned this pull request Mar 3, 2023

[release-v1.7] Unite release manifests #192

Closed

skonto mentioned this pull request Mar 3, 2023

[RELEASE-1.7][SRVKS-985] Add sec default no seccomp #194

Merged

ReToCode closed this Mar 6, 2023

[RELEASE-1.7][SRVKS-985] Add revision security defaults #186

[RELEASE-1.7][SRVKS-985] Add revision security defaults #186

Uh oh!

Conversation

skonto commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skonto commented Feb 28, 2023

Uh oh!

skonto commented Feb 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Feb 28, 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!

skonto Feb 28, 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!

matzew commented Feb 28, 2023

Uh oh!

skonto commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skonto commented Feb 28, 2023

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!

skonto Feb 28, 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!

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!

skonto commented Mar 2, 2023

Uh oh!

skonto commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skonto commented Mar 2, 2023

Uh oh!

ReToCode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci bot commented Mar 2, 2023

Uh oh!

ReToCode commented Mar 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skonto commented Feb 28, 2023 •

edited

Loading

skonto Feb 28, 2023 •

edited

Loading

skonto Feb 28, 2023 •

edited

Loading

skonto commented Feb 28, 2023 •

edited

Loading

skonto Feb 28, 2023 •

edited

Loading

skonto commented Mar 2, 2023 •

edited

Loading