OCPBUGS-33041: Add RoleBinding for BuildConfig Webhooks

Starting in OCP 4.16, the `system:webhook` ClusterRole will not be granted to anonymous users by default. This will break most systems that use BuildConfig webhooks to trigger builds, since many can't be add an OpenShift auth token to their HTTP headers (ex: GitHub). Only new installations will be impacted; upgrades to 4.16 will continue to support unauthenticated BuildConfig webhooks. This test update verifies that BuildConfig webhooks can be triggered using a namespace-scoped RoleBinding for the `system:unauthenticated` group. RoleBindings are preferable to ClusterRoleBindings as they limit unauthenticated API calls to specific namespaces, reducing the potential attack surface. The core webhook tests were also updated to verify that unauthenticated webhooks fail if this rolebinding is missing. Use of BuildConfig webhooks should be discouraged in favor of Pipelines as Code, which has more robust mechanisms for securing webhook calls from external systems. It also does not rely on an aggregated apiserver and associated RBAC. See also https://issues.redhat.com/browse/AUTH-509 Signed-off-by: Adam Kaplan <adam.kaplan@redhat.com>
openshift · Apr 30, 2024 · 0ae4678 · 0ae4678
1 parent 4e12068
commit 0ae4678
Show file tree

Hide file tree

Showing 4 changed files with 122 additions and 12 deletions.
diff --git a/test/extended/builds/start.go b/test/extended/builds/start.go
@@ -14,6 +14,7 @@ import (
 	o "github.com/onsi/gomega"
 
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
 	e2e "k8s.io/kubernetes/test/e2e/framework"
@@ -34,6 +35,7 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] starting a build using CL
 		exampleGemfile     = exutil.FixturePath("testdata", "builds", "test-build-app", "Gemfile")
 		exampleBuild       = exutil.FixturePath("testdata", "builds", "test-build-app")
 		symlinkFixture     = exutil.FixturePath("testdata", "builds", "test-symlink-build.yaml")
+		webhookRoleBinding = exutil.FixturePath("testdata", "builds", "webhook", "webhooks-unauth.yaml")
 		exampleGemfileURL  = "https://raw.githubusercontent.com/openshift/ruby-hello-world/master/Gemfile"
 		exampleArchiveURL  = "https://github.com/openshift/ruby-hello-world/archive/master.zip"
 		oc                 = exutil.NewCLIWithPodSecurityLevel("cli-start-build", admissionapi.LevelBaseline)
@@ -409,6 +411,20 @@ var _ = g.Describe("[sig-builds][Feature:Builds][Slow] starting a build using CL
 			})
 
 			g.Describe("start a build via a webhook", func() {
+
+				// AUTH-509: Webhooks do not allow unauthenticated requests by default.
+				// Create a role binding which allows unauthenticated webhooks.
+				g.BeforeEach(func() {
+					ctx := context.Background()
+					_, err := oc.AdminAuthorizationClient().AuthorizationV1().RoleBindings(oc.Namespace()).Get(ctx, "webooks-unauth", metav1.GetOptions{})
+					if apierrors.IsNotFound(err) {
+						err = oc.AsAdmin().Run("apply").Args("-f", webhookRoleBinding).Execute()
+						o.Expect(err).NotTo(o.HaveOccurred(), "creating webhook role binding")
+						return
+					}
+					o.Expect(err).NotTo(o.HaveOccurred(), "checking if webhook role binding exists")
+				})
+
 				g.It("should be able to start builds via the webhook with valid secrets and fail with invalid secrets [apigroup:build.openshift.io]", func() {
 					g.By("clearing existing builds")
 					_, err := oc.Run("delete").Args("builds", "--all").Output()

diff --git a/test/extended/builds/webhook.go b/test/extended/builds/webhook.go
@@ -3,6 +3,7 @@ package builds
 import (
 	"bytes"
 	"context"
+	"crypto/tls"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
@@ -25,7 +26,10 @@ import (
 
 var _ = g.Describe("[sig-builds][Feature:Builds][webhook]", func() {
 	defer g.GinkgoRecover()
-	oc := exutil.NewCLIWithPodSecurityLevel("build-webhooks", admissionapi.LevelBaseline)
+
+	var (
+		oc = exutil.NewCLIWithPodSecurityLevel("build-webhooks", admissionapi.LevelBaseline)
+	)
 
 	g.It("TestWebhook [apigroup:build.openshift.io][apigroup:image.openshift.io]", func() {
 		TestWebhook(g.GinkgoT(), oc)
@@ -43,6 +47,7 @@ var _ = g.Describe("[sig-builds][Feature:Builds][webhook]", func() {
 
 func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
 	clusterAdminBuildClient := oc.AdminBuildClient().BuildV1()
+	adminHTTPClient := clusterAdminBuildClient.RESTClient().(*rest.RESTClient).Client
 
 	// create buildconfig
 	buildConfig := mockBuildConfigImageParms("originalimage", "imagestream", "validtag")
@@ -53,10 +58,12 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
 	// Bug #1752581: reduce number of URLs per test case
 	// OCP 4.2 tests on GCP had high flake levels because namespaces took too long to tear down
 	tests := []struct {
-		Name       string
-		Payload    string
-		HeaderFunc func(*http.Header)
-		URLs       []string
+		Name           string
+		Payload        string
+		HeaderFunc     func(*http.Header)
+		URLs           []string
+		client         *http.Client
+		expectedStatus int
 	}{
 		{
 			Name:       "generic",
@@ -65,6 +72,8 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
 			URLs: []string{
 				"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret200/generic",
 			},
+			client:         adminHTTPClient,
+			expectedStatus: http.StatusOK,
 		},
 		{
 			Name:       "github",
@@ -73,6 +82,8 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
 			URLs: []string{
 				"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret100/github",
 			},
+			client:         adminHTTPClient,
+			expectedStatus: http.StatusOK,
 		},
 		{
 			Name:       "gitlab",
@@ -81,6 +92,8 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
 			URLs: []string{
 				"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret300/gitlab",
 			},
+			client:         adminHTTPClient,
+			expectedStatus: http.StatusOK,
 		},
 		{
 			Name:       "bitbucket",
@@ -89,6 +102,27 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
 			URLs: []string{
 				"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret400/bitbucket",
 			},
+			client:         adminHTTPClient,
+			expectedStatus: http.StatusOK,
+		},
+		{
+			// AUTH-509: Webhooks do not allow unauthenticated requests by default.
+			// Test will verify that an unauthenticated request fails with 403 Forbidden.
+			Name:       "unauthenticated forbidden",
+			Payload:    "generic/testdata/push-generic.json",
+			HeaderFunc: genericHeaderFunc,
+			URLs: []string{
+				"/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret200/generic",
+			},
+			// Need client to skip TLS verification - CI clusters have self-signed certificates.
+			client: &http.Client{
+				Transport: &http.Transport{
+					TLSClientConfig: &tls.Config{
+						InsecureSkipVerify: true,
+					},
+				},
+			},
+			expectedStatus: http.StatusForbidden,
 		},
 	}
 
@@ -99,8 +133,12 @@ func TestWebhook(t g.GinkgoTInterface, oc *exutil.CLI) {
 			clusterAdminClientConfig := oc.AdminConfig()
 
 			g.By("executing the webhook to get the build object")
-			body := postFile(clusterAdminBuildClient.RESTClient(), test.HeaderFunc, test.Payload, clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
+			body := postFile(test.client, test.HeaderFunc, test.Payload, clusterAdminClientConfig.Host+s, test.expectedStatus, t, oc)
 			o.Expect(body).NotTo(o.BeEmpty())
+			// If expected HTTP status is not 200 OK, continue as we will not receive a Build object in the response body.
+			if test.expectedStatus != http.StatusOK {
+				continue
+			}
 
 			g.By("Unmarshalling the build object")
 			returnedBuild := &buildv1.Build{}
@@ -124,6 +162,7 @@ func TestWebhookGitHubPushWithImage(t g.GinkgoTInterface, oc *exutil.CLI) {
 	clusterAdminClientConfig := oc.AdminConfig()
 	clusterAdminImageClient := oc.AdminImageClient().ImageV1()
 	clusterAdminBuildClient := oc.AdminBuildClient().BuildV1()
+	adminHTTPClient := clusterAdminBuildClient.RESTClient().(*rest.RESTClient).Client
 
 	// create imagerepo
 	imageStream := &imagev1.ImageStream{
@@ -173,7 +212,7 @@ func TestWebhookGitHubPushWithImage(t g.GinkgoTInterface, oc *exutil.CLI) {
 	} {
 
 		// trigger build event sending push notification
-		body := postFile(clusterAdminBuildClient.RESTClient(), githubHeaderFunc, "github/testdata/pushevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
+		body := postFile(adminHTTPClient, githubHeaderFunc, "github/testdata/pushevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
 		if len(body) == 0 {
 			t.Errorf("Webhook did not return Build in body")
 		}
@@ -201,7 +240,6 @@ func TestWebhookGitHubPushWithImage(t g.GinkgoTInterface, oc *exutil.CLI) {
 		if actual.Spec.Strategy.DockerStrategy.From.Name != "originalimage" {
 			t.Errorf("Expected %s, got %s", "originalimage", actual.Spec.Strategy.DockerStrategy.From.Name)
 		}
-
 		if actual.Name != returnedBuild.Name {
 			t.Errorf("Build returned in response body does not match created Build. Expected %s, got %s", actual.Name, returnedBuild.Name)
 		}
@@ -214,6 +252,7 @@ func TestWebhookGitHubPushWithImageStream(t g.GinkgoTInterface, oc *exutil.CLI)
 	clusterAdminClientConfig := oc.AdminConfig()
 	clusterAdminImageClient := oc.AdminImageClient().ImageV1()
 	clusterAdminBuildClient := oc.AdminBuildClient().BuildV1()
+	adminHTTPClient := clusterAdminBuildClient.RESTClient().(*rest.RESTClient).Client
 
 	// create imagerepo
 	imageStream := &imagev1.ImageStream{
@@ -265,7 +304,7 @@ func TestWebhookGitHubPushWithImageStream(t g.GinkgoTInterface, oc *exutil.CLI)
 	s := "/apis/build.openshift.io/v1/namespaces/" + oc.Namespace() + "/buildconfigs/pushbuild/webhooks/secret101/github"
 
 	// trigger build event sending push notification
-	postFile(clusterAdminBuildClient.RESTClient(), githubHeaderFunc, "github/testdata/pushevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
+	postFile(adminHTTPClient, githubHeaderFunc, "github/testdata/pushevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
 
 	var build *buildv1.Build
 
@@ -291,6 +330,7 @@ Loop:
 
 func TestWebhookGitHubPing(t g.GinkgoTInterface, oc *exutil.CLI) {
 	clusterAdminBuildClient := oc.AdminBuildClient().BuildV1()
+	adminHTTPClient := clusterAdminBuildClient.RESTClient().(*rest.RESTClient).Client
 
 	// create buildconfig
 	buildConfig := mockBuildConfigImageParms("originalimage", "imagestream", "validtag")
@@ -312,7 +352,7 @@ func TestWebhookGitHubPing(t g.GinkgoTInterface, oc *exutil.CLI) {
 		// trigger build event sending push notification
 		clusterAdminClientConfig := oc.AdminConfig()
 
-		postFile(clusterAdminBuildClient.RESTClient(), githubHeaderFuncPing, "github/testdata/pingevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
+		postFile(adminHTTPClient, githubHeaderFuncPing, "github/testdata/pingevent.json", clusterAdminClientConfig.Host+s, http.StatusOK, t, oc)
 
 		// TODO: improve negative testing
 		timer := time.NewTimer(time.Second * 5)
@@ -326,7 +366,7 @@ func TestWebhookGitHubPing(t g.GinkgoTInterface, oc *exutil.CLI) {
 	}
 }
 
-func postFile(client rest.Interface, headerFunc func(*http.Header), filename, url string, expStatusCode int, t g.GinkgoTInterface, oc *exutil.CLI) []byte {
+func postFile(client *http.Client, headerFunc func(*http.Header), filename, url string, expStatusCode int, t g.GinkgoTInterface, oc *exutil.CLI) []byte {
 	path := exutil.FixturePath("testdata", "builds", "webhook", filename)
 	data, err := ioutil.ReadFile(path)
 	if err != nil {
@@ -337,7 +377,7 @@ func postFile(client rest.Interface, headerFunc func(*http.Header), filename, ur
 		t.Fatalf("Error creating POST request: %v", err)
 	}
 	headerFunc(&req.Header)
-	resp, err := client.(*rest.RESTClient).Client.Do(req)
+	resp, err := client.Do(req)
 	if err != nil {
 		t.Fatalf("Failed posting webhook: %v", err)
 	}

diff --git a/test/extended/testdata/bindata.go b/test/extended/testdata/bindata.go
diff --git a/test/extended/testdata/builds/webhook/webhooks-unauth.yaml b/test/extended/testdata/builds/webhook/webhooks-unauth.yaml
@@ -0,0 +1,17 @@
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:
+  annotations:
+    rbac.authorization.kubernetes.io/autoupdate: "true"
+  name: webhooks-unauth
+roleRef:
+  apiGroup: rbac.authorization.k8s.io
+  kind: ClusterRole
+  name: system:webhook
+subjects:
+- apiGroup: rbac.authorization.k8s.io
+  kind: Group
+  name: system:authenticated
+- apiGroup: rbac.authorization.k8s.io
+  kind: Group
+  name: system:unauthenticated