diff --git a/pkg/apis/authorization/types.go b/pkg/apis/authorization/types.go index c4140b6d1cc..e798ec2123e 100644 --- a/pkg/apis/authorization/types.go +++ b/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. diff --git a/pkg/registry/authorization/localsubjectaccessreview/rest.go b/pkg/registry/authorization/localsubjectaccessreview/rest.go index e00b32bb70b..af49b6bd4d9 100644 --- a/pkg/registry/authorization/localsubjectaccessreview/rest.go +++ b/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 { diff --git a/pkg/registry/authorization/selfsubjectaccessreview/rest.go b/pkg/registry/authorization/selfsubjectaccessreview/rest.go index 5d917753a5b..38900a877f4 100644 --- a/pkg/registry/authorization/selfsubjectaccessreview/rest.go +++ b/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 { diff --git a/pkg/registry/authorization/subjectaccessreview/rest.go b/pkg/registry/authorization/subjectaccessreview/rest.go index 10fd27b7035..92da3d34244 100644 --- a/pkg/registry/authorization/subjectaccessreview/rest.go +++ b/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 { diff --git a/pkg/registry/authorization/subjectaccessreview/rest_test.go b/pkg/registry/authorization/subjectaccessreview/rest_test.go index 977a4376007..ce882a8b4bf 100644 --- a/pkg/registry/authorization/subjectaccessreview/rest_test.go +++ b/pkg/registry/authorization/subjectaccessreview/rest_test.go @@ -33,25 +33,22 @@ import ( type fakeAuthorizer struct { attrs authorizer.Attributes - ok bool - reason string - err error + decision authorizer.Decision + reason string + err error } func (f *fakeAuthorizer) Authorize(attrs authorizer.Attributes) (authorizer.Decision, string, error) { f.attrs = attrs - if f.ok { - return authorizer.DecisionAllow, f.reason, f.err - } - return authorizer.DecisionNoOpinion, f.reason, f.err + return f.decision, f.reason, f.err } func TestCreate(t *testing.T) { testcases := map[string]struct { - spec authorizationapi.SubjectAccessReviewSpec - ok bool - reason string - err error + spec authorizationapi.SubjectAccessReviewSpec + decision authorizer.Decision + reason string + 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, - reason: "myreason", - err: errors.New("myerror"), + decision: authorizer.DecisionNoOpinion, + reason: "myreason", + 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, - reason: "allowed", - err: nil, + decision: authorizer.DecisionAllow, + reason: "allowed", + 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, - reason: "myreason", - err: errors.New("myerror"), + decision: authorizer.DecisionNoOpinion, + reason: "myreason", + 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, - reason: "allowed", - err: nil, + decision: authorizer.DecisionAllow, + reason: "allowed", + 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, - reason: tc.reason, - err: tc.err, + decision: tc.decision, + reason: tc.reason, + err: tc.err, } storage := NewREST(auth) diff --git a/staging/src/k8s.io/api/authorization/v1/types.go b/staging/src/k8s.io/api/authorization/v1/types.go index 23b5ae7051c..86b05c54e37 100644 --- a/staging/src/k8s.io/api/authorization/v1/types.go +++ b/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"` diff --git a/staging/src/k8s.io/api/authorization/v1beta1/types.go b/staging/src/k8s.io/api/authorization/v1beta1/types.go index f62f59569ca..618ff8c0f1e 100644 --- a/staging/src/k8s.io/api/authorization/v1beta1/types.go +++ b/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"` diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go b/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go index 83577496eed..e9efac307c6 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go +++ b/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 -// error. We are failing open now to preserve backwards compatible behavior. -// Fix this after deprecation. +// TODO(mikedanese): We should eventually support failing closed when we +// encounter an error. We are failing open now to preserve backwards compatible +// 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 }