From 89a498de40b953780e31e1acf0abf4a1c77dbee1 Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Fri, 29 Sep 2017 14:21:08 -0700 Subject: [PATCH 1/4] refactor authorizer to return a tristate decision Kubernetes-commit: ee4d2d0a941b4298a3e07aab8fef5b3c5b85b27d --- pkg/authorization/authorizer/interfaces.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/pkg/authorization/authorizer/interfaces.go b/pkg/authorization/authorizer/interfaces.go index e94da3e1a..594109096 100644 --- a/pkg/authorization/authorizer/interfaces.go +++ b/pkg/authorization/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) (authorized bool, reason string, err error) + Authorize(a Attributes) (authorized Decision, reason string, err error) } -type AuthorizerFunc func(a Attributes) (bool, string, error) +type AuthorizerFunc func(a Attributes) (Decision, string, error) -func (f AuthorizerFunc) Authorize(a Attributes) (bool, string, error) { +func (f AuthorizerFunc) Authorize(a Attributes) (Decision, string, error) { return f(a) } @@ -144,3 +144,15 @@ func (a AttributesRecord) IsResourceRequest() bool { func (a AttributesRecord) GetPath() string { return a.Path } + +type Decision int + +const ( + // DecisionDeny means that an authorizer decided to deny the action. + DecisionDeny Decision = iota + // DecisionAllow means that an authorizer decided to allow the action. + DecisionAllow + // DecisionNoOpionion means that an authorizer has no opinion on wether + // to allow or deny an action. + DecisionNoOpinion +) From 06a5d258465e7e0f6da971aa274f8924f10b9fee Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Fri, 29 Sep 2017 14:21:40 -0700 Subject: [PATCH 2/4] move authorizers over to new interface Kubernetes-commit: 12125455d84c75562e6dd6a183762549adff747f --- pkg/admission/initializer/initializer_test.go | 4 ++-- .../plugin/initialization/initialization.go | 4 ++-- .../initialization/initialization_test.go | 6 ++--- .../authorizerfactory/authz_test.go | 8 +++---- .../authorizerfactory/builtin.go | 20 ++++++++-------- pkg/endpoints/filters/authorization.go | 2 +- pkg/endpoints/filters/impersonation.go | 4 ++-- pkg/endpoints/filters/impersonation_test.go | 24 +++++++++---------- pkg/server/genericapiserver_test.go | 4 ++-- plugin/pkg/authorizer/webhook/webhook.go | 16 +++++++++---- plugin/pkg/authorizer/webhook/webhook_test.go | 14 +++++------ 11 files changed, 57 insertions(+), 49 deletions(-) diff --git a/pkg/admission/initializer/initializer_test.go b/pkg/admission/initializer/initializer_test.go index cbf0206a2..43153b91c 100644 --- a/pkg/admission/initializer/initializer_test.go +++ b/pkg/admission/initializer/initializer_test.go @@ -133,8 +133,8 @@ var _ initializer.WantsAuthorizer = &WantAuthorizerAdmission{} // TestAuthorizer is a test stub that fulfills the WantsAuthorizer interface. type TestAuthorizer struct{} -func (t *TestAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) { - return false, "", nil +func (t *TestAuthorizer) Authorize(a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { + return authorizer.DecisionNoOpinion, "", nil } // wantClientCert is a test stub for testing that fulfulls the WantsClientCert interface. diff --git a/pkg/admission/plugin/initialization/initialization.go b/pkg/admission/plugin/initialization/initialization.go index e5adaf250..bf423ebb4 100644 --- a/pkg/admission/plugin/initialization/initialization.go +++ b/pkg/admission/plugin/initialization/initialization.go @@ -260,7 +260,7 @@ func (i *initializer) Admit(a admission.Attributes) (err error) { func (i *initializer) canInitialize(a admission.Attributes, message string) error { // caller must have the ability to mutate un-initialized resources - authorized, reason, err := i.authorizer.Authorize(authorizer.AttributesRecord{ + decision, reason, err := i.authorizer.Authorize(authorizer.AttributesRecord{ Name: a.GetName(), ResourceRequest: true, User: a.GetUserInfo(), @@ -273,7 +273,7 @@ func (i *initializer) canInitialize(a admission.Attributes, message string) erro if err != nil { return err } - if !authorized { + if decision != authorizer.DecisionAllow { return errors.NewForbidden(a.GetResource().GroupResource(), a.GetName(), fmt.Errorf("%s: %s", message, reason)) } return nil diff --git a/pkg/admission/plugin/initialization/initialization_test.go b/pkg/admission/plugin/initialization/initialization_test.go index 0832fe8d1..a3bb0991b 100644 --- a/pkg/admission/plugin/initialization/initialization_test.go +++ b/pkg/admission/plugin/initialization/initialization_test.go @@ -109,11 +109,11 @@ type fakeAuthorizer struct { accept bool } -func (f *fakeAuthorizer) Authorize(a authorizer.Attributes) (bool, string, error) { +func (f *fakeAuthorizer) Authorize(a authorizer.Attributes) (authorizer.Decision, string, error) { if f.accept { - return true, "", nil + return authorizer.DecisionAllow, "", nil } - return false, "denied", nil + return authorizer.DecisionNoOpinion, "denied", nil } func TestAdmitUpdate(t *testing.T) { diff --git a/pkg/authorization/authorizerfactory/authz_test.go b/pkg/authorization/authorizerfactory/authz_test.go index 73a428348..d1de3eba7 100644 --- a/pkg/authorization/authorizerfactory/authz_test.go +++ b/pkg/authorization/authorizerfactory/authz_test.go @@ -27,7 +27,7 @@ import ( // and always return nil. func TestNewAlwaysAllowAuthorizer(t *testing.T) { aaa := NewAlwaysAllowAuthorizer() - if authorized, _, _ := aaa.Authorize(nil); !authorized { + if decision, _, _ := aaa.Authorize(nil); decision != authorizer.DecisionAllow { t.Errorf("AlwaysAllowAuthorizer.Authorize did not authorize successfully.") } } @@ -36,7 +36,7 @@ func TestNewAlwaysAllowAuthorizer(t *testing.T) { // and always return an error as everything is forbidden. func TestNewAlwaysDenyAuthorizer(t *testing.T) { ada := NewAlwaysDenyAuthorizer() - if authorized, _, _ := ada.Authorize(nil); authorized { + if decision, _, _ := ada.Authorize(nil); decision == authorizer.DecisionAllow { t.Errorf("AlwaysDenyAuthorizer.Authorize returned nil instead of error.") } } @@ -47,10 +47,10 @@ func TestPrivilegedGroupAuthorizer(t *testing.T) { yes := authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"no", "allow-01"}}} no := authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"no", "deny-01"}}} - if authorized, _, _ := auth.Authorize(yes); !authorized { + if authorized, _, _ := auth.Authorize(yes); authorized != authorizer.DecisionAllow { t.Errorf("failed") } - if authorized, _, _ := auth.Authorize(no); authorized { + if authorized, _, _ := auth.Authorize(no); authorized == authorizer.DecisionAllow { t.Errorf("failed") } } diff --git a/pkg/authorization/authorizerfactory/builtin.go b/pkg/authorization/authorizerfactory/builtin.go index 8381e83f4..6f67d87b8 100644 --- a/pkg/authorization/authorizerfactory/builtin.go +++ b/pkg/authorization/authorizerfactory/builtin.go @@ -28,8 +28,8 @@ import ( // It is useful in tests and when using kubernetes in an open manner. type alwaysAllowAuthorizer struct{} -func (alwaysAllowAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) { - return true, "", nil +func (alwaysAllowAuthorizer) Authorize(a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { + return authorizer.DecisionAllow, "", nil } func (alwaysAllowAuthorizer) RulesFor(user user.Info, namespace string) ([]authorizer.ResourceRuleInfo, []authorizer.NonResourceRuleInfo, bool, error) { @@ -56,8 +56,8 @@ func NewAlwaysAllowAuthorizer() *alwaysAllowAuthorizer { // It is useful in unit tests to force an operation to be forbidden. type alwaysDenyAuthorizer struct{} -func (alwaysDenyAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) { - return false, "Everything is forbidden.", nil +func (alwaysDenyAuthorizer) Authorize(a authorizer.Attributes) (decision authorizer.Decision, reason string, err error) { + return authorizer.DecisionNoOpinion, "Everything is forbidden.", nil } func (alwaysDenyAuthorizer) RulesFor(user user.Info, namespace string) ([]authorizer.ResourceRuleInfo, []authorizer.NonResourceRuleInfo, bool, error) { @@ -73,8 +73,8 @@ func NewAlwaysDenyAuthorizer() *alwaysDenyAuthorizer { // 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 (alwaysFailAuthorizer) Authorize(a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { + return authorizer.DecisionNoOpinion, "", errors.New("Authorization failure.") } func NewAlwaysFailAuthorizer() authorizer.Authorizer { @@ -85,18 +85,18 @@ type privilegedGroupAuthorizer struct { groups []string } -func (r *privilegedGroupAuthorizer) Authorize(attr authorizer.Attributes) (bool, string, error) { +func (r *privilegedGroupAuthorizer) Authorize(attr authorizer.Attributes) (authorizer.Decision, string, error) { if attr.GetUser() == nil { - return false, "Error", errors.New("no user on request.") + return authorizer.DecisionNoOpinion, "Error", errors.New("no user on request.") } for _, attr_group := range attr.GetUser().GetGroups() { for _, priv_group := range r.groups { if priv_group == attr_group { - return true, "", nil + return authorizer.DecisionAllow, "", nil } } } - return false, "", nil + return authorizer.DecisionNoOpinion, "", nil } // NewPrivilegedGroups is for use in loopback scenarios diff --git a/pkg/endpoints/filters/authorization.go b/pkg/endpoints/filters/authorization.go index d244257a0..f60756b6f 100644 --- a/pkg/endpoints/filters/authorization.go +++ b/pkg/endpoints/filters/authorization.go @@ -47,7 +47,7 @@ func WithAuthorization(handler http.Handler, requestContextMapper request.Reques return } authorized, reason, err := a.Authorize(attributes) - if authorized { + if authorized == authorizer.DecisionAllow { handler.ServeHTTP(w, req) return } diff --git a/pkg/endpoints/filters/impersonation.go b/pkg/endpoints/filters/impersonation.go index 8f67caf86..9af292e1c 100644 --- a/pkg/endpoints/filters/impersonation.go +++ b/pkg/endpoints/filters/impersonation.go @@ -110,8 +110,8 @@ func WithImpersonation(handler http.Handler, requestContextMapper request.Reques return } - allowed, reason, err := a.Authorize(actingAsAttributes) - if err != nil || !allowed { + decision, reason, err := a.Authorize(actingAsAttributes) + if err != nil || decision != authorizer.DecisionAllow { glog.V(4).Infof("Forbidden: %#v, Reason: %s, Error: %v", req.RequestURI, reason, err) responsewriters.Forbidden(ctx, actingAsAttributes, w, req, reason, s) return diff --git a/pkg/endpoints/filters/impersonation_test.go b/pkg/endpoints/filters/impersonation_test.go index d43776507..814de2a26 100644 --- a/pkg/endpoints/filters/impersonation_test.go +++ b/pkg/endpoints/filters/impersonation_test.go @@ -35,50 +35,50 @@ import ( type impersonateAuthorizer struct{} -func (impersonateAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) { +func (impersonateAuthorizer) Authorize(a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { user := a.GetUser() switch { case user.GetName() == "system:admin": - return true, "", nil + return authorizer.DecisionAllow, "", nil case user.GetName() == "tester": - return false, "", fmt.Errorf("works on my machine") + return authorizer.DecisionNoOpinion, "", fmt.Errorf("works on my machine") case user.GetName() == "deny-me": - return false, "denied", nil + return authorizer.DecisionNoOpinion, "denied", nil } if len(user.GetGroups()) > 0 && user.GetGroups()[0] == "wheel" && a.GetVerb() == "impersonate" && a.GetResource() == "users" { - return true, "", nil + return authorizer.DecisionAllow, "", nil } if len(user.GetGroups()) > 0 && user.GetGroups()[0] == "sa-impersonater" && a.GetVerb() == "impersonate" && a.GetResource() == "serviceaccounts" { - return true, "", nil + return authorizer.DecisionAllow, "", nil } if len(user.GetGroups()) > 0 && user.GetGroups()[0] == "regular-impersonater" && a.GetVerb() == "impersonate" && a.GetResource() == "users" { - return true, "", nil + return authorizer.DecisionAllow, "", nil } if len(user.GetGroups()) > 1 && user.GetGroups()[1] == "group-impersonater" && a.GetVerb() == "impersonate" && a.GetResource() == "groups" { - return true, "", nil + return authorizer.DecisionAllow, "", nil } if len(user.GetGroups()) > 1 && user.GetGroups()[1] == "extra-setter-scopes" && a.GetVerb() == "impersonate" && a.GetResource() == "userextras" && a.GetSubresource() == "scopes" { - return true, "", nil + return authorizer.DecisionAllow, "", nil } if len(user.GetGroups()) > 1 && user.GetGroups()[1] == "extra-setter-particular-scopes" && a.GetVerb() == "impersonate" && a.GetResource() == "userextras" && a.GetSubresource() == "scopes" && a.GetName() == "scope-a" { - return true, "", nil + return authorizer.DecisionAllow, "", nil } if len(user.GetGroups()) > 1 && user.GetGroups()[1] == "extra-setter-project" && a.GetVerb() == "impersonate" && a.GetResource() == "userextras" && a.GetSubresource() == "project" { - return true, "", nil + return authorizer.DecisionAllow, "", nil } - return false, "deny by default", nil + return authorizer.DecisionNoOpinion, "deny by default", nil } func TestImpersonationFilter(t *testing.T) { diff --git a/pkg/server/genericapiserver_test.go b/pkg/server/genericapiserver_test.go index 9fc480d49..16fee408c 100644 --- a/pkg/server/genericapiserver_test.go +++ b/pkg/server/genericapiserver_test.go @@ -437,9 +437,9 @@ type mockAuthorizer struct { lastURI string } -func (authz *mockAuthorizer) Authorize(a authorizer.Attributes) (authorized bool, reason string, err error) { +func (authz *mockAuthorizer) Authorize(a authorizer.Attributes) (authorized authorizer.Decision, reason string, err error) { authz.lastURI = a.GetPath() - return true, "", nil + return authorizer.DecisionAllow, "", nil } type mockAuthenticator struct { diff --git a/plugin/pkg/authorizer/webhook/webhook.go b/plugin/pkg/authorizer/webhook/webhook.go index 890845cae..83577496e 100644 --- a/plugin/pkg/authorizer/webhook/webhook.go +++ b/plugin/pkg/authorizer/webhook/webhook.go @@ -140,7 +140,10 @@ func newWithBackoff(subjectAccessReview authorizationclient.SubjectAccessReviewI // } // } // -func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bool, reason string, err error) { +// 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. +func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (decision authorizer.Decision, reason string, err error) { r := &authorization.SubjectAccessReview{} if user := attr.GetUser(); user != nil { r.Spec = authorization.SubjectAccessReviewSpec{ @@ -169,7 +172,7 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo } key, err := json.Marshal(r.Spec) if err != nil { - return false, "", err + return authorizer.DecisionNoOpinion, "", err } if entry, ok := w.responseCache.Get(string(key)); ok { r.Status = entry.(authorization.SubjectAccessReviewStatus) @@ -185,7 +188,7 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo 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 false, "", err + return authorizer.DecisionNoOpinion, "", err } r.Status = result.Status if r.Status.Allowed { @@ -194,7 +197,12 @@ func (w *WebhookAuthorizer) Authorize(attr authorizer.Attributes) (authorized bo w.responseCache.Add(string(key), r.Status, w.unauthorizedTTL) } } - return r.Status.Allowed, r.Status.Reason, nil + if r.Status.Allowed { + return authorizer.DecisionAllow, r.Status.Reason, nil + } else { + return authorizer.DecisionNoOpinion, r.Status.Reason, nil + } + } //TODO: need to finish the method to get the rules when using webhook mode diff --git a/plugin/pkg/authorizer/webhook/webhook_test.go b/plugin/pkg/authorizer/webhook/webhook_test.go index 4f5bd2331..f637af6f2 100644 --- a/plugin/pkg/authorizer/webhook/webhook_test.go +++ b/plugin/pkg/authorizer/webhook/webhook_test.go @@ -396,13 +396,13 @@ func TestTLSConfig(t *testing.T) { // Allow all and see if we get an error. service.Allow() - authorized, _, err := wh.Authorize(attr) + decision, _, err := wh.Authorize(attr) if tt.wantAuth { - if !authorized { + if decision != authorizer.DecisionAllow { t.Errorf("expected successful authorization") } } else { - if authorized { + if decision == authorizer.DecisionAllow { t.Errorf("expected failed authorization") } } @@ -418,7 +418,7 @@ func TestTLSConfig(t *testing.T) { } service.Deny() - if authorized, _, _ := wh.Authorize(attr); authorized { + if decision, _, _ := wh.Authorize(attr); decision == authorizer.DecisionAllow { t.Errorf("%s: incorrectly authorized with DenyAll policy", tt.test) } }() @@ -522,11 +522,11 @@ func TestWebhook(t *testing.T) { } for i, tt := range tests { - authorized, _, err := wh.Authorize(tt.attr) + decision, _, err := wh.Authorize(tt.attr) if err != nil { t.Fatal(err) } - if !authorized { + if decision != authorizer.DecisionAllow { t.Errorf("case %d: authorization failed", i) continue } @@ -567,7 +567,7 @@ func testWebhookCacheCases(t *testing.T, serv *mockService, wh *WebhookAuthorize continue } - if test.expectedAuthorized != authorized { + if test.expectedAuthorized != (authorized == authorizer.DecisionAllow) { t.Errorf("%d: expected authorized=%v, got %v", i, test.expectedAuthorized, authorized) } From f4103391b277b3491f664619dc57376e927d1a32 Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Fri, 29 Sep 2017 14:22:08 -0700 Subject: [PATCH 3/4] modify the union authorizer to return on the first Approve or Deny and to continue on Unknown Kubernetes-commit: cfe580c99f60b26f39cb9a5022a8edaf64187a93 --- pkg/authorization/union/union.go | 22 ++++++-- pkg/authorization/union/union_test.go | 79 ++++++++++++++++++++------- 2 files changed, 76 insertions(+), 25 deletions(-) diff --git a/pkg/authorization/union/union.go b/pkg/authorization/union/union.go index 367da59d1..3246e4c07 100644 --- a/pkg/authorization/union/union.go +++ b/pkg/authorization/union/union.go @@ -14,6 +14,14 @@ See the License for the specific language governing permissions and limitations under the License. */ +// Package union implements an authorizer that combines multiple subauthorizer. +// The union authorizer iterates over each subauthorizer and returns the first +// decision that is either an Allow decision or a Deny decision. If a +// subauthorizer returns a NoOpinion, then the union authorizer moves onto the +// next authorizer or, if the subauthorizer was the last authorizer, returns +// NoOpinion as the aggregate decision. I.e. union authorizer creates an +// aggregate decision and supports short-circut allows and denies from +// subauthorizers. package union import ( @@ -33,14 +41,14 @@ 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) (bool, string, error) { +func (authzHandler unionAuthzHandler) Authorize(a authorizer.Attributes) (authorizer.Decision, string, error) { var ( errlist []error reasonlist []string ) for _, currAuthzHandler := range authzHandler { - authorized, reason, err := currAuthzHandler.Authorize(a) + decision, reason, err := currAuthzHandler.Authorize(a) if err != nil { errlist = append(errlist, err) @@ -48,13 +56,15 @@ func (authzHandler unionAuthzHandler) Authorize(a authorizer.Attributes) (bool, if len(reason) != 0 { reasonlist = append(reasonlist, reason) } - if !authorized { - continue + switch decision { + case authorizer.DecisionAllow, authorizer.DecisionDeny: + return decision, reason, err + case authorizer.DecisionNoOpinion: + // continue to the next authorizer } - return true, reason, nil } - return false, strings.Join(reasonlist, "\n"), utilerrors.NewAggregate(errlist) + return authorizer.DecisionNoOpinion, strings.Join(reasonlist, "\n"), utilerrors.NewAggregate(errlist) } // unionAuthzRulesHandler authorizer against a chain of authorizer.RuleResolver diff --git a/pkg/authorization/union/union_test.go b/pkg/authorization/union/union_test.go index 96d989fb6..a64138979 100644 --- a/pkg/authorization/union/union_test.go +++ b/pkg/authorization/union/union_test.go @@ -17,6 +17,7 @@ limitations under the License. package union import ( + "errors" "fmt" "reflect" "testing" @@ -26,49 +27,43 @@ import ( ) type mockAuthzHandler struct { - isAuthorized bool - err error + decision authorizer.Decision + err error } -func (mock *mockAuthzHandler) Authorize(a authorizer.Attributes) (bool, string, error) { - if mock.err != nil { - return false, "", mock.err - } - if !mock.isAuthorized { - return false, "", nil - } - return true, "", nil +func (mock *mockAuthzHandler) Authorize(a authorizer.Attributes) (authorizer.Decision, string, error) { + return mock.decision, "", mock.err } func TestAuthorizationSecondPasses(t *testing.T) { - handler1 := &mockAuthzHandler{isAuthorized: false} - handler2 := &mockAuthzHandler{isAuthorized: true} + handler1 := &mockAuthzHandler{decision: authorizer.DecisionNoOpinion} + handler2 := &mockAuthzHandler{decision: authorizer.DecisionAllow} authzHandler := New(handler1, handler2) authorized, _, _ := authzHandler.Authorize(nil) - if !authorized { + if authorized != authorizer.DecisionAllow { t.Errorf("Unexpected authorization failure") } } func TestAuthorizationFirstPasses(t *testing.T) { - handler1 := &mockAuthzHandler{isAuthorized: true} - handler2 := &mockAuthzHandler{isAuthorized: false} + handler1 := &mockAuthzHandler{decision: authorizer.DecisionAllow} + handler2 := &mockAuthzHandler{decision: authorizer.DecisionNoOpinion} authzHandler := New(handler1, handler2) authorized, _, _ := authzHandler.Authorize(nil) - if !authorized { + if authorized != authorizer.DecisionAllow { t.Errorf("Unexpected authorization failure") } } func TestAuthorizationNonePasses(t *testing.T) { - handler1 := &mockAuthzHandler{isAuthorized: false} - handler2 := &mockAuthzHandler{isAuthorized: false} + handler1 := &mockAuthzHandler{decision: authorizer.DecisionNoOpinion} + handler2 := &mockAuthzHandler{decision: authorizer.DecisionNoOpinion} authzHandler := New(handler1, handler2) authorized, _, _ := authzHandler.Authorize(nil) - if authorized { + if authorized == authorizer.DecisionAllow { t.Errorf("Expected failed authorization") } } @@ -223,3 +218,49 @@ func getNonResourceRules(infos []authorizer.NonResourceRuleInfo) []authorizer.De } return rules } + +func TestAuthorizationUnequivocalDeny(t *testing.T) { + cs := []struct { + authorizers []authorizer.Authorizer + decision authorizer.Decision + }{ + { + authorizers: []authorizer.Authorizer{}, + decision: authorizer.DecisionNoOpinion, + }, + { + authorizers: []authorizer.Authorizer{ + &mockAuthzHandler{decision: authorizer.DecisionNoOpinion}, + &mockAuthzHandler{decision: authorizer.DecisionAllow}, + &mockAuthzHandler{decision: authorizer.DecisionDeny}, + }, + decision: authorizer.DecisionAllow, + }, + { + authorizers: []authorizer.Authorizer{ + &mockAuthzHandler{decision: authorizer.DecisionNoOpinion}, + &mockAuthzHandler{decision: authorizer.DecisionDeny}, + &mockAuthzHandler{decision: authorizer.DecisionAllow}, + }, + decision: authorizer.DecisionDeny, + }, + { + authorizers: []authorizer.Authorizer{ + &mockAuthzHandler{decision: authorizer.DecisionNoOpinion}, + &mockAuthzHandler{decision: authorizer.DecisionDeny, err: errors.New("webhook failed closed")}, + &mockAuthzHandler{decision: authorizer.DecisionAllow}, + }, + decision: authorizer.DecisionDeny, + }, + } + for i, c := range cs { + t.Run(fmt.Sprintf("case %v", i), func(t *testing.T) { + authzHandler := New(c.authorizers...) + + decision, _, _ := authzHandler.Authorize(nil) + if decision != c.decision { + t.Errorf("Unexpected authorization failure: %v, expected: %v", decision, c.decision) + } + }) + } +} From c7a791258807ad6bd580e39b774179e3cf4a77e2 Mon Sep 17 00:00:00 2001 From: Mike Danese Date: Fri, 13 Oct 2017 13:51:38 -0700 Subject: [PATCH 4/4] add deny to SAR API Kubernetes-commit: 096da12fc4bf3c8b4003679d22f7228d3d178e54 --- plugin/pkg/authorizer/webhook/webhook.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/plugin/pkg/authorizer/webhook/webhook.go b/plugin/pkg/authorizer/webhook/webhook.go index 83577496e..e9efac307 100644 --- a/plugin/pkg/authorizer/webhook/webhook.go +++ b/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 }