From fa0dd46ab7369dbca900273587a49268fb7dbaf0 Mon Sep 17 00:00:00 2001 From: Michal Rostecki Date: Wed, 29 Jun 2016 16:20:31 +0200 Subject: [PATCH] Return (bool, error) in Authorizer.Authorize() Before this change, Authorize() method was just returning an error, regardless of whether the user is unauthorized or whether there is some other unrelated error. Returning boolean with information about user authorization and error (which should be unrelated to the authorization) separately will make it easier to debug. Fixes #27974 --- pkg/apiserver/authz.go | 21 +++- pkg/apiserver/authz_test.go | 6 +- pkg/apiserver/errors.go | 7 ++ pkg/apiserver/handlers.go | 20 +++- pkg/auth/authorizer/abac/abac.go | 7 +- pkg/auth/authorizer/abac/abac_test.go | 14 ++- pkg/auth/authorizer/interfaces.go | 6 +- pkg/auth/authorizer/union/union.go | 22 +++- pkg/auth/authorizer/union/union_test.go | 36 ++++--- pkg/kubelet/server/server.go | 11 +- pkg/kubelet/server/server_test.go | 58 ++++++++-- plugin/pkg/auth/authorizer/rbac/rbac.go | 12 ++- plugin/pkg/auth/authorizer/rbac/rbac_test.go | 6 +- plugin/pkg/auth/authorizer/webhook/webhook.go | 24 ++--- .../auth/authorizer/webhook/webhook_test.go | 101 ++++++++++++------ test/integration/auth/auth_test.go | 23 ++-- .../serviceaccount/service_account_test.go | 10 +- 17 files changed, 257 insertions(+), 127 deletions(-) diff --git a/pkg/apiserver/authz.go b/pkg/apiserver/authz.go index e9deaad5291..7a19f2d4fcd 100644 --- a/pkg/apiserver/authz.go +++ b/pkg/apiserver/authz.go @@ -42,8 +42,8 @@ type Attributes struct { // It is useful in tests and when using kubernetes in an open manner. type alwaysAllowAuthorizer struct{} -func (alwaysAllowAuthorizer) Authorize(a authorizer.Attributes) (err error) { - return nil +func (alwaysAllowAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) { + return true, "", nil } func NewAlwaysAllowAuthorizer() authorizer.Authorizer { @@ -55,14 +55,27 @@ func NewAlwaysAllowAuthorizer() authorizer.Authorizer { // It is useful in unit tests to force an operation to be forbidden. type alwaysDenyAuthorizer struct{} -func (alwaysDenyAuthorizer) Authorize(a authorizer.Attributes) (err error) { - return errors.New("Everything is forbidden.") +func (alwaysDenyAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) { + return false, "Everything is forbidden.", nil } func NewAlwaysDenyAuthorizer() authorizer.Authorizer { return new(alwaysDenyAuthorizer) } +// alwaysFailAuthorizer is an implementation of authorizer.Attributes +// which always says no to an authorization request. +// It is useful in unit tests to force an operation to fail with error. +type alwaysFailAuthorizer struct{} + +func (alwaysFailAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) { + return false, "", errors.New("Authorization failure.") +} + +func NewAlwaysFailAuthorizer() authorizer.Authorizer { + return new(alwaysFailAuthorizer) +} + const ( ModeAlwaysAllow string = "AlwaysAllow" ModeAlwaysDeny string = "AlwaysDeny" diff --git a/pkg/apiserver/authz_test.go b/pkg/apiserver/authz_test.go index 1c474363d92..bc9b6ebba8f 100644 --- a/pkg/apiserver/authz_test.go +++ b/pkg/apiserver/authz_test.go @@ -24,8 +24,8 @@ import ( // and always return nil. func TestNewAlwaysAllowAuthorizer(t *testing.T) { aaa := NewAlwaysAllowAuthorizer() - if result := aaa.Authorize(nil); result != nil { - t.Errorf("AlwaysAllowAuthorizer.Authorize did not return nil. (%s)", result) + if authorized, _, _ := aaa.Authorize(nil); !authorized { + t.Errorf("AlwaysAllowAuthorizer.Authorize did not authorize successfully.") } } @@ -33,7 +33,7 @@ func TestNewAlwaysAllowAuthorizer(t *testing.T) { // and always return an error as everything is forbidden. func TestNewAlwaysDenyAuthorizer(t *testing.T) { ada := NewAlwaysDenyAuthorizer() - if result := ada.Authorize(nil); result == nil { + if authorized, _, _ := ada.Authorize(nil); authorized { t.Errorf("AlwaysDenyAuthorizer.Authorize returned nil instead of error.") } } diff --git a/pkg/apiserver/errors.go b/pkg/apiserver/errors.go index 9f10c5c9cff..7f778b7720d 100644 --- a/pkg/apiserver/errors.go +++ b/pkg/apiserver/errors.go @@ -88,6 +88,13 @@ func forbidden(w http.ResponseWriter, req *http.Request) { fmt.Fprintf(w, "Forbidden: %#v", req.RequestURI) } +// internalError renders a simple internal error +func internalError(w http.ResponseWriter, req *http.Request, err error) { + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprintf(w, "Internal Server Error: %#v", req.RequestURI) + runtime.HandleError(err) +} + // errAPIPrefixNotFound indicates that a RequestInfo resolution failed because the request isn't under // any known API prefixes type errAPIPrefixNotFound struct { diff --git a/pkg/apiserver/handlers.go b/pkg/apiserver/handlers.go index 0b064da680c..4f593127361 100644 --- a/pkg/apiserver/handlers.go +++ b/pkg/apiserver/handlers.go @@ -446,8 +446,13 @@ func WithImpersonation(handler http.Handler, requestContextMapper api.RequestCon actingAsAttributes.Name = name } - err := a.Authorize(actingAsAttributes) + authorized, reason, err := a.Authorize(actingAsAttributes) if err != nil { + internalError(w, req, err) + return + } + if !authorized { + glog.V(4).Infof("Forbidden: %#v, Reason: %s", req.RequestURI, reason) forbidden(w, req) return } @@ -480,12 +485,17 @@ func WithImpersonation(handler http.Handler, requestContextMapper api.RequestCon // WithAuthorizationCheck passes all authorized requests on to handler, and returns a forbidden error otherwise. func WithAuthorizationCheck(handler http.Handler, getAttribs RequestAttributeGetter, a authorizer.Authorizer) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - err := a.Authorize(getAttribs.GetAttribs(req)) - if err == nil { - handler.ServeHTTP(w, req) + authorized, reason, err := a.Authorize(getAttribs.GetAttribs(req)) + if err != nil { + internalError(w, req, err) return } - forbidden(w, req) + if !authorized { + glog.V(4).Infof("Forbidden: %#v, Reason: %s", req.RequestURI, reason) + forbidden(w, req) + return + } + handler.ServeHTTP(w, req) }) } diff --git a/pkg/auth/authorizer/abac/abac.go b/pkg/auth/authorizer/abac/abac.go index ca5ef6c633a..75db334e819 100644 --- a/pkg/auth/authorizer/abac/abac.go +++ b/pkg/auth/authorizer/abac/abac.go @@ -21,7 +21,6 @@ package abac import ( "bufio" - "errors" "fmt" "os" "strings" @@ -222,13 +221,13 @@ func resourceMatches(p api.Policy, a authorizer.Attributes) bool { } // Authorizer implements authorizer.Authorize -func (pl policyList) Authorize(a authorizer.Attributes) error { +func (pl policyList) Authorize(a authorizer.Attributes) (bool, string, error) { for _, p := range pl { if matches(*p, a) { - return nil + return true, "", nil } } - return errors.New("No policy matched.") + return false, "No policy matched.", nil // TODO: Benchmark how much time policy matching takes with a medium size // policy file, compared to other steps such as encoding/decoding. // Then, add Caching only if needed. diff --git a/pkg/auth/authorizer/abac/abac_test.go b/pkg/auth/authorizer/abac/abac_test.go index 4a56b8dc396..0070fbdea32 100644 --- a/pkg/auth/authorizer/abac/abac_test.go +++ b/pkg/auth/authorizer/abac/abac_test.go @@ -130,12 +130,11 @@ func TestAuthorizeV0(t *testing.T) { ResourceRequest: len(tc.NS) > 0 || len(tc.Resource) > 0, } - err := a.Authorize(attr) - actualAllow := bool(err == nil) - if tc.ExpectAllow != actualAllow { + authorized, _, _ := a.Authorize(attr) + if tc.ExpectAllow != authorized { t.Logf("tc: %v -> attr %v", tc, attr) t.Errorf("%d: Expected allowed=%v but actually allowed=%v\n\t%v", - i, tc.ExpectAllow, actualAllow, tc) + i, tc.ExpectAllow, authorized, tc) } } } @@ -250,11 +249,10 @@ func TestAuthorizeV1beta1(t *testing.T) { Path: tc.Path, } // t.Logf("tc %2v: %v -> attr %v", i, tc, attr) - err := a.Authorize(attr) - actualAllow := bool(err == nil) - if tc.ExpectAllow != actualAllow { + authorized, _, _ := a.Authorize(attr) + if tc.ExpectAllow != authorized { t.Errorf("%d: Expected allowed=%v but actually allowed=%v, for case %+v & %+v", - i, tc.ExpectAllow, actualAllow, tc, attr) + i, tc.ExpectAllow, authorized, tc, attr) } } } diff --git a/pkg/auth/authorizer/interfaces.go b/pkg/auth/authorizer/interfaces.go index c44fd4a2b30..e23ea45960b 100644 --- a/pkg/auth/authorizer/interfaces.go +++ b/pkg/auth/authorizer/interfaces.go @@ -67,12 +67,12 @@ type Attributes interface { // zero or more calls to methods of the Attributes interface. It returns nil when an action is // authorized, otherwise it returns an error. type Authorizer interface { - Authorize(a Attributes) (err error) + Authorize(a Attributes) (authorized bool, reason string, err error) } -type AuthorizerFunc func(a Attributes) error +type AuthorizerFunc func(a Attributes) (bool, string, error) -func (f AuthorizerFunc) Authorize(a Attributes) error { +func (f AuthorizerFunc) Authorize(a Attributes) (bool, string, error) { return f(a) } diff --git a/pkg/auth/authorizer/union/union.go b/pkg/auth/authorizer/union/union.go index 7adbaa512b4..880d8c79eb3 100644 --- a/pkg/auth/authorizer/union/union.go +++ b/pkg/auth/authorizer/union/union.go @@ -17,6 +17,8 @@ limitations under the License. package union import ( + "strings" + "k8s.io/kubernetes/pkg/auth/authorizer" utilerrors "k8s.io/kubernetes/pkg/util/errors" ) @@ -30,16 +32,26 @@ func New(authorizationHandlers ...authorizer.Authorizer) authorizer.Authorizer { } // Authorizes against a chain of authorizer.Authorizer objects and returns nil if successful and returns error if unsuccessful -func (authzHandler unionAuthzHandler) Authorize(a authorizer.Attributes) error { - var errlist []error +func (authzHandler unionAuthzHandler) Authorize(a authorizer.Attributes) (bool, string, error) { + var ( + errlist []error + reasonlist []string + ) for _, currAuthzHandler := range authzHandler { - err := currAuthzHandler.Authorize(a) + authorized, reason, err := currAuthzHandler.Authorize(a) + if err != nil { errlist = append(errlist, err) continue } - return nil + if !authorized { + if reason != "" { + reasonlist = append(reasonlist, reason) + } + continue + } + return true, reason, nil } - return utilerrors.NewAggregate(errlist) + return false, strings.Join(reasonlist, "\n"), utilerrors.NewAggregate(errlist) } diff --git a/pkg/auth/authorizer/union/union_test.go b/pkg/auth/authorizer/union/union_test.go index 32bfbcd5a35..d6841188ca3 100644 --- a/pkg/auth/authorizer/union/union_test.go +++ b/pkg/auth/authorizer/union/union_test.go @@ -17,7 +17,7 @@ limitations under the License. package union import ( - "errors" + "fmt" "testing" "k8s.io/kubernetes/pkg/auth/authorizer" @@ -28,15 +28,14 @@ type mockAuthzHandler struct { err error } -func (mock *mockAuthzHandler) Authorize(a authorizer.Attributes) error { +func (mock *mockAuthzHandler) Authorize(a authorizer.Attributes) (bool, string, error) { if mock.err != nil { - return mock.err + return false, "", mock.err } if !mock.isAuthorized { - return errors.New("Request unauthorized") - } else { - return nil + return false, "", nil } + return true, "", nil } func TestAuthorizationSecondPasses(t *testing.T) { @@ -44,9 +43,9 @@ func TestAuthorizationSecondPasses(t *testing.T) { handler2 := &mockAuthzHandler{isAuthorized: true} authzHandler := New(handler1, handler2) - err := authzHandler.Authorize(nil) - if err != nil { - t.Errorf("Unexpected error: %v", err) + authorized, _, _ := authzHandler.Authorize(nil) + if !authorized { + t.Errorf("Unexpected authorization failure") } } @@ -55,9 +54,9 @@ func TestAuthorizationFirstPasses(t *testing.T) { handler2 := &mockAuthzHandler{isAuthorized: false} authzHandler := New(handler1, handler2) - err := authzHandler.Authorize(nil) - if err != nil { - t.Errorf("Unexpected error: %v", err) + authorized, _, _ := authzHandler.Authorize(nil) + if !authorized { + t.Errorf("Unexpected authorization failure") } } @@ -66,7 +65,18 @@ func TestAuthorizationNonePasses(t *testing.T) { handler2 := &mockAuthzHandler{isAuthorized: false} authzHandler := New(handler1, handler2) - err := authzHandler.Authorize(nil) + authorized, _, _ := authzHandler.Authorize(nil) + if authorized { + t.Errorf("Expected failed authorization") + } +} + +func TestAuthorizationError(t *testing.T) { + handler1 := &mockAuthzHandler{err: fmt.Errorf("foo")} + handler2 := &mockAuthzHandler{err: fmt.Errorf("foo")} + authzHandler := New(handler1, handler2) + + _, _, err := authzHandler.Authorize(nil) if err == nil { t.Errorf("Expected error: %v", err) } diff --git a/pkg/kubelet/server/server.go b/pkg/kubelet/server/server.go index cfb3600b5ba..38b4dad0b0f 100644 --- a/pkg/kubelet/server/server.go +++ b/pkg/kubelet/server/server.go @@ -221,8 +221,15 @@ func (s *Server) InstallAuthFilter() { attrs := s.auth.GetRequestAttributes(u, req.Request) // Authorize - if err := s.auth.Authorize(attrs); err != nil { - msg := fmt.Sprintf("Forbidden (user=%s, verb=%s, namespace=%s, resource=%s)", u.GetName(), attrs.GetVerb(), attrs.GetNamespace(), attrs.GetResource()) + authorized, reason, err := s.auth.Authorize(attrs) + if err != nil { + msg := fmt.Sprintf("Error (user=%s, verb=%s, namespace=%s, resource=%s)", u.GetName(), attrs.GetVerb(), attrs.GetNamespace(), attrs.GetResource()) + glog.Errorf(msg, err) + resp.WriteErrorString(http.StatusInternalServerError, msg) + return + } + if !authorized { + msg := fmt.Sprintf("Forbidden (reason=%s, user=%s, verb=%s, namespace=%s, resource=%s)", reason, u.GetName(), attrs.GetVerb(), attrs.GetNamespace(), attrs.GetResource()) glog.V(2).Info(msg) resp.WriteErrorString(http.StatusForbidden, msg) return diff --git a/pkg/kubelet/server/server_test.go b/pkg/kubelet/server/server_test.go index 1bc04481f51..4674a6ae4e8 100644 --- a/pkg/kubelet/server/server_test.go +++ b/pkg/kubelet/server/server_test.go @@ -161,7 +161,7 @@ func (fk *fakeKubelet) ListVolumesForPod(podUID types.UID) (map[string]volume.Vo type fakeAuth struct { authenticateFunc func(*http.Request) (user.Info, bool, error) attributesFunc func(user.Info, *http.Request) authorizer.Attributes - authorizeFunc func(authorizer.Attributes) (err error) + authorizeFunc func(authorizer.Attributes) (authorized bool, reason string, err error) } func (f *fakeAuth) AuthenticateRequest(req *http.Request) (user.Info, bool, error) { @@ -170,7 +170,7 @@ func (f *fakeAuth) AuthenticateRequest(req *http.Request) (user.Info, bool, erro func (f *fakeAuth) GetRequestAttributes(u user.Info, req *http.Request) authorizer.Attributes { return f.attributesFunc(u, req) } -func (f *fakeAuth) Authorize(a authorizer.Attributes) (err error) { +func (f *fakeAuth) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) { return f.authorizeFunc(a) } @@ -204,8 +204,8 @@ func newServerTest() *serverTestFramework { attributesFunc: func(u user.Info, req *http.Request) authorizer.Attributes { return &authorizer.AttributesRecord{User: u} }, - authorizeFunc: func(a authorizer.Attributes) (err error) { - return nil + authorizeFunc: func(a authorizer.Attributes) (authorized bool, reason string, err error) { + return true, "", nil }, } server := NewServer( @@ -626,12 +626,12 @@ func TestAuthFilters(t *testing.T) { } return expectedAttributes } - fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (err error) { + fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (authorized bool, reason string, err error) { calledAuthorize = true if a != expectedAttributes { t.Fatalf("%s: expected attributes %v, got %v", tc.Path, expectedAttributes, a) } - return errors.New("Forbidden") + return false, "", nil } req, err := http.NewRequest(tc.Method, fw.testHTTPServer.URL+tc.Path, nil) @@ -665,6 +665,44 @@ func TestAuthFilters(t *testing.T) { } } +func TestAuthenticationError(t *testing.T) { + var ( + expectedUser = &user.DefaultInfo{Name: "test"} + expectedAttributes = &authorizer.AttributesRecord{User: expectedUser} + + calledAuthenticate = false + calledAuthorize = false + calledAttributes = false + ) + + fw := newServerTest() + defer fw.testHTTPServer.Close() + fw.fakeAuth.authenticateFunc = func(req *http.Request) (user.Info, bool, error) { + calledAuthenticate = true + return expectedUser, true, nil + } + fw.fakeAuth.attributesFunc = func(u user.Info, req *http.Request) authorizer.Attributes { + calledAttributes = true + return expectedAttributes + } + fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (authorized bool, reason string, err error) { + calledAuthorize = true + return false, "", errors.New("Failed") + } + + assertHealthFails(t, fw.testHTTPServer.URL+"/healthz", http.StatusInternalServerError) + + if !calledAuthenticate { + t.Fatalf("Authenticate was not called") + } + if !calledAttributes { + t.Fatalf("Attributes was not called") + } + if !calledAuthorize { + t.Fatalf("Authorize was not called") + } +} + func TestAuthenticationFailure(t *testing.T) { var ( expectedUser = &user.DefaultInfo{Name: "test"} @@ -685,9 +723,9 @@ func TestAuthenticationFailure(t *testing.T) { calledAttributes = true return expectedAttributes } - fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (err error) { + fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (authorized bool, reason string, err error) { calledAuthorize = true - return errors.New("not allowed") + return false, "", nil } assertHealthFails(t, fw.testHTTPServer.URL+"/healthz", http.StatusUnauthorized) @@ -723,9 +761,9 @@ func TestAuthorizationSuccess(t *testing.T) { calledAttributes = true return expectedAttributes } - fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (err error) { + fw.fakeAuth.authorizeFunc = func(a authorizer.Attributes) (authorized bool, reason string, err error) { calledAuthorize = true - return nil + return true, "", nil } assertHealthIsOk(t, fw.testHTTPServer.URL+"/healthz") diff --git a/plugin/pkg/auth/authorizer/rbac/rbac.go b/plugin/pkg/auth/authorizer/rbac/rbac.go index 087e564a988..bfcb49d16c1 100644 --- a/plugin/pkg/auth/authorizer/rbac/rbac.go +++ b/plugin/pkg/auth/authorizer/rbac/rbac.go @@ -34,9 +34,9 @@ type RBACAuthorizer struct { authorizationRuleResolver validation.AuthorizationRuleResolver } -func (r *RBACAuthorizer) Authorize(attr authorizer.Attributes) error { +func (r *RBACAuthorizer) Authorize(attr authorizer.Attributes) (bool, string, error) { if r.superUser != "" && attr.GetUser() != nil && attr.GetUser().GetName() == r.superUser { - return nil + return true, "", nil } ctx := api.WithNamespace(api.WithUser(api.NewContext(), attr.GetUser()), attr.GetNamespace()) @@ -57,7 +57,13 @@ func (r *RBACAuthorizer) Authorize(attr authorizer.Attributes) error { } } - return validation.ConfirmNoEscalation(ctx, r.authorizationRuleResolver, []rbac.PolicyRule{requestedRule}) + // TODO(nhlfr): Try to find more lightweight way to check attributes than escalation checks. + err := validation.ConfirmNoEscalation(ctx, r.authorizationRuleResolver, []rbac.PolicyRule{requestedRule}) + if err != nil { + return false, err.Error(), nil + } + + return true, "", nil } func New(roleRegistry role.Registry, roleBindingRegistry rolebinding.Registry, clusterRoleRegistry clusterrole.Registry, clusterRoleBindingRegistry clusterrolebinding.Registry, superUser string) *RBACAuthorizer { diff --git a/plugin/pkg/auth/authorizer/rbac/rbac_test.go b/plugin/pkg/auth/authorizer/rbac/rbac_test.go index 7aaf104dc5b..6eefddf0add 100644 --- a/plugin/pkg/auth/authorizer/rbac/rbac_test.go +++ b/plugin/pkg/auth/authorizer/rbac/rbac_test.go @@ -192,13 +192,13 @@ func TestAuthorizer(t *testing.T) { ruleResolver := validation.NewTestRuleResolver(tt.roles, tt.roleBindings, tt.clusterRoles, tt.clusterRoleBindings) a := RBACAuthorizer{tt.superUser, ruleResolver} for _, attr := range tt.shouldPass { - if err := a.Authorize(attr); err != nil { - t.Errorf("case %d: incorrectly restricted %s: %T %v", i, attr, err, err) + if authorized, _, _ := a.Authorize(attr); !authorized { + t.Errorf("case %d: incorrectly restricted %s", i, attr) } } for _, attr := range tt.shouldFail { - if err := a.Authorize(attr); err == nil { + if authorized, _, _ := a.Authorize(attr); authorized { t.Errorf("case %d: incorrectly passed %s", i, attr) } } diff --git a/plugin/pkg/auth/authorizer/webhook/webhook.go b/plugin/pkg/auth/authorizer/webhook/webhook.go index 621c72ec9c2..1b4b463e2d4 100644 --- a/plugin/pkg/auth/authorizer/webhook/webhook.go +++ b/plugin/pkg/auth/authorizer/webhook/webhook.go @@ -19,7 +19,6 @@ package webhook import ( "encoding/json" - "errors" "fmt" "time" @@ -128,7 +127,7 @@ func newWithBackoff(kubeConfigFile string, authorizedTTL, unauthorizedTTL, initi // } // } // -func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (err error) { +func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bool, reason string, err error) { r := &v1beta1.SubjectAccessReview{} if user := attr.GetUser(); user != nil { r.Spec = v1beta1.SubjectAccessReviewSpec{ @@ -156,7 +155,7 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (err error) { } key, err := json.Marshal(r.Spec) if err != nil { - return err + return false, "", err } if entry, ok := w.responseCache.Get(string(key)); ok { r.Status = entry.(v1beta1.SubjectAccessReviewStatus) @@ -167,14 +166,17 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (err error) { if err := result.Error(); 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 err + return false, "", err } var statusCode int - if result.StatusCode(&statusCode); statusCode < 200 || statusCode >= 300 { - return fmt.Errorf("Error contacting webhook: %d", statusCode) + 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 err + return false, "", err } if r.Status.Allowed { w.responseCache.Add(string(key), r.Status, w.authorizedTTL) @@ -182,11 +184,5 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (err error) { w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL) } } - if r.Status.Allowed { - return nil - } - if r.Status.Reason != "" { - return errors.New(r.Status.Reason) - } - return errors.New("unauthorized") + return r.Status.Allowed, r.Status.Reason, nil } diff --git a/plugin/pkg/auth/authorizer/webhook/webhook_test.go b/plugin/pkg/auth/authorizer/webhook/webhook_test.go index 7210287dfe3..613e3db2dc0 100644 --- a/plugin/pkg/auth/authorizer/webhook/webhook_test.go +++ b/plugin/pkg/auth/authorizer/webhook/webhook_test.go @@ -299,22 +299,25 @@ func TestTLSConfig(t *testing.T) { test string clientCert, clientKey, clientCA []byte serverCert, serverKey, serverCA []byte - wantErr bool + wantAuth, wantErr bool }{ { test: "TLS setup between client and server", clientCert: clientCert, clientKey: clientKey, clientCA: caCert, serverCert: serverCert, serverKey: serverKey, serverCA: caCert, + wantAuth: true, }, { test: "Server does not require client auth", clientCA: caCert, serverCert: serverCert, serverKey: serverKey, + wantAuth: true, }, { test: "Server does not require client auth, client provides it", clientCert: clientCert, clientKey: clientKey, clientCA: caCert, serverCert: serverCert, serverKey: serverKey, + wantAuth: true, }, { test: "Client does not trust server", @@ -357,7 +360,16 @@ func TestTLSConfig(t *testing.T) { // Allow all and see if we get an error. service.Allow() - err = wh.Authorize(attr) + authorized, _, err := wh.Authorize(attr) + if tt.wantAuth { + if !authorized { + t.Errorf("expected successful authorization") + } + } else { + if authorized { + t.Errorf("expected failed authorization") + } + } if tt.wantErr { if err == nil { t.Errorf("expected error making authorization request: %v", err) @@ -370,7 +382,7 @@ func TestTLSConfig(t *testing.T) { } service.Deny() - if err := wh.Authorize(attr); err == nil { + if authorized, _, _ := wh.Authorize(attr); authorized { t.Errorf("%s: incorrectly authorized with DenyAll policy", tt.test) } }() @@ -473,8 +485,12 @@ func TestWebhook(t *testing.T) { } for i, tt := range tests { - if err := wh.Authorize(tt.attr); err != nil { - t.Errorf("case %d: authorization failed: %v", i, err) + authorized, _, err := wh.Authorize(tt.attr) + if err != nil { + t.Fatal(err) + } + if !authorized { + t.Errorf("case %d: authorization failed", i) continue } @@ -489,6 +505,34 @@ func TestWebhook(t *testing.T) { } } +type webhookCacheTestCase struct { + statusCode int + expectedErr bool + expectedAuthorized bool + expectedCached bool +} + +func testWebhookCacheCases(t *testing.T, serv *mockService, wh *WebhookAuthorizer, attr authorizer.AttributesRecord, tests []webhookCacheTestCase) { + for _, test := range tests { + serv.statusCode = test.statusCode + authorized, _, err := wh.Authorize(attr) + if test.expectedErr && err == nil { + t.Errorf("Expected error") + } else if !test.expectedErr && err != nil { + t.Fatal(err) + } + 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) + } + } +} + // TestWebhookCache verifies that error responses from the server are not // cached, but successful responses are. func TestWebhookCache(t *testing.T) { @@ -505,36 +549,27 @@ func TestWebhookCache(t *testing.T) { t.Fatal(err) } + 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}, + } + attr := authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "alice"}} serv.allow = true - serv.statusCode = 500 - if err := wh.Authorize(attr); err == nil { - t.Errorf("Webhook returned HTTP 500, but authorizer reported success.") - } - serv.statusCode = 404 - if err := wh.Authorize(attr); err == nil { - t.Errorf("Webhook returned HTTP 404, but authorizer reported success.") - } - serv.statusCode = 200 - if err := wh.Authorize(attr); err != nil { - t.Errorf("Webhook returned HTTP 200, but authorizer reported unauthorized.") - } - serv.statusCode = 500 - if err := wh.Authorize(attr); err != nil { - t.Errorf("Webhook should have successful response cached, but authorizer reported unauthorized.") - } + + 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"}} - serv.statusCode = 500 - if err := wh.Authorize(attr); err == nil { - t.Errorf("Webhook returned HTTP 500, but authorizer reported success.") - } - serv.statusCode = 200 - if err := wh.Authorize(attr); err != nil { - t.Errorf("Webhook returned HTTP 200, but authorizer reported unauthorized.") - } - serv.statusCode = 500 - if err := wh.Authorize(attr); err != nil { - t.Errorf("Webhook should have successful response cached, but authorizer reported unauthorized.") - } + + testWebhookCacheCases(t, serv, wh, attr, tests) } diff --git a/test/integration/auth/auth_test.go b/test/integration/auth/auth_test.go index d0e23fc2ad5..4370becd0cb 100644 --- a/test/integration/auth/auth_test.go +++ b/test/integration/auth/auth_test.go @@ -25,7 +25,6 @@ package auth import ( "bytes" "encoding/json" - "errors" "fmt" "io/ioutil" "net/http" @@ -536,11 +535,11 @@ func TestAuthModeAlwaysDeny(t *testing.T) { // TODO(etune): remove this test once a more comprehensive built-in authorizer is implemented. type allowAliceAuthorizer struct{} -func (allowAliceAuthorizer) Authorize(a authorizer.Attributes) error { +func (allowAliceAuthorizer) Authorize(a authorizer.Attributes) (bool, string, error) { if a.GetUser() != nil && a.GetUser().GetName() == "alice" { - return nil + return true, "", nil } - return errors.New("I can't allow that. Go ask alice.") + return false, "I can't allow that. Go ask alice.", nil } // TestAliceNotForbiddenOrUnauthorized tests a user who is known to @@ -703,24 +702,24 @@ func TestUnknownUserIsUnauthorized(t *testing.T) { type impersonateAuthorizer struct{} // alice can't act as anyone and bob can't do anything but act-as someone -func (impersonateAuthorizer) Authorize(a authorizer.Attributes) error { +func (impersonateAuthorizer) Authorize(a authorizer.Attributes) (bool, string, error) { // alice can impersonate service accounts and do other actions if a.GetUser() != nil && a.GetUser().GetName() == "alice" && a.GetVerb() == "impersonate" && a.GetResource() == "serviceaccounts" { - return nil + return true, "", nil } if a.GetUser() != nil && a.GetUser().GetName() == "alice" && a.GetVerb() != "impersonate" { - return nil + return true, "", nil } // bob can impersonate anyone, but that it if a.GetUser() != nil && a.GetUser().GetName() == "bob" && a.GetVerb() == "impersonate" { - return nil + return true, "", nil } // service accounts can do everything if a.GetUser() != nil && strings.HasPrefix(a.GetUser().GetName(), serviceaccount.ServiceAccountUsernamePrefix) { - return nil + return true, "", nil } - return errors.New("I can't allow that. Go ask alice.") + return false, "I can't allow that. Go ask alice.", nil } func TestImpersonateIsForbidden(t *testing.T) { @@ -862,9 +861,9 @@ type trackingAuthorizer struct { requestAttributes []authorizer.Attributes } -func (a *trackingAuthorizer) Authorize(attributes authorizer.Attributes) error { +func (a *trackingAuthorizer) Authorize(attributes authorizer.Attributes) (bool, string, error) { a.requestAttributes = append(a.requestAttributes, attributes) - return nil + return true, "", nil } // TestAuthorizationAttributeDetermination tests that authorization attributes are built correctly diff --git a/test/integration/serviceaccount/service_account_test.go b/test/integration/serviceaccount/service_account_test.go index f6a38824c0a..553f40438c2 100644 --- a/test/integration/serviceaccount/service_account_test.go +++ b/test/integration/serviceaccount/service_account_test.go @@ -369,7 +369,7 @@ func startServiceAccountTestServer(t *testing.T) (*clientset.Clientset, restclie // 1. The "root" user is allowed to do anything // 2. ServiceAccounts named "ro" are allowed read-only operations in their namespace // 3. ServiceAccounts named "rw" are allowed any operation in their namespace - authorizer := authorizer.AuthorizerFunc(func(attrs authorizer.Attributes) error { + authorizer := authorizer.AuthorizerFunc(func(attrs authorizer.Attributes) (bool, string, error) { username := "" if user := attrs.GetUser(); user != nil { username = user.GetName() @@ -379,7 +379,7 @@ func startServiceAccountTestServer(t *testing.T) (*clientset.Clientset, restclie // If the user is "root"... if username == rootUserName { // allow them to do anything - return nil + return true, "", nil } // If the user is a service account... @@ -389,15 +389,15 @@ func startServiceAccountTestServer(t *testing.T) (*clientset.Clientset, restclie switch serviceAccountName { case readOnlyServiceAccountName: if attrs.IsReadOnly() { - return nil + return true, "", nil } case readWriteServiceAccountName: - return nil + return true, "", nil } } } - return fmt.Errorf("User %s is denied (ns=%s, readonly=%v, resource=%s)", username, ns, attrs.IsReadOnly(), attrs.GetResource()) + return false, fmt.Sprintf("User %s is denied (ns=%s, readonly=%v, resource=%s)", username, ns, attrs.IsReadOnly(), attrs.GetResource()), nil }) // Set up admission plugin to auto-assign serviceaccounts to pods