⚠️ Use limited reader in webhooks (#2598)

* add missing limitedReader

Signed-off-by: Tim Ramlot <[email protected]>

* update maxRequestSize and explain values in comment

Signed-off-by: Tim Ramlot <[email protected]>

* use more specific http error code

Signed-off-by: Tim Ramlot <[email protected]>

* add extra comments and update value for maxRequestSize

Signed-off-by: Tim Ramlot <[email protected]>

* use const instead of var

Signed-off-by: Tim Ramlot <[email protected]>

---------

Signed-off-by: Tim Ramlot <[email protected]>

Author: inteon

pkg/webhook/admission/http.go

@@ -34,6 +34,26 @@ import (
 var admissionScheme = runtime.NewScheme()
 var admissionCodecs = serializer.NewCodecFactory(admissionScheme)
 
+// adapted from https://github.com/kubernetes/kubernetes/blob/c28c2009181fcc44c5f6b47e10e62dacf53e4da0/staging/src/k8s.io/pod-security-admission/cmd/webhook/server/server.go
+//
+// From https://github.com/kubernetes/apiserver/blob/d6876a0600de06fef75968c4641c64d7da499f25/pkg/server/config.go#L433-L442C5:
+//
+//	     1.5MB is the recommended client request size in byte
+//		 the etcd server should accept. See
+//		 https://github.com/etcd-io/etcd/blob/release-3.4/embed/config.go#L56.
+//		 A request body might be encoded in json, and is converted to
+//		 proto when persisted in etcd, so we allow 2x as the largest request
+//		 body size to be accepted and decoded in a write request.
+//
+// For the admission request, we can infer that it contains at most two objects
+// (the old and new versions of the object being admitted), each of which can
+// be at most 3MB in size. For the rest of the request, we can assume that
+// it will be less than 1MB in size. Therefore, we can set the max request
+// size to 7MB.
+// If your use case requires larger max request sizes, please
+// open an issue (https://github.com/kubernetes-sigs/controller-runtime/issues/new).
+const maxRequestSize = int64(7 * 1024 * 1024)
+
 func init() {
 	utilruntime.Must(v1.AddToScheme(admissionScheme))
 	utilruntime.Must(v1beta1.AddToScheme(admissionScheme))
@@ -55,12 +75,19 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	defer r.Body.Close()
-	body, err := io.ReadAll(r.Body)
+	limitedReader := &io.LimitedReader{R: r.Body, N: maxRequestSize}
+	body, err := io.ReadAll(limitedReader)
 	if err != nil {
 		wh.getLogger(nil).Error(err, "unable to read the body from the incoming request")
 		wh.writeResponse(w, Errored(http.StatusBadRequest, err))
 		return
 	}
+	if limitedReader.N <= 0 {
+		err := fmt.Errorf("request entity is too large; limit is %d bytes", maxRequestSize)
+		wh.getLogger(nil).Error(err, "unable to read the body from the incoming request; limit reached")
+		wh.writeResponse(w, Errored(http.StatusRequestEntityTooLarge, err))
+		return
+	}
 
 	// verify the content type is accurate
 	if contentType := r.Header.Get("Content-Type"); contentType != "application/json" {

pkg/webhook/admission/http_test.go

@@ -19,6 +19,7 @@ package admission
 import (
 	"bytes"
 	"context"
+	"crypto/rand"
 	"fmt"
 	"io"
 	"net/http"
@@ -97,6 +98,19 @@ var _ = Describe("Admission Webhooks", func() {
 			Expect(respRecorder.Body.String()).To(Equal(expected))
 		})
 
+		It("should error when given an infinite body", func() {
+			req := &http.Request{
+				Header: http.Header{"Content-Type": []string{"application/json"}},
+				Method: http.MethodPost,
+				Body:   nopCloser{Reader: rand.Reader},
+			}
+
+			expected := `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request entity is too large; limit is 7340032 bytes","code":413}}}
+`
+			webhook.ServeHTTP(respRecorder, req)
+			Expect(respRecorder.Body.String()).To(Equal(expected))
+		})
+
 		It("should return the response given by the handler with version defaulted to v1", func() {
 			req := &http.Request{
 				Header: http.Header{"Content-Type": []string{"application/json"}},

pkg/webhook/authentication/http.go

@@ -34,6 +34,13 @@ import (
 var authenticationScheme = runtime.NewScheme()
 var authenticationCodecs = serializer.NewCodecFactory(authenticationScheme)
 
+// The TokenReview resource mostly contains a bearer token which
+// at most should have a few KB's of size, so we picked 1 MB to
+// have plenty of buffer.
+// If your use case requires larger max request sizes, please
+// open an issue (https://github.com/kubernetes-sigs/controller-runtime/issues/new).
+const maxRequestSize = int64(1 * 1024 * 1024)
+
 func init() {
 	utilruntime.Must(authenticationv1.AddToScheme(authenticationScheme))
 	utilruntime.Must(authenticationv1beta1.AddToScheme(authenticationScheme))
@@ -55,12 +62,19 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	defer r.Body.Close()
-	body, err := io.ReadAll(r.Body)
+	limitedReader := &io.LimitedReader{R: r.Body, N: maxRequestSize}
+	body, err := io.ReadAll(limitedReader)
 	if err != nil {
 		wh.getLogger(nil).Error(err, "unable to read the body from the incoming request")
 		wh.writeResponse(w, Errored(err))
 		return
 	}
+	if limitedReader.N <= 0 {
+		err := fmt.Errorf("request entity is too large; limit is %d bytes", maxRequestSize)
+		wh.getLogger(nil).Error(err, "unable to read the body from the incoming request; limit reached")
+		wh.writeResponse(w, Errored(err))
+		return
+	}
 
 	// verify the content type is accurate
 	if contentType := r.Header.Get("Content-Type"); contentType != "application/json" {

pkg/webhook/authentication/http_test.go

@@ -19,6 +19,7 @@ package authentication
 import (
 	"bytes"
 	"context"
+	"crypto/rand"
 	"fmt"
 	"io"
 	"net/http"
@@ -107,6 +108,19 @@ var _ = Describe("Authentication Webhooks", func() {
 			Expect(respRecorder.Body.String()).To(Equal(expected))
 		})
 
+		It("should error when given an infinite body", func() {
+			req := &http.Request{
+				Header: http.Header{"Content-Type": []string{"application/json"}},
+				Method: http.MethodPost,
+				Body:   nopCloser{Reader: rand.Reader},
+			}
+
+			expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},"error":"request entity is too large; limit is 1048576 bytes"}}
+`
+			webhook.ServeHTTP(respRecorder, req)
+			Expect(respRecorder.Body.String()).To(Equal(expected))
+		})
+
 		It("should return the response given by the handler with version defaulted to v1", func() {
 			req := &http.Request{
 				Header: http.Header{"Content-Type": []string{"application/json"}},