From 002770bf1580e07159c5a4c9c9aba02c3c8b9410 Mon Sep 17 00:00:00 2001 From: Rafael Franzke Date: Thu, 20 May 2021 13:49:37 +0200 Subject: [PATCH 1/7] Add generic webhook handler implementation for SubjectAccessReviews --- .../authorization/authorization_suite_test.go | 40 +++ pkg/webhook/authorization/doc.go | 29 +++ pkg/webhook/authorization/http.go | 165 +++++++++++++ pkg/webhook/authorization/http_test.go | 229 ++++++++++++++++++ pkg/webhook/authorization/response.go | 71 ++++++ pkg/webhook/authorization/response_test.go | 83 +++++++ pkg/webhook/authorization/webhook.go | 129 ++++++++++ pkg/webhook/authorization/webhook_test.go | 138 +++++++++++ 8 files changed, 884 insertions(+) create mode 100644 pkg/webhook/authorization/authorization_suite_test.go create mode 100644 pkg/webhook/authorization/doc.go create mode 100644 pkg/webhook/authorization/http.go create mode 100644 pkg/webhook/authorization/http_test.go create mode 100644 pkg/webhook/authorization/response.go create mode 100644 pkg/webhook/authorization/response_test.go create mode 100644 pkg/webhook/authorization/webhook.go create mode 100644 pkg/webhook/authorization/webhook_test.go diff --git a/pkg/webhook/authorization/authorization_suite_test.go b/pkg/webhook/authorization/authorization_suite_test.go new file mode 100644 index 0000000000..ae0245029a --- /dev/null +++ b/pkg/webhook/authorization/authorization_suite_test.go @@ -0,0 +1,40 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package authorization + +import ( + "testing" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "sigs.k8s.io/controller-runtime/pkg/envtest/printer" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +func TestAuthorizationWebhook(t *testing.T) { + RegisterFailHandler(Fail) + suiteName := "Authorization Webhook Suite" + RunSpecsWithDefaultAndCustomReporters(t, suiteName, []Reporter{printer.NewlineReporter{}, printer.NewProwReporter(suiteName)}) +} + +var _ = BeforeSuite(func(done Done) { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + + close(done) +}, 60) diff --git a/pkg/webhook/authorization/doc.go b/pkg/webhook/authorization/doc.go new file mode 100644 index 0000000000..bea4fede0d --- /dev/null +++ b/pkg/webhook/authorization/doc.go @@ -0,0 +1,29 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/* +Package authorization provides implementation for authorization webhook and +methods to implement authorization webhook handlers. + +See examples/subjectaccessreview/ for an example of authorization webhooks. +*/ +package authorization + +import ( + logf "sigs.k8s.io/controller-runtime/pkg/internal/log" +) + +var log = logf.RuntimeLog.WithName("authorization") diff --git a/pkg/webhook/authorization/http.go b/pkg/webhook/authorization/http.go new file mode 100644 index 0000000000..c60366d2ea --- /dev/null +++ b/pkg/webhook/authorization/http.go @@ -0,0 +1,165 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package authorization + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "io/ioutil" + "net/http" + + authorizationv1 "k8s.io/api/authorization/v1" + authorizationv1beta1 "k8s.io/api/authorization/v1beta1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" +) + +var authorizationScheme = runtime.NewScheme() +var authorizationCodecs = serializer.NewCodecFactory(authorizationScheme) + +func init() { + utilruntime.Must(authorizationv1.AddToScheme(authorizationScheme)) + utilruntime.Must(authorizationv1beta1.AddToScheme(authorizationScheme)) +} + +var _ http.Handler = &Webhook{} + +func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { + var body []byte + var err error + ctx := r.Context() + if wh.WithContextFunc != nil { + ctx = wh.WithContextFunc(ctx, r) + } + + var reviewResponse Response + if r.Body == nil { + err = errors.New("request body is empty") + wh.log.Error(err, "bad request") + reviewResponse = Errored(err) + wh.writeResponse(w, reviewResponse) + return + } + + defer r.Body.Close() + if body, err = ioutil.ReadAll(r.Body); err != nil { + wh.log.Error(err, "unable to read the body from the incoming request") + reviewResponse = Errored(err) + wh.writeResponse(w, reviewResponse) + return + } + + // verify the content type is accurate + contentType := r.Header.Get("Content-Type") + if contentType != "application/json" { + err = fmt.Errorf("contentType=%s, expected application/json", contentType) + wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) + reviewResponse = Errored(err) + wh.writeResponse(w, reviewResponse) + return + } + + // Decode request body into authorizationv1.SubjectAccessReviewSpec structure + sar, actualTokRevGVK, err := wh.decodeRequestBody(body) + if err != nil { + wh.log.Error(err, "unable to decode the request") + reviewResponse = Errored(err) + wh.writeResponse(w, reviewResponse) + return + } + wh.log.V(1).Info("received request", "UID", sar.UID, "kind", sar.Kind) + + reviewResponse = wh.Handle(ctx, Request{sar.SubjectAccessReview}) + wh.writeResponseTyped(w, reviewResponse, actualTokRevGVK) +} + +// writeResponse writes response to w generically, i.e. without encoding GVK information. +func (wh *Webhook) writeResponse(w io.Writer, response Response) { + wh.writeSubjectAccessReviewResponse(w, response.SubjectAccessReview) +} + +// writeResponseTyped writes response to w with GVK set to subjRevGVK, which is necessary +// if multiple SubjectAccessReview versions are permitted by the webhook. +func (wh *Webhook) writeResponseTyped(w io.Writer, response Response, subjRevGVK *schema.GroupVersionKind) { + ar := response.SubjectAccessReview + + // Default to a v1 SubjectAccessReview, otherwise the API server may not recognize the request + // if multiple SubjectAccessReview versions are permitted by the webhook config. + if subjRevGVK == nil || *subjRevGVK == (schema.GroupVersionKind{}) { + ar.SetGroupVersionKind(authorizationv1.SchemeGroupVersion.WithKind("SubjectAccessReview")) + } else { + ar.SetGroupVersionKind(*subjRevGVK) + } + wh.writeSubjectAccessReviewResponse(w, ar) +} + +// writeSubjectAccessReviewResponse writes ar to w. +func (wh *Webhook) writeSubjectAccessReviewResponse(w io.Writer, ar authorizationv1.SubjectAccessReview) { + if err := json.NewEncoder(w).Encode(ar); err != nil { + wh.log.Error(err, "unable to encode the response") + wh.writeResponse(w, Errored(err)) + } + res := ar + if log := wh.log; log.V(1).Enabled() { + log.V(1).Info("wrote response", "UID", res.UID, "authorized", res.Status.Allowed) + } + return +} + +func (wh *Webhook) decodeRequestBody(body []byte) (unversionedSubjectAccessReview, *schema.GroupVersionKind, error) { + // v1 and v1beta1 SubjectAccessReview types are almost exactly the same (the only difference is the JSON key for the + // 'Groups' field).The v1beta1 api is deprecated as of 1.19 and will be removed in authorization as of v1.22. We + // decode the object into a v1 type and "manually" convert the 'Groups' field (see below). + // However, the runtime codec's decoder guesses which type to decode into by type name if an Object's TypeMeta + // isn't set. By setting TypeMeta of an unregistered type to the v1 GVK, the decoder will coerce a v1beta1 + // SubjectAccessReview to v1. + var obj unversionedSubjectAccessReview + obj.SetGroupVersionKind(authorizationv1.SchemeGroupVersion.WithKind("SubjectAccessReview")) + + _, gvk, err := authorizationCodecs.UniversalDeserializer().Decode(body, nil, &obj) + if err != nil { + return obj, nil, err + } + if gvk == nil { + return obj, nil, fmt.Errorf("could not determine GVK for object in the request body") + } + + // The only difference in v1beta1 is that the JSON key name of the 'Groups' field is different. Hence, when we + // detect that v1beta1 was sent, we decode it once again into the "correct" type and manually "convert" the 'Groups' + // information. + switch *gvk { + case authorizationv1beta1.SchemeGroupVersion.WithKind("SubjectAccessReview"): + var tmp authorizationv1beta1.SubjectAccessReview + if _, _, err := authorizationCodecs.UniversalDeserializer().Decode(body, nil, &tmp); err != nil { + return obj, gvk, err + } + obj.Spec.Groups = tmp.Spec.Groups + } + + return obj, gvk, nil +} + +// unversionedSubjectAccessReview is used to decode both v1 and v1beta1 SubjectAccessReview types. +type unversionedSubjectAccessReview struct { + authorizationv1.SubjectAccessReview +} + +var _ runtime.Object = &unversionedSubjectAccessReview{} diff --git a/pkg/webhook/authorization/http_test.go b/pkg/webhook/authorization/http_test.go new file mode 100644 index 0000000000..914c361edd --- /dev/null +++ b/pkg/webhook/authorization/http_test.go @@ -0,0 +1,229 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package authorization + +import ( + "bytes" + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + authorizationv1 "k8s.io/api/authorization/v1" + + logf "sigs.k8s.io/controller-runtime/pkg/internal/log" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" +) + +var _ = Describe("Authentication Webhooks", func() { + + const ( + gvkJSONv1 = `"kind":"SubjectAccessReview","apiVersion":"authorization.k8s.io/v1"` + gvkJSONv1beta1 = `"kind":"SubjectAccessReview","apiVersion":"authorization.k8s.io/v1beta1"` + ) + + Describe("HTTP Handler", func() { + var respRecorder *httptest.ResponseRecorder + webhook := &Webhook{ + Handler: nil, + } + BeforeEach(func() { + respRecorder = &httptest.ResponseRecorder{ + Body: bytes.NewBuffer(nil), + } + _, err := inject.LoggerInto(log.WithName("test-webhook"), webhook) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should return bad-request when given an empty body", func() { + req := &http.Request{Body: nil} + + expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,"evaluationError":"request body is empty"}} +` + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.String()).To(Equal(expected)) + }) + + It("should return bad-request when given the wrong content-type", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/foo"}}, + Method: http.MethodPost, + Body: nopCloser{Reader: bytes.NewBuffer(nil)}, + } + + expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,"evaluationError":"contentType=application/foo, expected application/json"}} +` + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.String()).To(Equal(expected)) + }) + + It("should return bad-request when given an undecodable body", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/json"}}, + Method: http.MethodPost, + Body: nopCloser{Reader: bytes.NewBufferString("{")}, + } + + expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,` + + `"evaluationError":"couldn't get version/kind; json parse error: unexpected end of JSON input"}} +` + 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"}}, + Method: http.MethodPost, + Body: nopCloser{Reader: bytes.NewBufferString(`{"spec":{"token":"foobar"}}`)}, + } + webhook := &Webhook{ + Handler: &fakeHandler{}, + log: logf.RuntimeLog.WithName("webhook"), + } + + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":true}} +`, gvkJSONv1) + + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.String()).To(Equal(expected)) + }) + + It("should return the v1 response given by the handler", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/json"}}, + Method: http.MethodPost, + Body: nopCloser{Reader: bytes.NewBufferString(fmt.Sprintf(`{%s,"spec":{"user":"foobar"}}`, gvkJSONv1))}, + } + webhook := &Webhook{ + Handler: &fakeHandler{}, + log: logf.RuntimeLog.WithName("webhook"), + } + + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":true}} +`, gvkJSONv1) + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.String()).To(Equal(expected)) + }) + + It("should return the v1beta1 response given by the handler", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/json"}}, + Method: http.MethodPost, + Body: nopCloser{Reader: bytes.NewBufferString(fmt.Sprintf(`{%s,"spec":{"user":"foobar"}}`, gvkJSONv1beta1))}, + } + webhook := &Webhook{ + Handler: &fakeHandler{}, + log: logf.RuntimeLog.WithName("webhook"), + } + + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":true}} +`, gvkJSONv1beta1) + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.String()).To(Equal(expected)) + }) + + It("should present the Context from the HTTP request, if any", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/json"}}, + Method: http.MethodPost, + Body: nopCloser{Reader: bytes.NewBufferString(`{"spec":{"token":"foobar"}}`)}, + } + type ctxkey int + const key ctxkey = 1 + const value = "from-ctx" + webhook := &Webhook{ + Handler: &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + <-ctx.Done() + return NoOpinion(ctx.Value(key).(string)) + }, + }, + log: logf.RuntimeLog.WithName("webhook"), + } + + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,"reason":%q}} +`, gvkJSONv1, value) + + ctx, cancel := context.WithCancel(context.WithValue(context.Background(), key, value)) + cancel() + webhook.ServeHTTP(respRecorder, req.WithContext(ctx)) + Expect(respRecorder.Body.String()).To(Equal(expected)) + }) + + It("should mutate the Context from the HTTP request, if func supplied", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/json"}}, + Method: http.MethodPost, + Body: nopCloser{Reader: bytes.NewBufferString(`{"spec":{"user":"foobar"}}`)}, + } + type ctxkey int + const key ctxkey = 1 + webhook := &Webhook{ + Handler: &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + return NoOpinion(ctx.Value(key).(string)) + }, + }, + WithContextFunc: func(ctx context.Context, r *http.Request) context.Context { + return context.WithValue(ctx, key, r.Header["Content-Type"][0]) + }, + log: logf.RuntimeLog.WithName("webhook"), + } + + expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,"reason":%q}} +`, gvkJSONv1, "application/json") + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + webhook.ServeHTTP(respRecorder, req.WithContext(ctx)) + Expect(respRecorder.Body.String()).To(Equal(expected)) + }) + }) +}) + +type nopCloser struct { + io.Reader +} + +func (nopCloser) Close() error { return nil } + +type fakeHandler struct { + invoked bool + fn func(context.Context, Request) Response + injectedString string +} + +func (h *fakeHandler) InjectString(s string) error { + h.injectedString = s + return nil +} + +func (h *fakeHandler) Handle(ctx context.Context, req Request) Response { + h.invoked = true + if h.fn != nil { + return h.fn(ctx, req) + } + return Response{SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: true, + }, + }} +} diff --git a/pkg/webhook/authorization/response.go b/pkg/webhook/authorization/response.go new file mode 100644 index 0000000000..27480846c2 --- /dev/null +++ b/pkg/webhook/authorization/response.go @@ -0,0 +1,71 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package authorization + +import ( + authorizationv1 "k8s.io/api/authorization/v1" +) + +// Allowed constructs a SubjectAccessReview and indicates in its status that the given operation is allowed. +func Allowed() Response { + return Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: true, + }, + }, + } +} + +// Denied constructs a SubjectAccessReview and indicates in its status that the given operation is denied and that +// other authenticators should not be consulted for their opinion. +func Denied(reason string) Response { + return Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: false, + Denied: true, + Reason: reason, + }, + }, + } +} + +// NoOpinion constructs a SubjectAccessReview and indicates in its status that the authorizer does not have an opinion +// about the result, i.e., other authenticators should be consulted for their opinion. +func NoOpinion(reason string) Response { + return Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: false, + Reason: reason, + }, + }, + } +} + +// Errored constructs a SubjectAccessReview and indicates in its status that the an error has been occurred during the +// evaluation of the result. +func Errored(err error) Response { + return Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + EvaluationError: err.Error(), + }, + }, + } +} diff --git a/pkg/webhook/authorization/response_test.go b/pkg/webhook/authorization/response_test.go new file mode 100644 index 0000000000..b9bd36e14f --- /dev/null +++ b/pkg/webhook/authorization/response_test.go @@ -0,0 +1,83 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package authorization + +import ( + "fmt" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + authorizationv1 "k8s.io/api/authorization/v1" +) + +var _ = Describe("Response", func() { + var ( + reason = "reason" + fakeErr = fmt.Errorf("fake") + ) + + Describe("#Allowed", func() { + It("should return the expected status", func() { + Expect(Allowed()).To(Equal(Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: true, + }, + }, + })) + }) + }) + + Describe("#Denied", func() { + It("should return the expected status", func() { + Expect(Denied(reason)).To(Equal(Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: false, + Denied: true, + Reason: reason, + }, + }, + })) + }) + }) + + Describe("#NoOpinion", func() { + It("should return the expected status", func() { + Expect(NoOpinion(reason)).To(Equal(Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: false, + Reason: reason, + }, + }, + })) + }) + }) + + Describe("#Errored", func() { + It("should return the expected status", func() { + Expect(Errored(fakeErr)).To(Equal(Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + EvaluationError: fakeErr.Error(), + }, + }, + })) + }) + }) +}) diff --git a/pkg/webhook/authorization/webhook.go b/pkg/webhook/authorization/webhook.go new file mode 100644 index 0000000000..95371d7bf6 --- /dev/null +++ b/pkg/webhook/authorization/webhook.go @@ -0,0 +1,129 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package authorization + +import ( + "context" + "errors" + "net/http" + + "github.com/go-logr/logr" + authorizationv1 "k8s.io/api/authorization/v1" + + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" +) + +var ( + errUnableToEncodeResponse = errors.New("unable to encode response") +) + +// Request defines the input for an authorization handler. +// It contains information to identify the object in +// question (group, version, kind, resource, subresource, +// name, namespace), as well as the operation in question +// (e.g. Get, Create, etc), and the object itself. +type Request struct { + authorizationv1.SubjectAccessReview +} + +// Response is the output of an authorization handler. +// It contains a response indicating if a given +// operation is allowed +type Response struct { + authorizationv1.SubjectAccessReview +} + +// Complete populates any fields that are yet to be set in +// the underlying TokenResponse, It mutates the response. +func (r *Response) Complete(req Request) error { + r.UID = req.UID + + return nil +} + +// Handler can handle an SubjectAccessReview. +type Handler interface { + // Handle yields a response to an SubjectAccessReview. + // + // The supplied context is extracted from the received http.Request, allowing wrapping + // http.Handlers to inject values into and control cancelation of downstream request processing. + Handle(context.Context, Request) Response +} + +// HandlerFunc implements Handler interface using a single function. +type HandlerFunc func(context.Context, Request) Response + +var _ Handler = HandlerFunc(nil) + +// Handle process the SubjectAccessReview by invoking the underlying function. +func (f HandlerFunc) Handle(ctx context.Context, req Request) Response { + return f(ctx, req) +} + +// Webhook represents each individual webhook. +type Webhook struct { + // Handler actually processes an authorization request returning whether it was authenticated or unauthenticated, + // and potentially patches to apply to the handler. + Handler Handler + + // WithContextFunc will allow you to take the http.Request.Context() and + // add any additional information such as passing the request path or + // headers thus allowing you to read them from within the handler + WithContextFunc func(context.Context, *http.Request) context.Context + + log logr.Logger +} + +// InjectLogger gets a handle to a logging instance, hopefully with more info about this particular webhook. +func (w *Webhook) InjectLogger(l logr.Logger) error { + w.log = l + return nil +} + +// Handle processes SubjectAccessReview. +func (w *Webhook) Handle(ctx context.Context, req Request) Response { + resp := w.Handler.Handle(ctx, req) + if err := resp.Complete(req); err != nil { + w.log.Error(err, "unable to encode response") + return Errored(errUnableToEncodeResponse) + } + + return resp +} + +// InjectFunc injects the field setter into the webhook. +func (w *Webhook) InjectFunc(f inject.Func) error { + // inject directly into the handlers. It would be more correct + // to do this in a sync.Once in Handle (since we don't have some + // other start/finalize-type method), but it's more efficient to + // do it here, presumably. + + var setFields inject.Func + setFields = func(target interface{}) error { + if err := f(target); err != nil { + return err + } + + if _, err := inject.InjectorInto(setFields, target); err != nil { + return err + } + + return nil + } + + return setFields(w.Handler) +} diff --git a/pkg/webhook/authorization/webhook_test.go b/pkg/webhook/authorization/webhook_test.go new file mode 100644 index 0000000000..c573669bf4 --- /dev/null +++ b/pkg/webhook/authorization/webhook_test.go @@ -0,0 +1,138 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package authorization + +import ( + "context" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + authorizationv1 "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + machinerytypes "k8s.io/apimachinery/pkg/types" + + logf "sigs.k8s.io/controller-runtime/pkg/internal/log" + "sigs.k8s.io/controller-runtime/pkg/runtime/inject" +) + +var _ = Describe("Authorization Webhooks", func() { + allowHandler := func() *Webhook { + handler := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + return Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: true, + }, + }, + } + }, + } + webhook := &Webhook{ + Handler: handler, + log: logf.RuntimeLog.WithName("webhook"), + } + + return webhook + } + + It("should invoke the handler to get a response", func() { + By("setting up a webhook with an allow handler") + webhook := allowHandler() + + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{}) + + By("checking that it allowed the request") + Expect(resp.Status.Allowed).To(BeTrue()) + }) + + It("should ensure that the response's UID is set to the request's UID", func() { + By("setting up a webhook") + webhook := allowHandler() + + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{SubjectAccessReview: authorizationv1.SubjectAccessReview{ObjectMeta: metav1.ObjectMeta{UID: "foobar"}}}) + + By("checking that the response share's the request's UID") + Expect(resp.UID).To(Equal(machinerytypes.UID("foobar"))) + }) + + It("should populate the status on a response if one is not provided", func() { + By("setting up a webhook") + webhook := allowHandler() + + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{}) + + By("checking that the response share's the request's UID") + Expect(resp.Status).To(Equal(authorizationv1.SubjectAccessReviewStatus{Allowed: true})) + }) + + It("shouldn't overwrite the status on a response", func() { + By("setting up a webhook that sets a status") + webhook := &Webhook{ + Handler: HandlerFunc(func(ctx context.Context, req Request) Response { + return Response{ + SubjectAccessReview: authorizationv1.SubjectAccessReview{ + Status: authorizationv1.SubjectAccessReviewStatus{ + Allowed: true, + EvaluationError: "Ground Control to Major Tom", + }, + }, + } + }), + log: logf.RuntimeLog.WithName("webhook"), + } + + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{}) + + By("checking that the message is intact") + Expect(resp.Status).NotTo(BeNil()) + Expect(resp.Status.Allowed).To(BeTrue()) + Expect(resp.Status.EvaluationError).To(Equal("Ground Control to Major Tom")) + }) + + Describe("dependency injection", func() { + It("should set dependencies passed in on the handler", func() { + By("setting up a webhook and injecting it with a injection func that injects a string") + setFields := func(target interface{}) error { + inj, ok := target.(stringInjector) + if !ok { + return nil + } + + return inj.InjectString("something") + } + handler := &fakeHandler{} + webhook := &Webhook{ + Handler: handler, + log: logf.RuntimeLog.WithName("webhook"), + } + Expect(setFields(webhook)).To(Succeed()) + Expect(inject.InjectorInto(setFields, webhook)).To(BeTrue()) + + By("checking that the string was injected") + Expect(handler.injectedString).To(Equal("something")) + }) + }) +}) + +type stringInjector interface { + InjectString(s string) error +} From 2b185d86977ab3018d28f60138f480b737048259 Mon Sep 17 00:00:00 2001 From: Rafael Franzke Date: Tue, 18 May 2021 11:09:17 +0200 Subject: [PATCH 2/7] Add example program for SubjectAccessReviews --- examples/subjectaccessreview/main.go | 58 +++++++++++++++++++ .../subjectaccessreview.go | 38 ++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 examples/subjectaccessreview/main.go create mode 100644 examples/subjectaccessreview/subjectaccessreview.go diff --git a/examples/subjectaccessreview/main.go b/examples/subjectaccessreview/main.go new file mode 100644 index 0000000000..3a527c9cdf --- /dev/null +++ b/examples/subjectaccessreview/main.go @@ -0,0 +1,58 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "os" + + _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" + "sigs.k8s.io/controller-runtime/pkg/client/config" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" + "sigs.k8s.io/controller-runtime/pkg/webhook/authorization" +) + +func init() { + log.SetLogger(zap.New()) +} + +func main() { + entryLog := log.Log.WithName("entrypoint") + + // Setup a Manager + entryLog.Info("setting up manager") + mgr, err := manager.New(config.GetConfigOrDie(), manager.Options{}) + if err != nil { + entryLog.Error(err, "unable to set up overall controller manager") + os.Exit(1) + } + + // Setup webhooks + entryLog.Info("setting up webhook server") + hookServer := mgr.GetWebhookServer() + + entryLog.Info("registering webhooks to the webhook server") + hookServer.Register("/validate-v1-subjectaccessreview", &authorization.Webhook{Handler: &authorizer{}}) + + entryLog.Info("starting manager") + if err := mgr.Start(signals.SetupSignalHandler()); err != nil { + entryLog.Error(err, "unable to run manager") + os.Exit(1) + } +} diff --git a/examples/subjectaccessreview/subjectaccessreview.go b/examples/subjectaccessreview/subjectaccessreview.go new file mode 100644 index 0000000000..3d500da528 --- /dev/null +++ b/examples/subjectaccessreview/subjectaccessreview.go @@ -0,0 +1,38 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "context" + + "sigs.k8s.io/controller-runtime/pkg/webhook/authorization" +) + +// authorizer validates subjectaccessreviews +type authorizer struct { +} + +// authorizer admits a request by the token. +func (a *authorizer) Handle(ctx context.Context, req authorization.Request) authorization.Response { + if req.Spec.User == "system:anonymous" { + return authorization.Denied("anonymous users are not allowed") + } + if req.Spec.User == "foo" { + return authorization.NoOpinion("I don't care if foo is authorized or not") + } + return authorization.Allowed() +} From c2f523da8d6f1fc336c2b9ab3ffbc69d9ede91bb Mon Sep 17 00:00:00 2001 From: Shingo Omura Date: Thu, 25 Jan 2024 01:53:01 +0900 Subject: [PATCH 3/7] do not use io/ioutil anymore --- pkg/webhook/authorization/http.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/webhook/authorization/http.go b/pkg/webhook/authorization/http.go index c60366d2ea..03d4b4a01f 100644 --- a/pkg/webhook/authorization/http.go +++ b/pkg/webhook/authorization/http.go @@ -21,7 +21,6 @@ import ( "errors" "fmt" "io" - "io/ioutil" "net/http" authorizationv1 "k8s.io/api/authorization/v1" @@ -60,7 +59,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { } defer r.Body.Close() - if body, err = ioutil.ReadAll(r.Body); err != nil { + if body, err = io.ReadAll(r.Body); err != nil { wh.log.Error(err, "unable to read the body from the incoming request") reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) From 93c9fa4e7782756981be577cc3d908895be8759d Mon Sep 17 00:00:00 2001 From: Shingo Omura Date: Thu, 25 Jan 2024 02:10:19 +0900 Subject: [PATCH 4/7] Migrate ginkgo v2 --- pkg/webhook/authorization/authorization_suite_test.go | 7 ++----- pkg/webhook/authorization/http_test.go | 2 +- pkg/webhook/authorization/response_test.go | 2 +- pkg/webhook/authorization/webhook_test.go | 2 +- 4 files changed, 5 insertions(+), 8 deletions(-) diff --git a/pkg/webhook/authorization/authorization_suite_test.go b/pkg/webhook/authorization/authorization_suite_test.go index ae0245029a..6cab0de706 100644 --- a/pkg/webhook/authorization/authorization_suite_test.go +++ b/pkg/webhook/authorization/authorization_suite_test.go @@ -19,22 +19,19 @@ package authorization import ( "testing" - . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "sigs.k8s.io/controller-runtime/pkg/envtest/printer" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" ) func TestAuthorizationWebhook(t *testing.T) { RegisterFailHandler(Fail) - suiteName := "Authorization Webhook Suite" - RunSpecsWithDefaultAndCustomReporters(t, suiteName, []Reporter{printer.NewlineReporter{}, printer.NewProwReporter(suiteName)}) } var _ = BeforeSuite(func(done Done) { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) close(done) -}, 60) +}) diff --git a/pkg/webhook/authorization/http_test.go b/pkg/webhook/authorization/http_test.go index 914c361edd..56c9c8ac82 100644 --- a/pkg/webhook/authorization/http_test.go +++ b/pkg/webhook/authorization/http_test.go @@ -24,7 +24,7 @@ import ( "net/http" "net/http/httptest" - . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" authorizationv1 "k8s.io/api/authorization/v1" diff --git a/pkg/webhook/authorization/response_test.go b/pkg/webhook/authorization/response_test.go index b9bd36e14f..df32fe576b 100644 --- a/pkg/webhook/authorization/response_test.go +++ b/pkg/webhook/authorization/response_test.go @@ -19,7 +19,7 @@ package authorization import ( "fmt" - . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" authorizationv1 "k8s.io/api/authorization/v1" ) diff --git a/pkg/webhook/authorization/webhook_test.go b/pkg/webhook/authorization/webhook_test.go index c573669bf4..d9236950a8 100644 --- a/pkg/webhook/authorization/webhook_test.go +++ b/pkg/webhook/authorization/webhook_test.go @@ -19,7 +19,7 @@ package authorization import ( "context" - . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" authorizationv1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" From d1351d060a7396efd0036d50c71fadb32499adb4 Mon Sep 17 00:00:00 2001 From: Shingo Omura Date: Thu, 25 Jan 2024 02:10:53 +0900 Subject: [PATCH 5/7] Remove pkg/inject --- pkg/webhook/authorization/doc.go | 6 --- pkg/webhook/authorization/http.go | 17 +++--- pkg/webhook/authorization/http_test.go | 20 +------ pkg/webhook/authorization/webhook.go | 64 +++++++++++++---------- pkg/webhook/authorization/webhook_test.go | 33 ------------ 5 files changed, 46 insertions(+), 94 deletions(-) diff --git a/pkg/webhook/authorization/doc.go b/pkg/webhook/authorization/doc.go index bea4fede0d..fca7777dc2 100644 --- a/pkg/webhook/authorization/doc.go +++ b/pkg/webhook/authorization/doc.go @@ -21,9 +21,3 @@ methods to implement authorization webhook handlers. See examples/subjectaccessreview/ for an example of authorization webhooks. */ package authorization - -import ( - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" -) - -var log = logf.RuntimeLog.WithName("authorization") diff --git a/pkg/webhook/authorization/http.go b/pkg/webhook/authorization/http.go index 03d4b4a01f..6bf2b41079 100644 --- a/pkg/webhook/authorization/http.go +++ b/pkg/webhook/authorization/http.go @@ -52,7 +52,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { var reviewResponse Response if r.Body == nil { err = errors.New("request body is empty") - wh.log.Error(err, "bad request") + wh.getLogger(nil).Error(err, "bad request") reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) return @@ -60,7 +60,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { defer r.Body.Close() if body, err = io.ReadAll(r.Body); err != nil { - wh.log.Error(err, "unable to read the body from the incoming request") + wh.getLogger(nil).Error(err, "unable to read the body from the incoming request") reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) return @@ -70,7 +70,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { contentType := r.Header.Get("Content-Type") if contentType != "application/json" { err = fmt.Errorf("contentType=%s, expected application/json", contentType) - wh.log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) + wh.getLogger(nil).Error(err, "unable to process a request with unknown content type") reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) return @@ -79,12 +79,12 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { // Decode request body into authorizationv1.SubjectAccessReviewSpec structure sar, actualTokRevGVK, err := wh.decodeRequestBody(body) if err != nil { - wh.log.Error(err, "unable to decode the request") + wh.getLogger(nil).Error(err, "unable to decode the request") reviewResponse = Errored(err) wh.writeResponse(w, reviewResponse) return } - wh.log.V(1).Info("received request", "UID", sar.UID, "kind", sar.Kind) + wh.getLogger(nil).V(5).Info("received request") reviewResponse = wh.Handle(ctx, Request{sar.SubjectAccessReview}) wh.writeResponseTyped(w, reviewResponse, actualTokRevGVK) @@ -113,13 +113,12 @@ func (wh *Webhook) writeResponseTyped(w io.Writer, response Response, subjRevGVK // writeSubjectAccessReviewResponse writes ar to w. func (wh *Webhook) writeSubjectAccessReviewResponse(w io.Writer, ar authorizationv1.SubjectAccessReview) { if err := json.NewEncoder(w).Encode(ar); err != nil { - wh.log.Error(err, "unable to encode the response") + wh.getLogger(nil).Error(err, "unable to encode the response") wh.writeResponse(w, Errored(err)) } res := ar - if log := wh.log; log.V(1).Enabled() { - log.V(1).Info("wrote response", "UID", res.UID, "authorized", res.Status.Allowed) - } + wh.getLogger(nil).V(5).Info("wrote response", "authorized", res.Status.Allowed) + return } diff --git a/pkg/webhook/authorization/http_test.go b/pkg/webhook/authorization/http_test.go index 56c9c8ac82..9ccdef489e 100644 --- a/pkg/webhook/authorization/http_test.go +++ b/pkg/webhook/authorization/http_test.go @@ -27,9 +27,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" authorizationv1 "k8s.io/api/authorization/v1" - - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) var _ = Describe("Authentication Webhooks", func() { @@ -48,8 +45,6 @@ var _ = Describe("Authentication Webhooks", func() { respRecorder = &httptest.ResponseRecorder{ Body: bytes.NewBuffer(nil), } - _, err := inject.LoggerInto(log.WithName("test-webhook"), webhook) - Expect(err).NotTo(HaveOccurred()) }) It("should return bad-request when given an empty body", func() { @@ -96,7 +91,6 @@ var _ = Describe("Authentication Webhooks", func() { } webhook := &Webhook{ Handler: &fakeHandler{}, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":true}} @@ -114,7 +108,6 @@ var _ = Describe("Authentication Webhooks", func() { } webhook := &Webhook{ Handler: &fakeHandler{}, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":true}} @@ -131,7 +124,6 @@ var _ = Describe("Authentication Webhooks", func() { } webhook := &Webhook{ Handler: &fakeHandler{}, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":true}} @@ -156,7 +148,6 @@ var _ = Describe("Authentication Webhooks", func() { return NoOpinion(ctx.Value(key).(string)) }, }, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,"reason":%q}} @@ -185,7 +176,6 @@ var _ = Describe("Authentication Webhooks", func() { WithContextFunc: func(ctx context.Context, r *http.Request) context.Context { return context.WithValue(ctx, key, r.Header["Content-Type"][0]) }, - log: logf.RuntimeLog.WithName("webhook"), } expected := fmt.Sprintf(`{%s,"metadata":{"creationTimestamp":null},"spec":{},"status":{"allowed":false,"reason":%q}} @@ -206,14 +196,8 @@ type nopCloser struct { func (nopCloser) Close() error { return nil } type fakeHandler struct { - invoked bool - fn func(context.Context, Request) Response - injectedString string -} - -func (h *fakeHandler) InjectString(s string) error { - h.injectedString = s - return nil + invoked bool + fn func(context.Context, Request) Response } func (h *fakeHandler) Handle(ctx context.Context, req Request) Response { diff --git a/pkg/webhook/authorization/webhook.go b/pkg/webhook/authorization/webhook.go index 95371d7bf6..da1e6b3322 100644 --- a/pkg/webhook/authorization/webhook.go +++ b/pkg/webhook/authorization/webhook.go @@ -20,11 +20,13 @@ import ( "context" "errors" "net/http" + "sync" "github.com/go-logr/logr" authorizationv1 "k8s.io/api/authorization/v1" + "k8s.io/klog/v2" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" + logf "sigs.k8s.io/controller-runtime/pkg/log" ) var ( @@ -85,45 +87,51 @@ type Webhook struct { // headers thus allowing you to read them from within the handler WithContextFunc func(context.Context, *http.Request) context.Context - log logr.Logger -} - -// InjectLogger gets a handle to a logging instance, hopefully with more info about this particular webhook. -func (w *Webhook) InjectLogger(l logr.Logger) error { - w.log = l - return nil + setupLogOnce sync.Once + log logr.Logger } // Handle processes SubjectAccessReview. -func (w *Webhook) Handle(ctx context.Context, req Request) Response { - resp := w.Handler.Handle(ctx, req) +func (wh *Webhook) Handle(ctx context.Context, req Request) Response { + resp := wh.Handler.Handle(ctx, req) if err := resp.Complete(req); err != nil { - w.log.Error(err, "unable to encode response") + wh.getLogger(&req).Error(err, "unable to encode response") return Errored(errUnableToEncodeResponse) } return resp } -// InjectFunc injects the field setter into the webhook. -func (w *Webhook) InjectFunc(f inject.Func) error { - // inject directly into the handlers. It would be more correct - // to do this in a sync.Once in Handle (since we don't have some - // other start/finalize-type method), but it's more efficient to - // do it here, presumably. - - var setFields inject.Func - setFields = func(target interface{}) error { - if err := f(target); err != nil { - return err +// getLogger constructs a logger from the injected log and LogConstructor. +func (wh *Webhook) getLogger(req *Request) logr.Logger { + wh.setupLogOnce.Do(func() { + if wh.log.GetSink() == nil { + wh.log = logf.Log.WithName("authentication") } + }) - if _, err := inject.InjectorInto(setFields, target); err != nil { - return err - } + return logConstructor(wh.log, req) +} - return nil +// logConstructor adds some commonly interesting fields to the given logger. +func logConstructor(base logr.Logger, req *Request) logr.Logger { + if req != nil { + logger := base.WithValues( + "user", req.Spec.User, + "requestID", req.UID, + ) + if req.Spec.ResourceAttributes != nil { + attr := req.Spec.ResourceAttributes + return logger.WithValues( + "object", klog.KRef(attr.Namespace, attr.Name), + "group", attr.Group, "version", attr.Version, "resource", attr.Resource, + ) + } + if req.Spec.NonResourceAttributes != nil { + attr := req.Spec.NonResourceAttributes + return logger.WithValues("path", attr.Path) + } + return base } - - return setFields(w.Handler) + return base } diff --git a/pkg/webhook/authorization/webhook_test.go b/pkg/webhook/authorization/webhook_test.go index d9236950a8..fade834cf3 100644 --- a/pkg/webhook/authorization/webhook_test.go +++ b/pkg/webhook/authorization/webhook_test.go @@ -24,9 +24,6 @@ import ( authorizationv1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" machinerytypes "k8s.io/apimachinery/pkg/types" - - logf "sigs.k8s.io/controller-runtime/pkg/internal/log" - "sigs.k8s.io/controller-runtime/pkg/runtime/inject" ) var _ = Describe("Authorization Webhooks", func() { @@ -44,7 +41,6 @@ var _ = Describe("Authorization Webhooks", func() { } webhook := &Webhook{ Handler: handler, - log: logf.RuntimeLog.WithName("webhook"), } return webhook @@ -96,7 +92,6 @@ var _ = Describe("Authorization Webhooks", func() { }, } }), - log: logf.RuntimeLog.WithName("webhook"), } By("invoking the webhook") @@ -107,32 +102,4 @@ var _ = Describe("Authorization Webhooks", func() { Expect(resp.Status.Allowed).To(BeTrue()) Expect(resp.Status.EvaluationError).To(Equal("Ground Control to Major Tom")) }) - - Describe("dependency injection", func() { - It("should set dependencies passed in on the handler", func() { - By("setting up a webhook and injecting it with a injection func that injects a string") - setFields := func(target interface{}) error { - inj, ok := target.(stringInjector) - if !ok { - return nil - } - - return inj.InjectString("something") - } - handler := &fakeHandler{} - webhook := &Webhook{ - Handler: handler, - log: logf.RuntimeLog.WithName("webhook"), - } - Expect(setFields(webhook)).To(Succeed()) - Expect(inject.InjectorInto(setFields, webhook)).To(BeTrue()) - - By("checking that the string was injected") - Expect(handler.injectedString).To(Equal("something")) - }) - }) }) - -type stringInjector interface { - InjectString(s string) error -} From 0e9f8f999bb7062a4d4048c30a72113591d581e8 Mon Sep 17 00:00:00 2001 From: Shingo Omura Date: Thu, 25 Jan 2024 02:23:27 +0900 Subject: [PATCH 6/7] cleanup variable assignment --- pkg/webhook/authorization/http.go | 32 ++++++++++++------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/pkg/webhook/authorization/http.go b/pkg/webhook/authorization/http.go index 6bf2b41079..75554d3302 100644 --- a/pkg/webhook/authorization/http.go +++ b/pkg/webhook/authorization/http.go @@ -42,37 +42,31 @@ func init() { var _ http.Handler = &Webhook{} func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { - var body []byte - var err error ctx := r.Context() if wh.WithContextFunc != nil { ctx = wh.WithContextFunc(ctx, r) } - var reviewResponse Response if r.Body == nil { - err = errors.New("request body is empty") + err := errors.New("request body is empty") wh.getLogger(nil).Error(err, "bad request") - reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, Errored(err)) return } defer r.Body.Close() - if body, err = io.ReadAll(r.Body); err != nil { + body, err := io.ReadAll(r.Body) + if err != nil { wh.getLogger(nil).Error(err, "unable to read the body from the incoming request") - reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, Errored(err)) return } // verify the content type is accurate - contentType := r.Header.Get("Content-Type") - if contentType != "application/json" { + if contentType := r.Header.Get("Content-Type"); contentType != "application/json" { err = fmt.Errorf("contentType=%s, expected application/json", contentType) wh.getLogger(nil).Error(err, "unable to process a request with unknown content type") - reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, Errored(err)) return } @@ -80,14 +74,14 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { sar, actualTokRevGVK, err := wh.decodeRequestBody(body) if err != nil { wh.getLogger(nil).Error(err, "unable to decode the request") - reviewResponse = Errored(err) - wh.writeResponse(w, reviewResponse) + wh.writeResponse(w, Errored(err)) return } - wh.getLogger(nil).V(5).Info("received request") + req := Request{} + req.SubjectAccessReview = sar.SubjectAccessReview + wh.getLogger(&req).V(5).Info("received request") - reviewResponse = wh.Handle(ctx, Request{sar.SubjectAccessReview}) - wh.writeResponseTyped(w, reviewResponse, actualTokRevGVK) + wh.writeResponseTyped(w, wh.Handle(ctx, req), actualTokRevGVK) } // writeResponse writes response to w generically, i.e. without encoding GVK information. @@ -118,8 +112,6 @@ func (wh *Webhook) writeSubjectAccessReviewResponse(w io.Writer, ar authorizatio } res := ar wh.getLogger(nil).V(5).Info("wrote response", "authorized", res.Status.Allowed) - - return } func (wh *Webhook) decodeRequestBody(body []byte) (unversionedSubjectAccessReview, *schema.GroupVersionKind, error) { From 3615595247169d0e2dc9829dd3c86cca48e87763 Mon Sep 17 00:00:00 2001 From: Shingo Omura Date: Thu, 25 Jan 2024 02:28:22 +0900 Subject: [PATCH 7/7] add missing NoBody handling --- pkg/webhook/authorization/http.go | 2 +- pkg/webhook/authorization/http_test.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/webhook/authorization/http.go b/pkg/webhook/authorization/http.go index 75554d3302..30042a7158 100644 --- a/pkg/webhook/authorization/http.go +++ b/pkg/webhook/authorization/http.go @@ -47,7 +47,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { ctx = wh.WithContextFunc(ctx, r) } - if r.Body == nil { + if r.Body == nil || r.Body == http.NoBody { err := errors.New("request body is empty") wh.getLogger(nil).Error(err, "bad request") wh.writeResponse(w, Errored(err)) diff --git a/pkg/webhook/authorization/http_test.go b/pkg/webhook/authorization/http_test.go index 9ccdef489e..0e5e18b70d 100644 --- a/pkg/webhook/authorization/http_test.go +++ b/pkg/webhook/authorization/http_test.go @@ -186,6 +186,19 @@ var _ = Describe("Authentication Webhooks", func() { webhook.ServeHTTP(respRecorder, req.WithContext(ctx)) Expect(respRecorder.Body.String()).To(Equal(expected)) }) + + It("should error when given a NoBody", func() { + req := &http.Request{ + Header: http.Header{"Content-Type": []string{"application/json"}}, + Method: http.MethodPost, + Body: http.NoBody, + } + + expected := `{"metadata":{"creationTimestamp":null},"spec":{},"status":{"user":{},"error":"request body is empty"}} + ` + webhook.ServeHTTP(respRecorder, req) + Expect(respRecorder.Body.String()).To(Equal(expected)) + }) }) })