Merge pull request #28281 from nhlfr/authorize-return-bool

Automatic merge from submit-queue

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

@@ -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) {
+func (alwaysAllowAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) {
-	return nil
+	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) {
+func (alwaysDenyAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) {
-	return errors.New("Everything is forbidden.")
+	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"

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 {
+	if authorized, _, _ := aaa.Authorize(nil); !authorized {
-		t.Errorf("AlwaysAllowAuthorizer.Authorize did not return nil. (%s)", result)
+		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.")
 	}
 }

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 {

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))
+		authorized, reason, err := a.Authorize(getAttribs.GetAttribs(req))
-		if err == nil {
+		if err != nil {
-			handler.ServeHTTP(w, req)
+			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)
 	})
 }
 

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.

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)
+		authorized, _, _ := a.Authorize(attr)
-		actualAllow := bool(err == nil)
+		if tc.ExpectAllow != authorized {
-		if tc.ExpectAllow != actualAllow {
 			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)
+		authorized, _, _ := a.Authorize(attr)
-		actualAllow := bool(err == nil)
+		if tc.ExpectAllow != authorized {
-		if tc.ExpectAllow != actualAllow {
 			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)
 		}
 	}
 }

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)
 }
 

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 {
+func (authzHandler unionAuthzHandler) Authorize(a authorizer.Attributes) (bool, string, error) {
-	var errlist []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)
 }

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")
+		return false, "", nil
-	} else {
-		return 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)
+	authorized, _, _ := authzHandler.Authorize(nil)
-	if err != nil {
+	if !authorized {
-		t.Errorf("Unexpected error: %v", err)
+		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)
+	authorized, _, _ := authzHandler.Authorize(nil)
-	if err != nil {
+	if !authorized {
-		t.Errorf("Unexpected error: %v", err)
+		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)
 	}

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 {
+		authorized, reason, err := s.auth.Authorize(attrs)
-			msg := fmt.Sprintf("Forbidden (user=%s, verb=%s, namespace=%s, resource=%s)", u.GetName(), attrs.GetVerb(), attrs.GetNamespace(), attrs.GetResource())
+		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

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) {
+		authorizeFunc: func(a authorizer.Attributes) (authorized bool, reason string, err error) {
-			return nil
+			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")

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 {

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 {
+			if authorized, _, _ := a.Authorize(attr); !authorized {
-				t.Errorf("case %d: incorrectly restricted %s: %T %v", i, attr, err, err)
+				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)
 			}
 		}

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 {
+		result.StatusCode(&statusCode)
-			return fmt.Errorf("Error contacting webhook: %d", 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 r.Status.Allowed, r.Status.Reason, nil
-		return nil
-	}
-	if r.Status.Reason != "" {
-		return errors.New(r.Status.Reason)
-	}
-	return errors.New("unauthorized")
 }

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 {
+		authorized, _, err := wh.Authorize(tt.attr)
-			t.Errorf("case %d: authorization failed: %v", i, err)
+		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 {
+	testWebhookCacheCases(t, serv, wh, attr, tests)
-		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.")
-	}
 	// 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 {
+	testWebhookCacheCases(t, serv, wh, attr, tests)
-		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.")
-	}
 }

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

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