From 43b42d4c623fec7f2e569f08d3e136ceea00715f Mon Sep 17 00:00:00 2001 From: Adam Kaplan Date: Fri, 26 Apr 2024 16:35:33 -0400 Subject: [PATCH] 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 --- test/extended/builds/start.go | 39 ++++++++++++++++++ test/extended/builds/webhook.go | 73 ++++++++++++++++++++++++++------- 2 files changed, 97 insertions(+), 15 deletions(-) diff --git a/test/extended/builds/start.go b/test/extended/builds/start.go index 786fc5493cba..4c436107b4f8 100644 --- a/test/extended/builds/start.go +++ b/test/extended/builds/start.go @@ -14,6 +14,8 @@ import ( o "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/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" @@ -409,6 +411,43 @@ 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() + adminRoleBindingsClient := oc.AdminKubeClient().RbacV1().RoleBindings(oc.Namespace()) + _, err := adminRoleBindingsClient.Get(ctx, "webooks-unauth", metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + unauthWebhooksRB := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "webooks-unauth", + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "ClusterRole", + Name: "system:webhook", + }, + Subjects: []rbacv1.Subject{ + { + APIGroup: "rbac.authorization.k8s.io", + Kind: "Group", + Name: "system:authenticated", + }, + { + APIGroup: "rbac.authorization.k8s.io", + Kind: "Group", + Name: "system:unauthenticated", + }, + }, + } + _, err = adminRoleBindingsClient.Create(ctx, unauthWebhooksRB, metav1.CreateOptions{}) + 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 index 41894a6991e6..6206e516df78 100644 --- a/test/extended/builds/webhook.go +++ b/test/extended/builds/webhook.go @@ -3,10 +3,12 @@ package builds import ( "bytes" "context" + "crypto/tls" "encoding/json" "fmt" - "io/ioutil" + "io" "net/http" + "os" "time" g "github.com/onsi/ginkgo/v2" @@ -25,7 +27,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 +48,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 +59,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 +73,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 +83,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 +93,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 +103,29 @@ 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. + // Transport also needs to accept proxy information from *_PROXY environment variables. + client: &http.Client{ + Transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, + }, + }, + }, + expectedStatus: http.StatusForbidden, }, } @@ -99,8 +136,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 +165,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 +215,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 +243,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 +255,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 +307,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 +333,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 +355,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,9 +369,9 @@ 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) + data, err := os.ReadFile(path) if err != nil { t.Fatalf("Failed to open %s: %v", filename, err) } @@ -337,11 +380,11 @@ 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) } - body, _ := ioutil.ReadAll(resp.Body) + body, _ := io.ReadAll(resp.Body) if resp.StatusCode != expStatusCode { t.Errorf("Wrong response code, expecting %d, got %d: %s!", expStatusCode, resp.StatusCode, string(body)) }