From 6b3dde745f4f313785e162ab4647edb8d5939cee Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Tue, 4 Oct 2016 22:39:53 -0400 Subject: [PATCH] Allow webhook authorizer to use SubjectAccessReviewInterface --- plugin/pkg/auth/authorizer/webhook/webhook.go | 97 ++++++++----- .../auth/authorizer/webhook/webhook_test.go | 129 ++++++++++++------ 2 files changed, 150 insertions(+), 76 deletions(-) diff --git a/plugin/pkg/auth/authorizer/webhook/webhook.go b/plugin/pkg/auth/authorizer/webhook/webhook.go index 95645b88dd1..8c4efce6420 100644 --- a/plugin/pkg/auth/authorizer/webhook/webhook.go +++ b/plugin/pkg/auth/authorizer/webhook/webhook.go @@ -19,15 +19,15 @@ package webhook import ( "encoding/json" - "fmt" "time" "github.com/golang/glog" "k8s.io/kubernetes/pkg/api/unversioned" + "k8s.io/kubernetes/pkg/apis/authorization" "k8s.io/kubernetes/pkg/apis/authorization/v1beta1" "k8s.io/kubernetes/pkg/auth/authorizer" - "k8s.io/kubernetes/pkg/client/restclient" + authorizationclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/authorization/unversioned" "k8s.io/kubernetes/pkg/util/cache" "k8s.io/kubernetes/plugin/pkg/webhook" @@ -44,10 +44,16 @@ const retryBackoff = 500 * time.Millisecond var _ authorizer.Authorizer = (*WebhookAuthorizer)(nil) type WebhookAuthorizer struct { - *webhook.GenericWebhook - responseCache *cache.LRUExpireCache - authorizedTTL time.Duration - unauthorizedTTL time.Duration + subjectAccessReview authorizationclient.SubjectAccessReviewInterface + responseCache *cache.LRUExpireCache + authorizedTTL time.Duration + unauthorizedTTL time.Duration + initialBackoff time.Duration +} + +// NewFromInterface creates a WebhookAuthorizer using the given subjectAccessReview client +func NewFromInterface(subjectAccessReview authorizationclient.SubjectAccessReviewInterface, authorizedTTL, unauthorizedTTL time.Duration) (*WebhookAuthorizer, error) { + return newWithBackoff(subjectAccessReview, authorizedTTL, unauthorizedTTL, retryBackoff) } // New creates a new WebhookAuthorizer from the provided kubeconfig file. @@ -71,16 +77,22 @@ type WebhookAuthorizer struct { // For additional HTTP configuration, refer to the kubeconfig documentation // http://kubernetes.io/v1.1/docs/user-guide/kubeconfig-file.html. func New(kubeConfigFile string, authorizedTTL, unauthorizedTTL time.Duration) (*WebhookAuthorizer, error) { - return newWithBackoff(kubeConfigFile, authorizedTTL, unauthorizedTTL, retryBackoff) -} - -// newWithBackoff allows tests to skip the sleep. -func newWithBackoff(kubeConfigFile string, authorizedTTL, unauthorizedTTL, initialBackoff time.Duration) (*WebhookAuthorizer, error) { - gw, err := webhook.NewGenericWebhook(kubeConfigFile, groupVersions, initialBackoff) + subjectAccessReview, err := subjectAccessReviewInterfaceFromKubeconfig(kubeConfigFile) if err != nil { return nil, err } - return &WebhookAuthorizer{gw, cache.NewLRUExpireCache(1024), authorizedTTL, unauthorizedTTL}, nil + return newWithBackoff(subjectAccessReview, authorizedTTL, unauthorizedTTL, retryBackoff) +} + +// newWithBackoff allows tests to skip the sleep. +func newWithBackoff(subjectAccessReview authorizationclient.SubjectAccessReviewInterface, authorizedTTL, unauthorizedTTL, initialBackoff time.Duration) (*WebhookAuthorizer, error) { + return &WebhookAuthorizer{ + subjectAccessReview: subjectAccessReview, + responseCache: cache.NewLRUExpireCache(1024), + authorizedTTL: authorizedTTL, + unauthorizedTTL: unauthorizedTTL, + initialBackoff: initialBackoff, + }, nil } // Authorize makes a REST request to the remote service describing the attempted action as a JSON @@ -128,9 +140,9 @@ func newWithBackoff(kubeConfigFile string, authorizedTTL, unauthorizedTTL, initi // } // func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bool, reason string, err error) { - r := &v1beta1.SubjectAccessReview{} + r := &authorization.SubjectAccessReview{} if user := attr.GetUser(); user != nil { - r.Spec = v1beta1.SubjectAccessReviewSpec{ + r.Spec = authorization.SubjectAccessReviewSpec{ User: user.GetName(), Groups: user.GetGroups(), Extra: convertToSARExtra(user.GetExtra()), @@ -138,7 +150,7 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo } if attr.IsResourceRequest() { - r.Spec.ResourceAttributes = &v1beta1.ResourceAttributes{ + r.Spec.ResourceAttributes = &authorization.ResourceAttributes{ Namespace: attr.GetNamespace(), Verb: attr.GetVerb(), Group: attr.GetAPIGroup(), @@ -148,7 +160,7 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo Name: attr.GetName(), } } else { - r.Spec.NonResourceAttributes = &v1beta1.NonResourceAttributes{ + r.Spec.NonResourceAttributes = &authorization.NonResourceAttributes{ Path: attr.GetPath(), Verb: attr.GetVerb(), } @@ -158,26 +170,22 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo return false, "", err } if entry, ok := w.responseCache.Get(string(key)); ok { - r.Status = entry.(v1beta1.SubjectAccessReviewStatus) + r.Status = entry.(authorization.SubjectAccessReviewStatus) } else { - result := w.WithExponentialBackoff(func() restclient.Result { - return w.RestClient.Post().Body(r).Do() + var ( + result *authorization.SubjectAccessReview + err error + ) + webhook.WithExponentialBackoff(w.initialBackoff, func() error { + result, err = w.subjectAccessReview.Create(r) + return err }) - if err := result.Error(); err != nil { + if err != nil { // An error here indicates bad configuration or an outage. Log for debugging. glog.Errorf("Failed to make webhook authorizer request: %v", err) return false, "", err } - var statusCode int - result.StatusCode(&statusCode) - switch { - case statusCode < 200, - statusCode >= 300: - return false, "", fmt.Errorf("Error contacting webhook: %d", statusCode) - } - if err := result.Into(r); err != nil { - return false, "", err - } + r.Status = result.Status if r.Status.Allowed { w.responseCache.Add(string(key), r.Status, w.authorizedTTL) } else { @@ -187,14 +195,35 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo return r.Status.Allowed, r.Status.Reason, nil } -func convertToSARExtra(extra map[string][]string) map[string]v1beta1.ExtraValue { +func convertToSARExtra(extra map[string][]string) map[string]authorization.ExtraValue { if extra == nil { return nil } - ret := map[string]v1beta1.ExtraValue{} + ret := map[string]authorization.ExtraValue{} for k, v := range extra { - ret[k] = v1beta1.ExtraValue(v) + ret[k] = authorization.ExtraValue(v) } return ret } + +// subjectAccessReviewInterfaceFromKubeconfig builds a client from the specified kubeconfig file, +// and returns a SubjectAccessReviewInterface that uses that client. Note that the client submits SubjectAccessReview +// requests to the exact path specified in the kubeconfig file, so arbitrary non-API servers can be targeted. +func subjectAccessReviewInterfaceFromKubeconfig(kubeConfigFile string) (authorizationclient.SubjectAccessReviewInterface, error) { + gw, err := webhook.NewGenericWebhook(kubeConfigFile, groupVersions, 0) + if err != nil { + return nil, err + } + return &subjectAccessReviewClient{gw}, nil +} + +type subjectAccessReviewClient struct { + w *webhook.GenericWebhook +} + +func (t *subjectAccessReviewClient) Create(subjectAccessReview *authorization.SubjectAccessReview) (*authorization.SubjectAccessReview, error) { + result := &authorization.SubjectAccessReview{} + err := t.w.RestClient.Post().Body(subjectAccessReview).Do().Into(result) + return result, err +} diff --git a/plugin/pkg/auth/authorizer/webhook/webhook_test.go b/plugin/pkg/auth/authorizer/webhook/webhook_test.go index 703fc02c6f7..420eedbfe7d 100644 --- a/plugin/pkg/auth/authorizer/webhook/webhook_test.go +++ b/plugin/pkg/auth/authorizer/webhook/webhook_test.go @@ -24,6 +24,7 @@ import ( "io/ioutil" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "reflect" @@ -183,7 +184,11 @@ current-context: default return fmt.Errorf("failed to execute test template: %v", err) } // Create a new authorizer - _, err = newWithBackoff(p, 0, 0, 0) + sarClient, err := subjectAccessReviewInterfaceFromKubeconfig(p) + if err != nil { + return fmt.Errorf("error building sar client: %v", err) + } + _, err = newWithBackoff(sarClient, 0, 0, 0) return err }() if err != nil && !tt.wantErr { @@ -203,6 +208,7 @@ type Service interface { // NewTestServer wraps a Service as an httptest.Server. func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error) { + const webhookPath = "/testserver" var tlsConfig *tls.Config if cert != nil { cert, err := tls.X509KeyPair(cert, key) @@ -223,26 +229,44 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error } serveHTTP := func(w http.ResponseWriter, r *http.Request) { + if r.Method != "POST" { + http.Error(w, fmt.Sprintf("unexpected method: %v", r.Method), http.StatusMethodNotAllowed) + return + } + if r.URL.Path != webhookPath { + http.Error(w, fmt.Sprintf("unexpected path: %v", r.URL.Path), http.StatusNotFound) + return + } + var review v1beta1.SubjectAccessReview - if err := json.NewDecoder(r.Body).Decode(&review); err != nil { + bodyData, _ := ioutil.ReadAll(r.Body) + if err := json.Unmarshal(bodyData, &review); err != nil { http.Error(w, fmt.Sprintf("failed to decode body: %v", err), http.StatusBadRequest) return } + + // ensure we received the serialized review as expected + if review.APIVersion != "authorization.k8s.io/v1beta1" { + http.Error(w, fmt.Sprintf("wrong api version: %s", string(bodyData)), http.StatusBadRequest) + return + } + // once we have a successful request, always call the review to record that we were called + s.Review(&review) if s.HTTPStatusCode() < 200 || s.HTTPStatusCode() >= 300 { http.Error(w, "HTTP Error", s.HTTPStatusCode()) return } - s.Review(&review) type status struct { - Allowed bool `json:"allowed"` - Reason string `json:"reason"` + Allowed bool `json:"allowed"` + Reason string `json:"reason"` + EvaluationError string `json:"evaluationError"` } resp := struct { APIVersion string `json:"apiVersion"` Status status `json:"status"` }{ APIVersion: v1beta1.SchemeGroupVersion.String(), - Status: status{review.Status.Allowed, review.Status.Reason}, + Status: status{review.Status.Allowed, review.Status.Reason, review.Status.EvaluationError}, } w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(resp) @@ -251,6 +275,12 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error server := httptest.NewUnstartedServer(http.HandlerFunc(serveHTTP)) server.TLS = tlsConfig server.StartTLS() + + // Adjust the path to point to our custom path + serverURL, _ := url.Parse(server.URL) + serverURL.Path = webhookPath + server.URL = serverURL.String() + return server, nil } @@ -258,9 +288,11 @@ func NewTestServer(s Service, cert, key, caCert []byte) (*httptest.Server, error type mockService struct { allow bool statusCode int + called int } func (m *mockService) Review(r *v1beta1.SubjectAccessReview) { + m.called++ r.Status.Allowed = m.allow } func (m *mockService) Allow() { m.allow = true } @@ -291,7 +323,11 @@ func newAuthorizer(callbackURL string, clientCert, clientKey, ca []byte, cacheTi if err := json.NewEncoder(tempfile).Encode(config); err != nil { return nil, err } - return newWithBackoff(p, cacheTime, cacheTime, 0) + sarClient, err := subjectAccessReviewInterfaceFromKubeconfig(p) + if err != nil { + return nil, fmt.Errorf("error building sar client: %v", err) + } + return newWithBackoff(sarClient, cacheTime, cacheTime, 0) } func TestTLSConfig(t *testing.T) { @@ -506,29 +542,36 @@ func TestWebhook(t *testing.T) { } type webhookCacheTestCase struct { - statusCode int + attr authorizer.AttributesRecord + + allow bool + statusCode int + expectedErr bool expectedAuthorized bool - expectedCached bool + expectedCalls int } -func testWebhookCacheCases(t *testing.T, serv *mockService, wh *WebhookAuthorizer, attr authorizer.AttributesRecord, tests []webhookCacheTestCase) { - for _, test := range tests { +func testWebhookCacheCases(t *testing.T, serv *mockService, wh *WebhookAuthorizer, tests []webhookCacheTestCase) { + for i, test := range tests { + serv.called = 0 + serv.allow = test.allow serv.statusCode = test.statusCode - authorized, _, err := wh.Authorize(attr) + authorized, _, err := wh.Authorize(test.attr) if test.expectedErr && err == nil { - t.Errorf("Expected error") + t.Errorf("%d: Expected error", i) + continue } else if !test.expectedErr && err != nil { - t.Fatal(err) + t.Errorf("%d: unexpected error: %v", i, err) + continue } - if test.expectedAuthorized && !authorized { - if test.expectedCached { - t.Errorf("Webhook should have successful response cached, but authorizer reported unauthorized.") - } else { - t.Errorf("Webhook returned HTTP %d, but authorizer reported unauthorized.", test.statusCode) - } - } else if !test.expectedAuthorized && authorized { - t.Errorf("Webhook returned HTTP %d, but authorizer reported success.", test.statusCode) + + if test.expectedAuthorized != authorized { + t.Errorf("%d: expected authorized=%v, got %v", i, test.expectedAuthorized, authorized) + } + + if test.expectedCalls != serv.called { + t.Errorf("%d: expected %d calls, got %d", i, test.expectedCalls, serv.called) } } } @@ -549,27 +592,29 @@ func TestWebhookCache(t *testing.T) { t.Fatal(err) } + aliceAttr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "alice"}} + bobAttr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "bob"}} + tests := []webhookCacheTestCase{ - {statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCached: false}, - {statusCode: 404, expectedErr: true, expectedAuthorized: false, expectedCached: false}, - {statusCode: 403, expectedErr: true, expectedAuthorized: false, expectedCached: false}, - {statusCode: 401, expectedErr: true, expectedAuthorized: false, expectedCached: false}, - {statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCached: false}, - {statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCached: true}, + // server error and 429's retry + {attr: aliceAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5}, + {attr: aliceAttr, allow: false, statusCode: 429, expectedErr: true, expectedAuthorized: false, expectedCalls: 5}, + // regular errors return errors but do not retry + {attr: aliceAttr, allow: false, statusCode: 404, expectedErr: true, expectedAuthorized: false, expectedCalls: 1}, + {attr: aliceAttr, allow: false, statusCode: 403, expectedErr: true, expectedAuthorized: false, expectedCalls: 1}, + {attr: aliceAttr, allow: false, statusCode: 401, expectedErr: true, expectedAuthorized: false, expectedCalls: 1}, + // successful responses are cached + {attr: aliceAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1}, + // later requests within the cache window don't hit the backend + {attr: aliceAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0}, + + // a request with different attributes doesn't hit the cache + {attr: bobAttr, allow: false, statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCalls: 5}, + // successful response for other attributes is cached + {attr: bobAttr, allow: true, statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCalls: 1}, + // later requests within the cache window don't hit the backend + {attr: bobAttr, allow: false, statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCalls: 0}, } - attr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "alice"}} - serv.allow = true - - testWebhookCacheCases(t, serv, wh, attr, tests) - - // For a different request, webhook should be called again. - tests = []webhookCacheTestCase{ - {statusCode: 500, expectedErr: true, expectedAuthorized: false, expectedCached: false}, - {statusCode: 200, expectedErr: false, expectedAuthorized: true, expectedCached: false}, - {statusCode: 500, expectedErr: false, expectedAuthorized: true, expectedCached: true}, - } - attr = authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "bob"}} - - testWebhookCacheCases(t, serv, wh, attr, tests) + testWebhookCacheCases(t, serv, wh, tests) }