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 kubernetes#27974
lavalamp · Jul 18, 2016 · fa0dd46 · fa0dd46
1 parent 524c5b5
commit fa0dd46
Show file tree

Hide file tree

Showing 17 changed files with 257 additions and 127 deletions.
diff --git 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
@@ -24,16 +24,16 @@ 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.")
 	}
 }
 
 // NewAlwaysDenyAuthorizer must return a struct which implements authorizer.Authorizer
 // 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -17,7 +17,7 @@ limitations under the License.
 package union
 
 import (
-	"errors"
+	"fmt"
 	"testing"
 
 	"k8s.io/kubernetes/pkg/auth/authorizer"
@@ -28,25 +28,24 @@ 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) {
 	handler1 := &mockAuthzHandler{isAuthorized: false}
 	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
@@ -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