From 90ea1c1a6c31a13aebabd0de80486880ac84c0e9 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Sun, 3 Dec 2023 10:10:20 +0100 Subject: [PATCH 1/5] add missing limitedReader Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/webhook/admission/http.go | 12 +++++++++++- pkg/webhook/admission/http_test.go | 14 ++++++++++++++ pkg/webhook/authentication/http.go | 12 +++++++++++- pkg/webhook/authentication/http_test.go | 14 ++++++++++++++ 4 files changed, 50 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 8bcbaf45c3..2dc5ebbdab 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -34,6 +34,9 @@ import ( var admissionScheme = runtime.NewScheme() var admissionCodecs = serializer.NewCodecFactory(admissionScheme) +// based on https://github.com/kubernetes/kubernetes/blob/c28c2009181fcc44c5f6b47e10e62dacf53e4da0/staging/src/k8s.io/pod-security-admission/cmd/webhook/server/server.go +var maxRequestSize = int64(3 * 1024 * 1024) + func init() { utilruntime.Must(v1.AddToScheme(admissionScheme)) utilruntime.Must(v1beta1.AddToScheme(admissionScheme)) @@ -55,12 +58,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.StatusBadRequest, err)) + return + } // verify the content type is accurate if contentType := r.Header.Get("Content-Type"); contentType != "application/json" { diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index ad5e809415..b9e0b5f5c1 100644 --- a/pkg/webhook/admission/http_test.go +++ b/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 3145728 bytes","code":400}}} +` + 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"}}, diff --git a/pkg/webhook/authentication/http.go b/pkg/webhook/authentication/http.go index 884f572123..ea26521e17 100644 --- a/pkg/webhook/authentication/http.go +++ b/pkg/webhook/authentication/http.go @@ -34,6 +34,9 @@ import ( var authenticationScheme = runtime.NewScheme() var authenticationCodecs = serializer.NewCodecFactory(authenticationScheme) +// based on https://github.com/kubernetes/kubernetes/blob/c28c2009181fcc44c5f6b47e10e62dacf53e4da0/staging/src/k8s.io/pod-security-admission/cmd/webhook/server/server.go +var maxRequestSize = int64(3 * 1024 * 1024) + func init() { utilruntime.Must(authenticationv1.AddToScheme(authenticationScheme)) utilruntime.Must(authenticationv1beta1.AddToScheme(authenticationScheme)) @@ -55,12 +58,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" { diff --git a/pkg/webhook/authentication/http_test.go b/pkg/webhook/authentication/http_test.go index 0305cefc62..696f7a198f 100644 --- a/pkg/webhook/authentication/http_test.go +++ b/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 3145728 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"}}, From 5ed1ff85cbdae42394c2a2a98d523b68fd201c93 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Thu, 21 Dec 2023 10:00:29 +0100 Subject: [PATCH 2/5] update maxRequestSize and explain values in comment Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/webhook/admission/http.go | 19 +++++++++++++++++-- pkg/webhook/admission/http_test.go | 2 +- pkg/webhook/authentication/http.go | 13 ++++++++++++- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 2dc5ebbdab..15e408dc43 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -34,8 +34,23 @@ import ( var admissionScheme = runtime.NewScheme() var admissionCodecs = serializer.NewCodecFactory(admissionScheme) -// based on https://github.com/kubernetes/kubernetes/blob/c28c2009181fcc44c5f6b47e10e62dacf53e4da0/staging/src/k8s.io/pod-security-admission/cmd/webhook/server/server.go -var maxRequestSize = int64(3 * 1024 * 1024) +// 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. +var maxRequestSize = int64(7 * 1024 * 1024) func init() { utilruntime.Must(v1.AddToScheme(admissionScheme)) diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index b9e0b5f5c1..75517a66f9 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -105,7 +105,7 @@ var _ = Describe("Admission Webhooks", func() { Body: nopCloser{Reader: rand.Reader}, } - expected := `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request entity is too large; limit is 3145728 bytes","code":400}}} + expected := `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request entity is too large; limit is 7340032 bytes","code":400}}} ` webhook.ServeHTTP(respRecorder, req) Expect(respRecorder.Body.String()).To(Equal(expected)) diff --git a/pkg/webhook/authentication/http.go b/pkg/webhook/authentication/http.go index ea26521e17..dd6ed9cc99 100644 --- a/pkg/webhook/authentication/http.go +++ b/pkg/webhook/authentication/http.go @@ -34,7 +34,18 @@ import ( var authenticationScheme = runtime.NewScheme() var authenticationCodecs = serializer.NewCodecFactory(authenticationScheme) -// based on https://github.com/kubernetes/kubernetes/blob/c28c2009181fcc44c5f6b47e10e62dacf53e4da0/staging/src/k8s.io/pod-security-admission/cmd/webhook/server/server.go +// 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 TokenReview request, we can assume that it too will be less than 3MB in size. var maxRequestSize = int64(3 * 1024 * 1024) func init() { From de5393154aa55f53b232afd062bdfdb03e5a7c4d Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Sat, 6 Jan 2024 13:37:38 +0100 Subject: [PATCH 3/5] use more specific http error code Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/webhook/admission/http.go | 2 +- pkg/webhook/admission/http_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 15e408dc43..1121814fc0 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -83,7 +83,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { 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.StatusBadRequest, err)) + wh.writeResponse(w, Errored(http.StatusRequestEntityTooLarge, err)) return } diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index 75517a66f9..86f35ac882 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -105,7 +105,7 @@ var _ = Describe("Admission Webhooks", func() { Body: nopCloser{Reader: rand.Reader}, } - expected := `{"response":{"uid":"","allowed":false,"status":{"metadata":{},"message":"request entity is too large; limit is 7340032 bytes","code":400}}} + 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)) From 930ab18c391f48be1811a11f697038287fbc00ae Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Sat, 6 Jan 2024 13:38:55 +0100 Subject: [PATCH 4/5] add extra comments and update value for maxRequestSize Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/webhook/admission/http.go | 2 ++ pkg/webhook/authentication/http.go | 19 ++++++------------- pkg/webhook/authentication/http_test.go | 2 +- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 1121814fc0..2861a7b250 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -50,6 +50,8 @@ var admissionCodecs = serializer.NewCodecFactory(admissionScheme) // 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). var maxRequestSize = int64(7 * 1024 * 1024) func init() { diff --git a/pkg/webhook/authentication/http.go b/pkg/webhook/authentication/http.go index dd6ed9cc99..b2aaa07e5d 100644 --- a/pkg/webhook/authentication/http.go +++ b/pkg/webhook/authentication/http.go @@ -34,19 +34,12 @@ import ( var authenticationScheme = runtime.NewScheme() var authenticationCodecs = serializer.NewCodecFactory(authenticationScheme) -// 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 TokenReview request, we can assume that it too will be less than 3MB in size. -var maxRequestSize = int64(3 * 1024 * 1024) +// 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). +var maxRequestSize = int64(1 * 1024 * 1024) func init() { utilruntime.Must(authenticationv1.AddToScheme(authenticationScheme)) diff --git a/pkg/webhook/authentication/http_test.go b/pkg/webhook/authentication/http_test.go index 696f7a198f..101b0c702d 100644 --- a/pkg/webhook/authentication/http_test.go +++ b/pkg/webhook/authentication/http_test.go @@ -115,7 +115,7 @@ var _ = Describe("Authentication Webhooks", func() { Body: nopCloser{Reader: rand.Reader}, } - expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},"error":"request entity is too large; limit is 3145728 bytes"}} + 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)) From 78b8b06add5e4c02fe1b737eaee0854545a351c6 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Mon, 8 Jan 2024 19:39:47 +0100 Subject: [PATCH 5/5] use const instead of var Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- pkg/webhook/admission/http.go | 2 +- pkg/webhook/authentication/http.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 2861a7b250..f049fb66e6 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -52,7 +52,7 @@ var admissionCodecs = serializer.NewCodecFactory(admissionScheme) // 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). -var maxRequestSize = int64(7 * 1024 * 1024) +const maxRequestSize = int64(7 * 1024 * 1024) func init() { utilruntime.Must(v1.AddToScheme(admissionScheme)) diff --git a/pkg/webhook/authentication/http.go b/pkg/webhook/authentication/http.go index b2aaa07e5d..abf95e5421 100644 --- a/pkg/webhook/authentication/http.go +++ b/pkg/webhook/authentication/http.go @@ -39,7 +39,7 @@ var authenticationCodecs = serializer.NewCodecFactory(authenticationScheme) // 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). -var maxRequestSize = int64(1 * 1024 * 1024) +const maxRequestSize = int64(1 * 1024 * 1024) func init() { utilruntime.Must(authenticationv1.AddToScheme(authenticationScheme))