p/a/path: fail on broken pattern early, add tests

cmd/kube-rbac-proxy/app/kube-rbac-proxy.go

@@ -298,14 +298,20 @@ func setupAuthorizer(krbInfo *server.KubeRBACProxyInfo, delegatedAuthz *serverco
 	// pathAuthorizer is running before any other authorizer.
 	// It works outside of the default authorizers.
 	var pathAuthorizer authorizer.Authorizer
-
+	var err error
 	// AllowPaths denies access to paths that are not listed.
 	// IgnorePaths doesn't auth(n/z) paths that are listed.
 	switch {
 	case len(krbInfo.AllowPaths) > 0:
-		pathAuthorizer = path.NewAllowPathAuthorizer(krbInfo.AllowPaths)
+		pathAuthorizer, err = path.NewDenyUnlessAllowedPathAuthorizer(krbInfo.AllowPaths)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create allow path authorizer: %w", err)
+		}
 	case len(krbInfo.IgnorePaths) > 0:
-		pathAuthorizer = path.NewAlwaysAllowPathAuthorizer(krbInfo.IgnorePaths)
+		pathAuthorizer, err = path.NewPathAuthorizer(krbInfo.IgnorePaths)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create ignore path authorizer: %w", err)
+		}
 	}
 
 	// Delegated authorizers are running after the pathAuthorizer

pkg/authorization/path/path.go

@@ -32,10 +32,15 @@ type pathAuthorizer struct {
 	pathPatterns []string
 }
 
-func newPathAuthorizer(onMatch, onNoMatch authorizer.Decision, inputPaths []string) *pathAuthorizer {
+func newPathAuthorizer(onMatch, onNoMatch authorizer.Decision, inputPaths []string) (*pathAuthorizer, error) {
 	var patterns []string
 	paths := sets.NewString() // faster than trying to match every pattern every time
 	for _, p := range inputPaths {
+		_, err := path.Match(p, "")
+		if err != nil {
+			return nil, err
+		}
+
 		p = strings.TrimPrefix(p, "/")
 		if len(p) == 0 {
 			// matches "/"
@@ -54,7 +59,7 @@ func newPathAuthorizer(onMatch, onNoMatch authorizer.Decision, inputPaths []stri
 		noMatchDecision: onNoMatch,
 		paths:           paths,
 		pathPatterns:    patterns,
-	}
+	}, nil
 }
 
 func (a *pathAuthorizer) Authorize(ctx context.Context, attr authorizer.Attributes) (authorizer.Decision, string, error) {
@@ -74,15 +79,15 @@ func (a *pathAuthorizer) Authorize(ctx context.Context, attr authorizer.Attribut
 	return a.noMatchDecision, "", nil
 }
 
-func NewAllowPathAuthorizer(allowPaths []string) authorizer.Authorizer {
+func NewDenyUnlessAllowedPathAuthorizer(allowPaths []string) (authorizer.Authorizer, error) {
 	if len(allowPaths) == 0 {
 		return authorizer.AuthorizerFunc(func(context.Context, authorizer.Attributes) (authorizer.Decision, string, error) {
 			return authorizer.DecisionNoOpinion, "", nil
-		})
+		}), nil
 	}
 	return newPathAuthorizer(authorizer.DecisionNoOpinion, authorizer.DecisionDeny, allowPaths)
 }
 
-func NewAlwaysAllowPathAuthorizer(alwaysAllowPaths []string) authorizer.Authorizer {
-	return newPathAuthorizer(authorizer.DecisionAllow, authorizer.DecisionNoOpinion, alwaysAllowPaths)
+func NewPathAuthorizer(ignorePaths []string) (authorizer.Authorizer, error) {
+	return newPathAuthorizer(authorizer.DecisionAllow, authorizer.DecisionNoOpinion, ignorePaths)
 }

pkg/authorization/path/path_test.go

@@ -23,6 +23,65 @@ import (
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 )
 
+func TestIgnorePath(t *testing.T) {
+	validPath := "/allowed/path/withsuffix"
+
+	for _, tt := range []struct {
+		name        string
+		paths       []string
+		decision    authorizer.Decision
+		expectError bool
+	}{
+		{
+			name:     "should authorize attributes, if path is identical",
+			paths:    []string{validPath},
+			decision: authorizer.DecisionAllow,
+		},
+		{
+			name:     "should authorize attributes, if path matches with wildcard",
+			paths:    []string{"/allowed/*/withsuffix"},
+			decision: authorizer.DecisionAllow,
+		},
+		{
+			name:     "should not authorize attributes, if path doesn't match",
+			paths:    []string{"/denied"},
+			decision: authorizer.DecisionNoOpinion,
+		},
+		{
+			name:     "should not authorize attributes, if no path specified",
+			paths:    []string{},
+			decision: authorizer.DecisionNoOpinion,
+		},
+		{
+			name:        "should not authorize attributes, if pattern is non-sense",
+			paths:       []string{"[]a]/*"},
+			expectError: true,
+		},
+	} {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			authz, err := path.NewPathAuthorizer(tt.paths)
+			if err != nil && !tt.expectError {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if err == nil && tt.expectError {
+				t.Fatalf("expected error, got none")
+			}
+			if tt.expectError {
+				return
+			}
+
+			decision, _, _ := authz.Authorize(context.Background(), authorizer.AttributesRecord{
+				Path: validPath,
+			})
+
+			if decision != tt.decision {
+				t.Fatalf("expected decision %v, got %v", tt.decision, decision)
+			}
+		})
+	}
+}
+
 func TestAllowPath(t *testing.T) {
 	validPath := "/allowed/path/withsuffix"
 
@@ -33,43 +92,48 @@ func TestAllowPath(t *testing.T) {
 		expectError bool
 	}{
 		{
-			name:     "should let request through if path allowed",
+			name:     "should let attributes through to next authorizer, if path allowed",
 			paths:    []string{validPath},
 			decision: authorizer.DecisionNoOpinion,
 		},
 		{
-			name:     "should let request through if path allowed by wildcard",
+			name:     "should let attributes through to next authorizer, if path allowed by wildcard",
 			paths:    []string{"/allowed/*/withsuffix"},
 			decision: authorizer.DecisionNoOpinion,
 		},
 		{
-			name:     "should not let request through if path not allowed",
+			name:     "should not let attributes through to next authorizer, if path not allowed",
 			paths:    []string{"/denied"},
 			decision: authorizer.DecisionDeny,
 		},
 		{
-			name:     "should let request through if no path specified",
+			name:     "should let attributes through to next authorizer, if no path specified",
 			paths:    []string{},
 			decision: authorizer.DecisionNoOpinion,
 		},
 		{
-			name:        "should not let request through if path is non-sense",
+			name:        "should not let attributes through to next authorizer, if pattern is non-sense",
 			paths:       []string{"[]a]/*"},
-			decision:    authorizer.DecisionNoOpinion,
 			expectError: true,
 		},
 	} {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			authz := path.NewAllowPathAuthorizer(tt.paths)
-			decision, _, err := authz.Authorize(context.Background(), authorizer.AttributesRecord{
+			authz, err := path.NewDenyUnlessAllowedPathAuthorizer(tt.paths)
+			if err != nil && !tt.expectError {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if err == nil && tt.expectError {
+				t.Fatalf("expected error, got none")
+			}
+			if tt.expectError {
+				return
+			}
+
+			decision, _, _ := authz.Authorize(context.Background(), authorizer.AttributesRecord{
 				Path: validPath,
 			})
 
-			if (err != nil) != tt.expectError {
-				t.Fatalf("expected error: %v; got: %v", tt.expectError, err)
-			}
-
 			if decision != tt.decision {
 				t.Errorf("want: %d\nhave: %d\n", tt.decision, decision)
 			}