From 3da28e4bf73647bfdd9fedd786cf1cc9cb1faf53 Mon Sep 17 00:00:00 2001 From: Roke Jung Date: Mon, 20 Jul 2026 03:27:40 -0400 Subject: [PATCH] :bug: fix(placement): correct debug permission checks and reject bad methods (#1623) Map GET/POST debug requests to get/create SAR verbs and return 403 on denial. Reject unsupported HTTP methods with 405 instead of treating them as GET. Update the GET RBAC integration test Role to grant get. Also free unused disk on e2e runners before image builds to reduce "no space left on device" flakes. Fixes open-cluster-management-io/ocm#1622 Signed-off-by: Roke Jung Co-authored-by: Cursor --- .github/scripts/free-disk-space.sh | 23 ++++++ .github/workflows/e2e.yml | 8 ++ pkg/placement/debugger/debugger.go | 40 +++++++--- pkg/placement/debugger/debugger_test.go | 87 +++++++++++++++++++-- test/integration/placement/debugger_test.go | 7 +- 5 files changed, 147 insertions(+), 18 deletions(-) create mode 100755 .github/scripts/free-disk-space.sh diff --git a/.github/scripts/free-disk-space.sh b/.github/scripts/free-disk-space.sh new file mode 100755 index 000000000..eedc5159d --- /dev/null +++ b/.github/scripts/free-disk-space.sh @@ -0,0 +1,23 @@ +#!/usr/bin/env bash +# Reclaim disk on GitHub-hosted Ubuntu runners before heavy image builds. +set -euo pipefail + +echo "::group::Disk before" +df -h / +echo "::endgroup::" + +sudo rm -rf \ + /usr/share/dotnet \ + /usr/local/lib/android \ + /opt/ghc \ + /opt/hostedtoolcache/CodeQL \ + /usr/local/share/boost \ + /usr/share/swift \ + /usr/local/.ghcup || true + +sudo docker image prune -af || true +sudo docker builder prune -af || true + +echo "::group::Disk after" +df -h / +echo "::endgroup::" diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index e1766f2ba..b50fd227b 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -32,6 +32,8 @@ jobs: if: ${{ env.ACT }} # this step only runs locally when using the https://github.com/nektos/act to debug the e2e - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - name: Free disk space + run: bash .github/scripts/free-disk-space.sh - name: Setup Go uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: @@ -57,6 +59,8 @@ jobs: if: ${{ env.ACT }} # this step only runs locally when using the https://github.com/nektos/act to debug the e2e - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - name: Free disk space + run: bash .github/scripts/free-disk-space.sh - name: Setup Go uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: @@ -82,6 +86,8 @@ jobs: if: ${{ env.ACT }} # this step only runs locally when using the https://github.com/nektos/act to debug the e2e - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - name: Free disk space + run: bash .github/scripts/free-disk-space.sh - name: Setup Go uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: @@ -107,6 +113,8 @@ jobs: if: ${{ env.ACT }} # this step only runs locally when using the https://github.com/nektos/act to debug the e2e - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - name: Free disk space + run: bash .github/scripts/free-disk-space.sh - name: Setup Go uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: diff --git a/pkg/placement/debugger/debugger.go b/pkg/placement/debugger/debugger.go index 627989940..baf52097d 100644 --- a/pkg/placement/debugger/debugger.go +++ b/pkg/placement/debugger/debugger.go @@ -80,16 +80,15 @@ func (d *Debugger) Handler(w http.ResponseWriter, r *http.Request) { var placement *clusterv1beta1.Placement var err error - // Support both GET (fetch from API) and POST (accept JSON body) - if r.Method == http.MethodPost { + switch r.Method { + case http.MethodPost: // POST: Parse Placement from request body placement, err = d.parsePlacementFromBody(r) if err != nil { d.reportErr(w, http.StatusBadRequest, err) return } - } else { - // GET: Fetch Placement from API (original behavior) + case http.MethodGet: namespace, name, err := d.parsePath(r.URL.Path) if err != nil { d.reportErr(w, http.StatusBadRequest, err) @@ -109,9 +108,12 @@ func (d *Debugger) Handler(w http.ResponseWriter, r *http.Request) { } return } + default: + d.reportErr(w, http.StatusMethodNotAllowed, fmt.Errorf("method %s not allowed", r.Method)) + return } - // Check if user has permission to create placements in this namespace + // Check if user has permission to access placements in this namespace if err := d.checkPermission(r, placement.Namespace); err != nil { d.reportPermissionErr(w, err) return @@ -209,11 +211,24 @@ func (d *Debugger) reportPermissionErr(w http.ResponseWriter, err error) { statusCode = http.StatusUnauthorized case strings.Contains(msg, "does not have permission"): statusCode = http.StatusForbidden + case strings.Contains(msg, "unsupported method"): + statusCode = http.StatusMethodNotAllowed } d.reportErr(w, statusCode, err) } -// checkPermission checks if the user has permission to create placements in the namespace using SAR +func placementPermissionVerb(r *http.Request) (string, error) { + switch r.Method { + case http.MethodPost: + return "create", nil + case http.MethodGet: + return "get", nil + default: + return "", fmt.Errorf("unsupported method %s", r.Method) + } +} + +// checkPermission checks if the user has permission to access placements in the namespace using SAR func (d *Debugger) checkPermission(r *http.Request, namespace string) error { // Get user from request context (authenticated by GenericAPIServer) ctx := r.Context() @@ -229,7 +244,12 @@ func (d *Debugger) checkPermission(r *http.Request, namespace string) error { extra[k] = authorizationv1.ExtraValue(v) } - // Create SubjectAccessReview to check if user can create placements + verb, err := placementPermissionVerb(r) + if err != nil { + return err + } + + // Create SubjectAccessReview to check if user can access placements sar := &authorizationv1.SubjectAccessReview{ Spec: authorizationv1.SubjectAccessReviewSpec{ User: username, @@ -237,7 +257,7 @@ func (d *Debugger) checkPermission(r *http.Request, namespace string) error { Extra: extra, ResourceAttributes: &authorizationv1.ResourceAttributes{ Namespace: namespace, - Verb: "create", + Verb: verb, Group: "cluster.open-cluster-management.io", Version: "v1beta1", Resource: "placements", @@ -254,8 +274,8 @@ func (d *Debugger) checkPermission(r *http.Request, namespace string) error { } if !result.Status.Allowed { - return fmt.Errorf("user does not have permission to create placements in namespace %s: %s", - namespace, result.Status.Reason) + return fmt.Errorf("user does not have permission to %s placements in namespace %s: %s", + verb, namespace, result.Status.Reason) } return nil diff --git a/pkg/placement/debugger/debugger_test.go b/pkg/placement/debugger/debugger_test.go index 5d1ec58d4..32687fe61 100644 --- a/pkg/placement/debugger/debugger_test.go +++ b/pkg/placement/debugger/debugger_test.go @@ -230,10 +230,20 @@ func TestReportPermissionErr(t *testing.T) { expectStatus: http.StatusUnauthorized, }, { - name: "forbidden", + name: "forbidden create", err: fmt.Errorf("user does not have permission to create placements in namespace test: denied"), expectStatus: http.StatusForbidden, }, + { + name: "forbidden get", + err: fmt.Errorf("user does not have permission to get placements in namespace test: denied"), + expectStatus: http.StatusForbidden, + }, + { + name: "unsupported method", + err: fmt.Errorf("unsupported method DELETE"), + expectStatus: http.StatusMethodNotAllowed, + }, { name: "internal", err: fmt.Errorf("failed to check permissions: boom"), @@ -301,6 +311,27 @@ func TestDebuggerHandlerReportErr(t *testing.T) { expectStatus: http.StatusNotFound, errorContains: "does-not-exist", }, + { + name: "DELETE method not allowed", + method: http.MethodDelete, + pathSuffix: placementNamespace + "/" + placementName, + expectStatus: http.StatusMethodNotAllowed, + errorContains: "method DELETE not allowed", + }, + { + name: "PUT method not allowed", + method: http.MethodPut, + pathSuffix: placementNamespace + "/" + placementName, + expectStatus: http.StatusMethodNotAllowed, + errorContains: "method PUT not allowed", + }, + { + name: "PATCH method not allowed", + method: http.MethodPatch, + pathSuffix: placementNamespace + "/" + placementName, + expectStatus: http.StatusMethodNotAllowed, + errorContains: "method PATCH not allowed", + }, { name: "GET placement lister failure", initObjs: validObjs, @@ -769,6 +800,8 @@ func TestDebuggerPermissionCheck(t *testing.T) { cases := []struct { name string + method string + body []byte injectUser bool userInfo *user.DefaultInfo sarAllowed bool @@ -793,12 +826,33 @@ func TestDebuggerPermissionCheck(t *testing.T) { expectStatusCode: http.StatusOK, }, { - name: "User without permission - should reject", + name: "GET user without permission - should reject with get verb", + method: http.MethodGet, injectUser: true, sarAllowed: false, expectError: true, expectStatusCode: http.StatusForbidden, - errorContains: "does not have permission", + errorContains: "does not have permission to get placements", + verifySARRequest: func(t *testing.T, sar *authorizationv1.SubjectAccessReview) { + if sar.Spec.ResourceAttributes.Verb != "get" { + t.Errorf("Expected SAR verb get, got %s", sar.Spec.ResourceAttributes.Verb) + } + }, + }, + { + name: "POST user without permission - should reject with create verb", + method: http.MethodPost, + body: []byte(`{"apiVersion":"cluster.open-cluster-management.io/v1beta1","kind":"Placement","metadata":{"name":"test-placement","namespace":"test-ns"},"spec":{"numberOfClusters":1}}`), + injectUser: true, + sarAllowed: false, + expectError: true, + expectStatusCode: http.StatusForbidden, + errorContains: "does not have permission to create placements", + verifySARRequest: func(t *testing.T, sar *authorizationv1.SubjectAccessReview) { + if sar.Spec.ResourceAttributes.Verb != "create" { + t.Errorf("Expected SAR verb create, got %s", sar.Spec.ResourceAttributes.Verb) + } + }, }, { name: "SAR API call fails", @@ -899,8 +953,31 @@ func TestDebuggerPermissionCheck(t *testing.T) { defer server.Close() // Send request - url := fmt.Sprintf("%s%s%s/%s", server.URL, DebugPath, placementNamespace, placementName) - res, err := http.Get(url) + method := c.method + if method == "" { + method = http.MethodGet + } + + var url string + var body io.Reader + if method == http.MethodPost { + url = fmt.Sprintf("%s%s", server.URL, DebugPath) + if len(c.body) > 0 { + body = bytes.NewBuffer(c.body) + } + } else { + url = fmt.Sprintf("%s%s%s/%s", server.URL, DebugPath, placementNamespace, placementName) + } + + req, err := http.NewRequest(method, url, body) + if err != nil { + t.Fatalf("Failed to create request: %v", err) + } + if method == http.MethodPost { + req.Header.Set("Content-Type", "application/json") + } + + res, err := http.DefaultClient.Do(req) if err != nil { t.Fatalf("Unexpected error: %v", err) } diff --git a/test/integration/placement/debugger_test.go b/test/integration/placement/debugger_test.go index d97c1fb3a..0c9e6db1f 100644 --- a/test/integration/placement/debugger_test.go +++ b/test/integration/placement/debugger_test.go @@ -92,7 +92,7 @@ var _ = ginkgo.Describe("DebugService", func() { _, err := kubeClient.CoreV1().ServiceAccounts(namespace).Create(context.Background(), sa, metav1.CreateOptions{}) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - // Create Role with permission to create placements + // Create Role with permission to get placements (GET debug endpoint checks get verb) role := &rbacv1.Role{ ObjectMeta: metav1.ObjectMeta{ Name: roleName, @@ -102,7 +102,7 @@ var _ = ginkgo.Describe("DebugService", func() { { APIGroups: []string{"cluster.open-cluster-management.io"}, Resources: []string{"placements"}, - Verbs: []string{"create"}, + Verbs: []string{"get"}, }, }, } @@ -258,8 +258,9 @@ var _ = ginkgo.Describe("DebugService", func() { gomega.Expect(err).ToNot(gomega.HaveOccurred()) ginkgo.By("Verify request was rejected due to lack of permission") + gomega.Expect(resp.StatusCode).To(gomega.Equal(http.StatusForbidden)) gomega.Expect(result.Error).ToNot(gomega.BeEmpty(), "Should have permission error") - gomega.Expect(result.Error).To(gomega.ContainSubstring("does not have permission")) + gomega.Expect(result.Error).To(gomega.ContainSubstring("does not have permission to get placements")) }) ginkgo.It("Should accept POST request with placement JSON and valid token", func() {