add deny to SAR API

pkg/apis/authorization/types.go

@@ -140,8 +140,13 @@ type SelfSubjectAccessReviewSpec struct {
 
 // SubjectAccessReviewStatus
 type SubjectAccessReviewStatus struct {
-	// Allowed is required.  True if the action would be allowed, false otherwise.
+	// Allowed is required. True if the action would be allowed, false otherwise.
 	Allowed bool
+	// Denied is optional. True if the action would be denied, otherwise
+	// false. If both allowed is false and denied is false, then the
+	// authorizer has no opinion on whether to authorize the action. Denied
+	// may not be true if Allowed is true.
+	Denied bool
 	// Reason is optional.  It indicates why a request was allowed or denied.
 	Reason string
 	// EvaluationError is an indication that some error occurred during the authorization check.

pkg/registry/authorization/localsubjectaccessreview/rest.go

@@ -62,6 +62,7 @@ func (r *REST) Create(ctx genericapirequest.Context, obj runtime.Object, createV
 
 	localSubjectAccessReview.Status = authorizationapi.SubjectAccessReviewStatus{
 		Allowed: (decision == authorizer.DecisionAllow),
+		Denied:  (decision == authorizer.DecisionDeny),
 		Reason:  reason,
 	}
 	if evaluationErr != nil {

pkg/registry/authorization/selfsubjectaccessreview/rest.go

@@ -65,6 +65,7 @@ func (r *REST) Create(ctx genericapirequest.Context, obj runtime.Object, createV
 
 	selfSAR.Status = authorizationapi.SubjectAccessReviewStatus{
 		Allowed: (decision == authorizer.DecisionAllow),
+		Denied:  (decision == authorizer.DecisionDeny),
 		Reason:  reason,
 	}
 	if evaluationErr != nil {

pkg/registry/authorization/subjectaccessreview/rest.go

@@ -55,6 +55,7 @@ func (r *REST) Create(ctx genericapirequest.Context, obj runtime.Object, createV
 
 	subjectAccessReview.Status = authorizationapi.SubjectAccessReviewStatus{
 		Allowed: (decision == authorizer.DecisionAllow),
+		Denied:  (decision == authorizer.DecisionDeny),
 		Reason:  reason,
 	}
 	if evaluationErr != nil {

pkg/registry/authorization/subjectaccessreview/rest_test.go

@@ -33,25 +33,22 @@ import (
 type fakeAuthorizer struct {
 	attrs authorizer.Attributes
 
-	ok     bool
+	decision authorizer.Decision
-	reason string
+	reason   string
-	err    error
+	err      error
 }
 
 func (f *fakeAuthorizer) Authorize(attrs authorizer.Attributes) (authorizer.Decision, string, error) {
 	f.attrs = attrs
-	if f.ok {
+	return f.decision, f.reason, f.err
-		return authorizer.DecisionAllow, f.reason, f.err
-	}
-	return authorizer.DecisionNoOpinion, f.reason, f.err
 }
 
 func TestCreate(t *testing.T) {
 	testcases := map[string]struct {
-		spec   authorizationapi.SubjectAccessReviewSpec
+		spec     authorizationapi.SubjectAccessReviewSpec
-		ok     bool
+		decision authorizer.Decision
-		reason string
+		reason   string
-		err    error
+		err      error
 
 		expectedErr    string
 		expectedAttrs  authorizer.Attributes
@@ -66,9 +63,9 @@ func TestCreate(t *testing.T) {
 				User: "bob",
 				NonResourceAttributes: &authorizationapi.NonResourceAttributes{Verb: "get", Path: "/mypath"},
 			},
-			ok:     false,
+			decision: authorizer.DecisionNoOpinion,
-			reason: "myreason",
+			reason:   "myreason",
-			err:    errors.New("myerror"),
+			err:      errors.New("myerror"),
 			expectedAttrs: authorizer.AttributesRecord{
 				User:            &user.DefaultInfo{Name: "bob"},
 				Verb:            "get",
@@ -87,9 +84,9 @@ func TestCreate(t *testing.T) {
 				User: "bob",
 				NonResourceAttributes: &authorizationapi.NonResourceAttributes{Verb: "get", Path: "/mypath"},
 			},
-			ok:     true,
+			decision: authorizer.DecisionAllow,
-			reason: "allowed",
+			reason:   "allowed",
-			err:    nil,
+			err:      nil,
 			expectedAttrs: authorizer.AttributesRecord{
 				User:            &user.DefaultInfo{Name: "bob"},
 				Verb:            "get",
@@ -116,9 +113,9 @@ func TestCreate(t *testing.T) {
 					Name:        "mydeployment",
 				},
 			},
-			ok:     false,
+			decision: authorizer.DecisionNoOpinion,
-			reason: "myreason",
+			reason:   "myreason",
-			err:    errors.New("myerror"),
+			err:      errors.New("myerror"),
 			expectedAttrs: authorizer.AttributesRecord{
 				User:            &user.DefaultInfo{Name: "bob"},
 				Namespace:       "myns",
@@ -132,6 +129,7 @@ func TestCreate(t *testing.T) {
 			},
 			expectedStatus: authorizationapi.SubjectAccessReviewStatus{
 				Allowed:         false,
+				Denied:          false,
 				Reason:          "myreason",
 				EvaluationError: "myerror",
 			},
@@ -150,9 +148,9 @@ func TestCreate(t *testing.T) {
 					Name:        "mydeployment",
 				},
 			},
-			ok:     true,
+			decision: authorizer.DecisionAllow,
-			reason: "allowed",
+			reason:   "allowed",
-			err:    nil,
+			err:      nil,
 			expectedAttrs: authorizer.AttributesRecord{
 				User:            &user.DefaultInfo{Name: "bob"},
 				Namespace:       "myns",
@@ -166,17 +164,34 @@ func TestCreate(t *testing.T) {
 			},
 			expectedStatus: authorizationapi.SubjectAccessReviewStatus{
 				Allowed:         true,
+				Denied:          false,
 				Reason:          "allowed",
 				EvaluationError: "",
 			},
 		},
+
+		"resource denied": {
+			spec: authorizationapi.SubjectAccessReviewSpec{
+				User:               "bob",
+				ResourceAttributes: &authorizationapi.ResourceAttributes{},
+			},
+			decision: authorizer.DecisionDeny,
+			expectedAttrs: authorizer.AttributesRecord{
+				User:            &user.DefaultInfo{Name: "bob"},
+				ResourceRequest: true,
+			},
+			expectedStatus: authorizationapi.SubjectAccessReviewStatus{
+				Allowed: false,
+				Denied:  true,
+			},
+		},
 	}
 
 	for k, tc := range testcases {
 		auth := &fakeAuthorizer{
-			ok:     tc.ok,
+			decision: tc.decision,
-			reason: tc.reason,
+			reason:   tc.reason,
-			err:    tc.err,
+			err:      tc.err,
 		}
 		storage := NewREST(auth)
 

staging/src/k8s.io/api/authorization/v1/types.go

@@ -169,8 +169,14 @@ type SelfSubjectAccessReviewSpec struct {
 
 // SubjectAccessReviewStatus
 type SubjectAccessReviewStatus struct {
-	// Allowed is required.  True if the action would be allowed, false otherwise.
+	// Allowed is required. True if the action would be allowed, false otherwise.
 	Allowed bool `json:"allowed" protobuf:"varint,1,opt,name=allowed"`
+	// Denied is optional. True if the action would be denied, otherwise
+	// false. If both allowed is false and denied is false, then the
+	// authorizer has no opinion on whether to authorize the action. Denied
+	// may not be true if Allowed is true.
+	// +optional
+	Denied bool `json:"denied,omitempty" protobuf:"varint,4,opt,name=denied"`
 	// Reason is optional.  It indicates why a request was allowed or denied.
 	// +optional
 	Reason string `json:"reason,omitempty" protobuf:"bytes,2,opt,name=reason"`

staging/src/k8s.io/api/authorization/v1beta1/types.go

@@ -169,8 +169,14 @@ type SelfSubjectAccessReviewSpec struct {
 
 // SubjectAccessReviewStatus
 type SubjectAccessReviewStatus struct {
-	// Allowed is required.  True if the action would be allowed, false otherwise.
+	// Allowed is required. True if the action would be allowed, false otherwise.
 	Allowed bool `json:"allowed" protobuf:"varint,1,opt,name=allowed"`
+	// Denied is optional. True if the action would be denied, otherwise
+	// false. If both allowed is false and denied is false, then the
+	// authorizer has no opinion on whether to authorize the action. Denied
+	// may not be true if Allowed is true.
+	// +optional
+	Denied bool `json:"denied,omitempty" protobuf:"varint,4,opt,name=denied"`
 	// Reason is optional.  It indicates why a request was allowed or denied.
 	// +optional
 	Reason string `json:"reason,omitempty" protobuf:"bytes,2,opt,name=reason"`

staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go

@@ -50,6 +50,7 @@ type WebhookAuthorizer struct {
 	authorizedTTL       time.Duration
 	unauthorizedTTL     time.Duration
 	initialBackoff      time.Duration
+	decisionOnError     authorizer.Decision
 }
 
 // NewFromInterface creates a WebhookAuthorizer using the given subjectAccessReview client
@@ -93,6 +94,7 @@ func newWithBackoff(subjectAccessReview authorizationclient.SubjectAccessReviewI
 		authorizedTTL:       authorizedTTL,
 		unauthorizedTTL:     unauthorizedTTL,
 		initialBackoff:      initialBackoff,
+		decisionOnError:     authorizer.DecisionNoOpinion,
 	}, nil
 }
 
@@ -140,9 +142,9 @@ func newWithBackoff(subjectAccessReview authorizationclient.SubjectAccessReviewI
 //       }
 //     }
 //
-// TODO(mikedanese): We should eventually fail closed when we encounter and
+// TODO(mikedanese): We should eventually support failing closed when we
-// error. We are failing open now to preserve backwards compatible behavior.
+// encounter an error. We are failing open now to preserve backwards compatible
-// Fix this after deprecation.
+// behavior.
 func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (decision authorizer.Decision, reason string, err error) {
 	r := &authorization.SubjectAccessReview{}
 	if user := attr.GetUser(); user != nil {
@@ -172,7 +174,7 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (decision auth
 	}
 	key, err := json.Marshal(r.Spec)
 	if err != nil {
-		return authorizer.DecisionNoOpinion, "", err
+		return w.decisionOnError, "", err
 	}
 	if entry, ok := w.responseCache.Get(string(key)); ok {
 		r.Status = entry.(authorization.SubjectAccessReviewStatus)
@@ -188,7 +190,7 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (decision auth
 		if err != nil {
 			// An error here indicates bad configuration or an outage. Log for debugging.
 			glog.Errorf("Failed to make webhook authorizer request: %v", err)
-			return authorizer.DecisionNoOpinion, "", err
+			return w.decisionOnError, "", err
 		}
 		r.Status = result.Status
 		if r.Status.Allowed {
@@ -197,9 +199,14 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (decision auth
 			w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL)
 		}
 	}
-	if r.Status.Allowed {
+	switch {
+	case r.Status.Denied && r.Status.Allowed:
+		return authorizer.DecisionDeny, r.Status.Reason, fmt.Errorf("webhook subject access review returned both allow and deny response")
+	case r.Status.Denied:
+		return authorizer.DecisionDeny, r.Status.Reason, nil
+	case r.Status.Allowed:
 		return authorizer.DecisionAllow, r.Status.Reason, nil
-	} else {
+	default:
 		return authorizer.DecisionNoOpinion, r.Status.Reason, nil
 	}