diff --git a/cmd/cluster-agent/admission/server.go b/cmd/cluster-agent/admission/server.go index 2fc99b54751a..c95d2e246235 100644 --- a/cmd/cluster-agent/admission/server.go +++ b/cmd/cluster-agent/admission/server.go @@ -44,6 +44,10 @@ import ( const jsonContentType = "application/json" +// maxRequestBodyBytes bounds the AdmissionReview body the webhook will buffer. +// Matches controller-runtime's own maxRequestSize. +const maxRequestBodyBytes = int64(7 << 20) + // Request contains the information of an admission request type Request struct { // UID is the unique identifier of the AdmissionRequest @@ -200,17 +204,23 @@ func (s *Server) handle(w http.ResponseWriter, r *http.Request, webhookName stri return } - body, err := io.ReadAll(r.Body) - if err != nil { + if contentType := r.Header.Get("Content-Type"); contentType != jsonContentType { w.WriteHeader(http.StatusBadRequest) - log.Warnf("Could not read request body: %v", err) + log.Warnf("Unsupported content type %s, only %s is supported", contentType, jsonContentType) return } - defer r.Body.Close() - if contentType := r.Header.Get("Content-Type"); contentType != jsonContentType { + defer r.Body.Close() + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, maxRequestBodyBytes)) + if err != nil { + var maxBytesErr *http.MaxBytesError + if errors.As(err, &maxBytesErr) { + w.WriteHeader(http.StatusRequestEntityTooLarge) + log.Warnf("Request body exceeds %d bytes", maxRequestBodyBytes) + return + } w.WriteHeader(http.StatusBadRequest) - log.Warnf("Unsupported content type %s, only %s is supported", contentType, jsonContentType) + log.Warnf("Could not read request body: %v", err) return } diff --git a/cmd/cluster-agent/admission/server_test.go b/cmd/cluster-agent/admission/server_test.go index 2cf58fa2f40b..5eec0d64607b 100644 --- a/cmd/cluster-agent/admission/server_test.go +++ b/cmd/cluster-agent/admission/server_test.go @@ -8,7 +8,11 @@ package admission import ( + "bytes" "encoding/json" + "io" + "net/http" + "net/http/httptest" "testing" "github.com/stretchr/testify/assert" @@ -84,3 +88,138 @@ func TestProbeResponse_ProbeObject(t *testing.T) { require.True(t, ok) assert.Equal(t, "true", annotations[admicommon.ProbeReceivedAnnotationKey]) } + +// newTestServer skips NewServer to avoid leaking a health.RegisterReadiness +// goroutine that these tests never drain or deregister. +func newTestServer(t *testing.T) *Server { + t.Helper() + s := &Server{mux: http.NewServeMux()} + s.initDecoder() + return s +} + +// countingFiller hands out up to `remaining` bytes without allocating them +// up front, and records how many were actually read. +type countingFiller struct { + remaining int64 + read int64 +} + +func (f *countingFiller) Read(p []byte) (int, error) { + if f.remaining <= 0 { + return 0, io.EOF + } + n := len(p) + if int64(n) > f.remaining { + n = int(f.remaining) + } + for i := range p[:n] { + p[i] = 'a' + } + f.remaining -= int64(n) + f.read += int64(n) + return n, nil +} + +func TestHandleBodyLimits(t *testing.T) { + // Well over maxRequestBodyBytes (7 MiB), but small enough to not stress the runner. + const oversizedBodySize = 32 << 20 + + validReview := []byte(`{"apiVersion":"admission.k8s.io/v1","kind":"AdmissionReview","request":{"uid":"test-uid"}}`) + + tests := []struct { + name string + method string + contentType string + body []byte + oversized bool + wantStatus int + wantWebhookHit bool + }{ + { + name: "oversized body is rejected before being fully buffered", + method: http.MethodPost, + contentType: jsonContentType, + oversized: true, + wantStatus: http.StatusRequestEntityTooLarge, + }, + { + name: "oversized body with wrong content type is rejected without reading", + method: http.MethodPost, + contentType: "text/plain", + oversized: true, + wantStatus: http.StatusBadRequest, + }, + { + name: "oversized body with wrong method is rejected without reading", + method: http.MethodGet, + contentType: jsonContentType, + oversized: true, + wantStatus: http.StatusMethodNotAllowed, + }, + { + name: "valid v1 review is accepted", + method: http.MethodPost, + contentType: jsonContentType, + body: validReview, + wantStatus: http.StatusOK, + wantWebhookHit: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := newTestServer(t) + hit := false + s.Register("/injectconfig", "test", admicommon.MutatingWebhook, func(req *Request) *admiv1.AdmissionResponse { + hit = true + return &admiv1.AdmissionResponse{Allowed: true} + }, nil, nil) + + var body io.Reader + var filler *countingFiller + if tt.oversized { + filler = &countingFiller{remaining: oversizedBodySize} + body = filler + } else { + body = bytes.NewReader(tt.body) + } + + req := httptest.NewRequest(tt.method, "/injectconfig", body) + req.Header.Set("Content-Type", tt.contentType) + w := httptest.NewRecorder() + + s.mux.ServeHTTP(w, req) + + assert.Equal(t, tt.wantStatus, w.Code) + assert.Equal(t, tt.wantWebhookHit, hit) + if filler != nil { + // Handler must not buffer meaningfully more than the cap. + assert.LessOrEqual(t, filler.read, maxRequestBodyBytes+int64(64<<10), + "handler buffered more than the configured cap") + } + }) + } +} + +func TestHandleBodyLimitAppliesToEveryRoute(t *testing.T) { + s := newTestServer(t) + for _, uri := range []string{"/injectconfig", "/autoscaling"} { + s.Register(uri, "test", admicommon.MutatingWebhook, func(*Request) *admiv1.AdmissionResponse { + t.Fatalf("webhook func should not be called for an oversized request to %s", uri) + return nil + }, nil, nil) + } + + for _, uri := range []string{"/injectconfig", "/autoscaling"} { + filler := &countingFiller{remaining: 32 << 20} + req := httptest.NewRequest(http.MethodPost, uri, filler) + req.Header.Set("Content-Type", jsonContentType) + w := httptest.NewRecorder() + + s.mux.ServeHTTP(w, req) + + assert.Equal(t, http.StatusRequestEntityTooLarge, w.Code, "route %s", uri) + assert.LessOrEqual(t, filler.read, maxRequestBodyBytes+int64(64<<10), "route %s", uri) + } +} diff --git a/releasenotes-dca/notes/admission-webhook-body-size-limit-4030c456f1261543.yaml b/releasenotes-dca/notes/admission-webhook-body-size-limit-4030c456f1261543.yaml new file mode 100644 index 000000000000..aab71e99e214 --- /dev/null +++ b/releasenotes-dca/notes/admission-webhook-body-size-limit-4030c456f1261543.yaml @@ -0,0 +1,4 @@ +--- +security: + - | + The Cluster Agent's admission controller webhook now enforces a size limit on incoming request bodies and validates the request content type before reading the body.