diff --git a/pkg/apiserver/authz.go b/pkg/apiserver/authz.go index e9deaad529132..7a19f2d4fcd77 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 1c474363d92e1..bc9b6ebba8f24 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 9f10c5c9cff58..7f778b7720df1 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 0b064da680c99..4f593127361a5 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 ca5ef6c633aa7..75db334e819bd 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 4a56b8dc39663..0070fbdea329c 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 c44fd4a2b306e..e23ea45960b99 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 7adbaa512b481..880d8c79eb394 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 32bfbcd5a3593..d6841188ca398 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 cfb3600b5ba7c..38b4dad0b0f2d 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 1bc04481f5131..4674a6ae4e897 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 087e564a988f3..bfcb49d16c19d 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 7aaf104dc5bd5..6eefddf0adda9 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 621c72ec9c235..1b4b463e2d427 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 7210287dfe3a0..613e3db2dc0ef 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. - 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.") + 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) } diff --git a/test/integration/auth/auth_test.go b/test/integration/auth/auth_test.go index d0e23fc2ad5f7..4370becd0cbca 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 f6a38824c0a64..553f40438c2ee 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