Merge pull request #250 from ibihim/path-authorizer-fix-test

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

README.md

@@ -18,6 +18,10 @@ To get kube-rbac-proxy accepted as an official Kubernetes SIG Auth repository, w
 - `--insecure-listen-addres` - the proxy is handling authentication information and will no longer run in an unsafe mode
 - `--auth-header-fields-enabled` - its value is derived from at least one of `--auth-header-groups-field-name` or `--auth-header-user-field-name` being set
 - `--tls-reload-interval` - upstream Kubernetes TLS material is now being reloaded by Kubernetes upstream code which chooses its own reasonable defaults
+- **infix regex** for `--allow-paths` and `--ignore-paths` - the proxy now uses postfix matching with `*` as its sole wildcard operator, instead of infix matching with different wildcard operators:
+  - `/api/v1/*` matches now `/api/v1`, `/api/v1/foo` and `/api/v1/foo/bar`, **previously** it didn't match `/api/v1/foo/bar`.
+  - `/api/v1/*/values` creates an error on server startup, **previously** it matched `/api/v1/foo/values` and `/api/v1/bar/values`.
+  - details: [Allow Path and Ignore Path changes](docs/path.md)
 
 #### Replaced:
 - `--kubeconfig` - use `--authentication-kubeconfig` and `--authorization-kubeconfig` instead

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
-
-	// AllowPaths denies access to paths that are not listed.
-	// IgnorePaths doesn't auth(n/z) paths that are listed.
+	var err error
+	// AllowPaths are the only paths that are not denied.
+	// IgnorePaths bypass all authorization checks.
 	switch {
 	case len(krbInfo.AllowPaths) > 0:
-		pathAuthorizer = path.NewAllowPathAuthorizer(krbInfo.AllowPaths)
+		pathAuthorizer, err = path.NewAllowedPathsAuthorizer(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.NewPassthroughAuthorizer(krbInfo.IgnorePaths)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create ignore path authorizer: %w", err)
+		}
 	}
 
 	// Delegated authorizers are running after the pathAuthorizer

docs/migration/path.md

@@ -0,0 +1,44 @@
+# Allow Path and Ignore Path
+
+## Description
+
+If `--allow-paths` is set, only the requests to these paths will be considered for proxying, the rest will be rejected.
+If `--ignore-paths` is set, requests to these paths will be passed upstream without any authorization.
+
+Only one of `--allow-paths`/`--ignore-paths` is allowed to be set at a time.
+
+## Change
+
+Previously, it was possible to configure an **infix wildcard** but that will now fail.
+Now **infix regex** will cause an error on server startup.
+kube-rbac-proxy won't start with an **infix regex**.
+E.g. `--allow-path="/api/v1/*/values"` will cause kube-rbac-proxy fail to start.
+E.g. `--ignore-path="/api/v1/*/values"` will cause kube-rbac-proxy fail to start.
+
+Previously `*` would count as a single-path-segment wildcard ([path.Match](https://pkg.go.dev/path#Match)).
+Now the wildcard is matching any string, even if it contains `/`.
+E.g. `--allow-path="/api/v1/*"` would have rejected `/api/v1/label/values`, with the change it is being evaluated.
+E.g. `--ignore-path="/api/v1/*"` would have evaluated `/api/v1/label/values`, with the change it is being passed through. 
+
+### Reason
+
+We are in an effort to mirate kube-rbac-proxy to the k8s sig-auth organization.
+In order to meet the requirements of the k8s sig-auth organization we need to adjust the code base.
+The kubernetes code base doesn't allow **infix regex**.
+It is considered a security risk in case of misconfiguration.
+
+## Call to action
+
+You need to act if you:
+
+### Use infix wildcard operator
+
+If `/api/v1/*/values` was used and `/api/v1/labels` shouldn't match `/api/v1/*`, it needs to be replaced with every individual path segment.
+
+### Expect wildcard operator to match exactly one path sagment
+
+If `/api/v1/*` shouldn't match `/api/v1/label/values` but `/api/v1/label` it is necessary to replace the wildcard operator with every individual path sagment.
+
+### Use other wildcard operators than `*`, like `?`
+
+If `/api/v1/label*` shouldn't match `/api/v1/labels/values` but `/api/v1/labels` it is necessary to replace the wildcard operator with every individual path sagment.

pkg/authorization/path/path.go

@@ -18,71 +18,41 @@ package path
 
 import (
 	"context"
-	"path"
-	"strings"
+	"fmt"
 
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
+	pathauthorizer "k8s.io/apiserver/pkg/authorization/path"
 )
 
-type pathAuthorizer struct {
-	matchDecision, noMatchDecision authorizer.Decision
-
-	paths        sets.String
-	pathPatterns []string
-}
-
-func newPathAuthorizer(onMatch, onNoMatch authorizer.Decision, inputPaths []string) *pathAuthorizer {
-	var patterns []string
-	paths := sets.NewString() // faster than trying to match every pattern every time
-	for _, p := range inputPaths {
-		p = strings.TrimPrefix(p, "/")
-		if len(p) == 0 {
-			// matches "/"
-			paths.Insert(p)
-			continue
-		}
-		if strings.ContainsRune(p, '*') {
-			patterns = append(patterns, p)
-		} else {
-			paths.Insert(p)
-		}
+// NewAllowedPathsAuthorizer returns an authorizer that allows requests to
+// `allowPaths` through with `NoOpinion` and denies all others. This ensures that
+// subsequent authorizers in a union are still called on the requests that pass through
+// this authorizer.
+// The provided paths can include simple glob patterns.
+func NewAllowedPathsAuthorizer(allowPaths []string) (authorizer.Authorizer, error) {
+	delegatedPathAuthorizer, err := pathauthorizer.NewAuthorizer(allowPaths)
+	if err != nil {
+		return nil, fmt.Errorf("error creating path authorizer: %v", err)
 	}
 
-	return &pathAuthorizer{
-		matchDecision:   onMatch,
-		noMatchDecision: onNoMatch,
-		paths:           paths,
-		pathPatterns:    patterns,
-	}
-}
+	return authorizer.AuthorizerFunc(func(ctx context.Context, attr authorizer.Attributes) (authorizer.Decision, string, error) {
+		decision, reason, err := delegatedPathAuthorizer.Authorize(ctx, attr)
 
-func (a *pathAuthorizer) Authorize(ctx context.Context, attr authorizer.Attributes) (authorizer.Decision, string, error) {
-	pth := strings.TrimPrefix(attr.GetPath(), "/")
-	if a.paths.Has(pth) {
-		return a.matchDecision, "", nil
-	}
-
-	for _, pattern := range a.pathPatterns {
-		if found, err := path.Match(pattern, pth); err != nil {
-			return authorizer.DecisionNoOpinion, "Error", err
-		} else if found {
-			return a.matchDecision, "", nil
+		// There is a match on the path, so we have no opinion and let subsequent
+		// authorizers in a union decide.
+		if err == nil && decision == authorizer.DecisionAllow {
+			return authorizer.DecisionNoOpinion, reason, nil
 		}
-	}
 
-	return a.noMatchDecision, "", nil
-}
-
-func NewAllowPathAuthorizer(allowPaths []string) authorizer.Authorizer {
-	if len(allowPaths) == 0 {
-		return authorizer.AuthorizerFunc(func(context.Context, authorizer.Attributes) (authorizer.Decision, string, error) {
-			return authorizer.DecisionNoOpinion, "", nil
-		})
-	}
-	return newPathAuthorizer(authorizer.DecisionNoOpinion, authorizer.DecisionDeny, allowPaths)
+		return authorizer.DecisionDeny, fmt.Sprintf("NOT(%s)", reason), err
+	}), nil
 }
 
-func NewAlwaysAllowPathAuthorizer(alwaysAllowPaths []string) authorizer.Authorizer {
-	return newPathAuthorizer(authorizer.DecisionAllow, authorizer.DecisionNoOpinion, alwaysAllowPaths)
+// NewPassthroughAuthorizer returns an authorizer that allows on matches for
+// the given paths and has no opinion on all others. This allows skipping
+// subsequent authorizers in a union, effectively passing through the given
+// paths.
+// The given paths can include simple glob patterns.
+func NewPassthroughAuthorizer(ignorePaths []string) (authorizer.Authorizer, error) {
+	return pathauthorizer.NewAuthorizer(ignorePaths)
 }

pkg/authorization/path/path_test.go

@@ -23,8 +23,8 @@ import (
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 )
 
-func TestAllowPath(t *testing.T) {
-	validPath := "/allowed/path/withsuffix"
+func TestIgnorePath(t *testing.T) {
+	requestPath := "/allowed/path/with/suffix"
 
 	for _, tt := range []struct {
 		name        string
@@ -33,42 +33,116 @@ func TestAllowPath(t *testing.T) {
 		expectError bool
 	}{
 		{
-			name:     "should let request through if path allowed",
-			paths:    []string{validPath},
-			decision: authorizer.DecisionNoOpinion,
+			name:     "should authorize attributes, if path is identical",
+			paths:    []string{requestPath},
+			decision: authorizer.DecisionAllow,
+		},
+		{
+			name:     "should authorize attributes, if path matches with postfix wildcard",
+			paths:    []string{"/allowed/*"},
+			decision: authorizer.DecisionAllow,
 		},
 		{
-			name:     "should let request through if path allowed by wildcard",
-			paths:    []string{"/allowed/*/withsuffix"},
+			name:     "should authorize attributes, if path matches with generous postfix wildcard",
+			paths:    []string{"/a*"},
+			decision: authorizer.DecisionAllow,
+		},
+		{
+			name:     "shouldn't authorize attributes, if path matches without trailing slash",
+			paths:    []string{"/allowed/path/with/suffix/*"},
 			decision: authorizer.DecisionNoOpinion,
 		},
 		{
-			name:     "should not let request through if path not allowed",
+			name:        "should fail on initialization with infix wildcard",
+			paths:       []string{"/allowed/*/withsuffix"},
+			expectError: true,
+		},
+		{
+			name:     "should not authorize attributes, if path doesn't match",
 			paths:    []string{"/denied"},
-			decision: authorizer.DecisionDeny,
+			decision: authorizer.DecisionNoOpinion,
 		},
 		{
-			name:     "should let request through if no path specified",
+			name:     "should not authorize attributes, if no path specified",
 			paths:    []string{},
 			decision: authorizer.DecisionNoOpinion,
 		},
 		{
-			name:        "should not let request through if path is non-sense",
-			paths:       []string{"[]a]/*"},
-			decision:    authorizer.DecisionNoOpinion,
-			expectError: true,
+			name:     "should not authorize attributes, if no path specified",
+			paths:    []string{"/all?wed/path/with/suffix"},
+			decision: authorizer.DecisionNoOpinion,
 		},
 	} {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			authz := path.NewAllowPathAuthorizer(tt.paths)
-			decision, _, err := authz.Authorize(context.Background(), authorizer.AttributesRecord{
-				Path: validPath,
+			authz, err := path.NewPassthroughAuthorizer(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: requestPath,
 			})
 
-			if (err != nil) != tt.expectError {
-				t.Fatalf("expected error: %v; got: %v", tt.expectError, err)
+			if decision != tt.decision {
+				t.Fatalf("expected decision %v, got %v", tt.decision, decision)
 			}
+		})
+	}
+}
+
+func TestAllowPath(t *testing.T) {
+	requestPath := "/allowed/path/with/suffix"
+
+	for _, tt := range []struct {
+		name        string
+		paths       []string
+		decision    authorizer.Decision
+		expectError bool
+	}{
+		{
+			name:     "should let attributes through to next authorizer, if path allowed",
+			paths:    []string{requestPath},
+			decision: authorizer.DecisionNoOpinion,
+		},
+		{
+			name:     "should let attributes through to next authorizer with postfix wildcard",
+			paths:    []string{"/allowed/path/*"},
+			decision: authorizer.DecisionNoOpinion,
+		},
+		{
+			name:        "should not let attributes through to next authorizer with infix wildcard",
+			paths:       []string{"/allowed/*/with/suffix"},
+			expectError: true,
+		},
+		{
+			name:     "should not let attributes through to next authorizer, if path not allowed",
+			paths:    []string{"/denied"},
+			decision: authorizer.DecisionDeny,
+		},
+	} {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			authz, err := path.NewAllowedPathsAuthorizer(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: requestPath,
+			})
 
 			if decision != tt.decision {
 				t.Errorf("want: %d\nhave: %d\n", tt.decision, decision)

test/e2e/allowpaths/deployment.yaml

@@ -21,7 +21,7 @@ spec:
             - "--secure-port=8443"
             - "--proxy-endpoints-port=8643"
             - "--upstream=http://127.0.0.1:8081/"
-            - "--allow-paths=/metrics,/api/v1/label/*/values"
+            - "--allow-paths=/metrics,/api/v1/label/*"
             - "--authentication-skip-lookup"
             - "--logtostderr=true"
             - "--v=10"

test/e2e/basics.go

@@ -358,7 +358,7 @@ func testAllowPathsRegexp(client kubernetes.Interface) kubetest.TestSuite {
 				),
 				kubetest.ClientSucceeds(
 					client,
-					fmt.Sprintf(command, "/api/v1/label/name", 403, 403),
+					fmt.Sprintf(command, "/api/v1", 403, 403),
 					nil,
 				),
 			),
@@ -450,11 +450,21 @@ func testIgnorePaths(client kubernetes.Interface) kubetest.TestSuite {
 					fmt.Sprintf(commandWithoutAuth, "/metrics", 200, 200),
 					nil,
 				),
+				kubetest.ClientSucceeds(
+					client,
+					fmt.Sprintf(commandWithoutAuth, "/api/v1/", 200, 200),
+					nil,
+				),
 				kubetest.ClientSucceeds(
 					client,
 					fmt.Sprintf(commandWithoutAuth, "/api/v1/labels", 200, 200),
 					nil,
 				),
+				kubetest.ClientSucceeds(
+					client,
+					fmt.Sprintf(commandWithoutAuth, "/api/v1/label/values", 200, 200),
+					nil,
+				),
 			),
 		}.Run(t)
 
@@ -497,7 +507,12 @@ func testIgnorePaths(client kubernetes.Interface) kubetest.TestSuite {
 				),
 				kubetest.ClientSucceeds(
 					client,
-					fmt.Sprintf(commandWithoutAuth, "/api/v1/label/job/values", 403, 403),
+					fmt.Sprintf(commandWithoutAuth, "/api/v1", 403, 403),
+					nil,
+				),
+				kubetest.ClientSucceeds(
+					client,
+					fmt.Sprintf(commandWithoutAuth, "/api/v12", 403, 403),
 					nil,
 				),
 			),