Extend SCC tests with their corresponding service account variants #27491

ingvagabund · 2022-10-24T18:14:11Z

Duplicate existing TestPodUpdateSCCEnforcement and TestAllowedSCCViaRBAC e2e tests with their corresponding service account equivalents to drop api group dependency on apigroup:user.openshift.io and apigroup:authorization.openshift.io.

stlaz · 2022-10-25T11:18:21Z

test/extended/security/scc.go

+	g.It("TestPodUpdateSCCEnforcement with service account", func() {
+		t := g.GinkgoT()
+
+		clusterAdminKubeClientset := oc.AdminKubeClient()
+
+		projectName := oc.Namespace()
+
+		sa := createServiceAccount(ctx, oc, projectName)
+		createPodAdminRoleOrDie(ctx, oc, sa)
+		restrictedClient, _ := createClientFromServiceAccount(oc, sa)
+
+		// so cluster-admin can create privileged pods, but harold cannot.  This means that harold should not be able
+		// to update the privileged pods either, even if he lies about its privileged nature
+		privilegedPod := getPrivilegedPod("unsafe")
+
+		if _, err := restrictedClient.CoreV1().Pods(projectName).Create(ctx, privilegedPod, metav1.CreateOptions{}); !isForbiddenBySCC(err) {
+			t.Fatalf("missing forbidden: %v", err)
+		}
+
+		actualPod, err := clusterAdminKubeClientset.CoreV1().Pods(projectName).Create(ctx, privilegedPod, metav1.CreateOptions{})
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+
+		actualPod.Spec.Containers[0].Image = "something-nefarious"
+		if _, err := restrictedClient.CoreV1().Pods(projectName).Update(ctx, actualPod, metav1.UpdateOptions{}); !isForbiddenBySCC(err) {
+			t.Fatalf("missing forbidden: %v", err)
+		}
+
+		// try to connect to /exec subresource as harold
+		restrictedClientCorev1Rest := restrictedClient.CoreV1().RESTClient()
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		result := &metav1.Status{}
+		err = restrictedClientCorev1Rest.Post().
+			Resource("pods").
+			Namespace(projectName).
+			Name(actualPod.Name).
+			SubResource("exec").
+			Param("container", "first").
+			Do(ctx).
+			Into(result)
+		if !isForbiddenBySCCExecRestrictions(err) {
+			t.Fatalf("missing forbidden by SCCExecRestrictions: %v", err)
+		}
+
+		// try to lie about the privileged nature
+		actualPod.Spec.HostPID = false
+		if _, err := restrictedClient.CoreV1().Pods(projectName).Update(context.Background(), actualPod, metav1.UpdateOptions{}); err == nil {
+			t.Fatalf("missing error: %v", err)
+		}
+	})


This looks like a copy-pasta of the above. Could you just derive the variables (I believe it's just the restConfig for the user?) from the code and turn this into a function that's run from the two tests?

ingvagabund · 2022-10-25T16:18:11Z

/retest-required

ingvagabund · 2022-10-26T09:47:32Z

/retest-required

test/extended/security/scc.go

stlaz · 2022-10-26T11:25:05Z

test/extended/security/scc.go

+}
+
+func createPodsecuritypolicyselfsubjectreviewsRoleBindingOrDie(ctx context.Context, oc *exutil.CLI, sa *corev1.ServiceAccount) {
+	framework.Logf("Creating pspssr role")


creating what?

createPodSecurityPolicySelfSubjectReviewsRoleBindingOrDie :D

my point is - please expand the shortcut in the log, otherwise people will hate you whenever they will be trying to understand what's going on in here :)

stlaz · 2022-10-27T10:43:42Z

/lgtm

mfojtik · 2022-10-27T14:52:06Z

/aprove

mfojtik · 2022-10-27T15:06:01Z

/approve

soltysh

/approve

openshift-ci · 2022-10-27T15:11:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund, mfojtik, soltysh, stlaz

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:

~~test/OWNERS~~ [mfojtik,soltysh]

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

openshift-ci · 2022-10-27T21:00:30Z

@ingvagabund: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-csi	`8304466`	link	false	`/test e2e-gcp-csi`
ci/prow/e2e-metal-ipi-ovn-ipv6	`8304466`	link	false	`/test e2e-metal-ipi-ovn-ipv6`
ci/prow/e2e-aws-ovn-single-node	`8304466`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-aws-ovn-single-node-upgrade	`8304466`	link	false	`/test e2e-aws-ovn-single-node-upgrade`
ci/prow/e2e-agnostic-ovn-cmd	`8304466`	link	false	`/test e2e-agnostic-ovn-cmd`
ci/prow/e2e-gcp-ovn-rt-upgrade	`8304466`	link	false	`/test e2e-gcp-ovn-rt-upgrade`
ci/prow/e2e-openstack-ovn	`8304466`	link	false	`/test e2e-openstack-ovn`
ci/prow/e2e-aws-csi	`8304466`	link	false	`/test e2e-aws-csi`
ci/prow/e2e-metal-ipi-sdn	`8304466`	link	false	`/test e2e-metal-ipi-sdn`
ci/prow/e2e-aws-ovn-single-node-serial	`8304466`	link	false	`/test e2e-aws-ovn-single-node-serial`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

ingvagabund · 2022-10-28T08:16:16Z

/retest-required

…c-with-service-account" This reverts commit 519b7eb, reversing changes made to 86a216a.

…irror-scc-with-service-account"" This reverts commit 6a8ff38.

…c-with-service-account" This reverts commit 519b7eb, reversing changes made to 86a216a.

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2022

openshift-ci bot requested review from mandre and mrogers950 October 24, 2022 18:15

ingvagabund force-pushed the mirror-scc-with-service-account branch from 9f9f09b to e103e1f Compare October 25, 2022 09:31

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 25, 2022

stlaz suggested changes Oct 25, 2022

View reviewed changes

ingvagabund force-pushed the mirror-scc-with-service-account branch from e103e1f to 94e1dc1 Compare October 25, 2022 11:38

ingvagabund force-pushed the mirror-scc-with-service-account branch from 94e1dc1 to 77ca6e1 Compare October 26, 2022 09:08

stlaz suggested changes Oct 26, 2022

View reviewed changes

ingvagabund force-pushed the mirror-scc-with-service-account branch 2 times, most recently from d04fe0d to f9aa361 Compare October 27, 2022 10:43

Extend SCC tests with their corresponding service account variants

8304466

ingvagabund force-pushed the mirror-scc-with-service-account branch from f9aa361 to 8304466 Compare October 27, 2022 10:44

openshift-ci bot assigned stlaz Oct 27, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2022

soltysh approved these changes Oct 27, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2022

openshift-merge-robot merged commit 519b7eb into openshift:master Oct 28, 2022

stbenjam added a commit to stbenjam/origin that referenced this pull request Oct 28, 2022

Revert "Merge pull request openshift#27491 from ingvagabund/mirror-sc…

6a8ff38

…c-with-service-account" This reverts commit 519b7eb, reversing changes made to 86a216a.

stbenjam mentioned this pull request Oct 28, 2022

Revert 1.25 again #27499

Merged

ingvagabund deleted the mirror-scc-with-service-account branch October 30, 2022 17:05

bparees added a commit to bparees/origin that referenced this pull request Nov 1, 2022

Revert "Revert "Merge pull request openshift#27491 from ingvagabund/m…

ac98b0a

…irror-scc-with-service-account"" This reverts commit 6a8ff38.

bparees added a commit to bparees/origin that referenced this pull request Nov 1, 2022

Revert "Revert "Merge pull request openshift#27491 from ingvagabund/m…

7f1ed15

…irror-scc-with-service-account"" This reverts commit 6a8ff38.

bparees added a commit to bparees/origin that referenced this pull request Nov 1, 2022

Revert "Revert "Merge pull request openshift#27491 from ingvagabund/m…

7eb2ea9

…irror-scc-with-service-account"" This reverts commit 6a8ff38.

tjungblu pushed a commit to tjungblu/origin that referenced this pull request Apr 11, 2023

Revert "Merge pull request openshift#27491 from ingvagabund/mirror-sc…

7e632a4

…c-with-service-account" This reverts commit 519b7eb, reversing changes made to 86a216a.

Extend SCC tests with their corresponding service account variants #27491

Extend SCC tests with their corresponding service account variants #27491

Uh oh!

Conversation

ingvagabund commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stlaz Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

ingvagabund commented Oct 25, 2022

Uh oh!

ingvagabund commented Oct 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stlaz Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

ingvagabund Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

stlaz Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

stlaz commented Oct 27, 2022

Uh oh!

mfojtik commented Oct 27, 2022

Uh oh!

mfojtik commented Oct 27, 2022

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Oct 27, 2022

Uh oh!

openshift-ci bot commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ingvagabund commented Oct 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ingvagabund commented Oct 24, 2022 •

edited

Loading

openshift-ci bot commented Oct 27, 2022 •

edited

Loading