Fix hasMountPath for segment wildcard mounts; introduce priority order (#6532)

* Add prioritization when multiple segment/glob rules can match.

* Disallow ambiguous "+*" in policy paths.

vault/acl.go

@@ -4,11 +4,12 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"sort"
 	"strings"
 
-	radix "github.com/armon/go-radix"
+	"github.com/armon/go-radix"
 	"github.com/hashicorp/errwrap"
-	multierror "github.com/hashicorp/go-multierror"
+	"github.com/hashicorp/go-multierror"
 	"github.com/hashicorp/vault/helper/identity"
 	"github.com/hashicorp/vault/helper/namespace"
 	"github.com/hashicorp/vault/helper/strutil"
@@ -367,78 +368,12 @@ func (a *ACL) AllowOperation(ctx context.Context, req *logical.Request, capCheck
 		}
 	}
 
-	// Find a prefix rule, default deny if no match
-	_, raw, ok = a.prefixRules.LongestPrefix(path)
-	if ok {
-		permissions = raw.(*ACLPermissions)
+	permissions = a.CheckAllowedFromNonExactPaths(path, false)
+	if permissions != nil {
 		capabilities = permissions.CapabilitiesBitmap
 		goto CHECK
 	}
 
-	if len(a.segmentWildcardPaths) > 0 {
-		pathParts := strings.Split(path, "/")
-		for currWCPath := range a.segmentWildcardPaths {
-			if currWCPath == "" {
-				continue
-			}
-
-			var isPrefix bool
-			var invalid bool
-			origCurrWCPath := currWCPath
-
-			if currWCPath[len(currWCPath)-1] == '*' {
-				isPrefix = true
-				currWCPath = currWCPath[0 : len(currWCPath)-1]
-			}
-			splitCurrWCPath := strings.Split(currWCPath, "/")
-			if len(pathParts) < len(splitCurrWCPath) {
-				// The path coming in is shorter; it can't match
-				continue
-			}
-			if !isPrefix && len(splitCurrWCPath) != len(pathParts) {
-				// If it's not a prefix we expect the same number of segments
-				continue
-			}
-			// We key off splitK here since it might be less than pathParts
-			for i, aclPart := range splitCurrWCPath {
-				if aclPart == "+" {
-					// Matches anything in the segment, so keep checking
-					continue
-				}
-				if i == len(splitCurrWCPath)-1 && isPrefix {
-					// In this case we may have foo* or just * depending on if
-					// originally it was foo* or foo/*.
-					if aclPart == "" {
-						// Ended in /*, so at this point we're at the final
-						// glob which will match anything, so return success
-						break
-					}
-					if !strings.HasPrefix(pathParts[i], aclPart) {
-						// E.g., the final part of the acl is foo* and the
-						// final part of the path is boofar
-						invalid = true
-						break
-					}
-					// Final prefixed matched and the rest is a wildcard,
-					// matches
-					break
-				}
-				if aclPart != pathParts[i] {
-					// Mismatch, exit out
-					invalid = true
-					break
-				}
-			}
-			// If invalid isn't set then we got through the full segmented path
-			// without finding a mismatch, so it's valid
-			if !invalid {
-				permissions = a.segmentWildcardPaths[origCurrWCPath].(*ACLPermissions)
-				capabilities = permissions.CapabilitiesBitmap
-				goto CHECK
-			}
-		}
-	}
-
 	// No exact, prefix, or segment wildcard paths found, return without
 	// setting allowed
 	return
@@ -569,6 +504,172 @@ CHECK:
 	return
 }
 
+type wcPathDescr struct {
+	firstWCOrGlob int
+	wildcards     int
+	isPrefix      bool
+	wcPath        string
+	perms         *ACLPermissions
+}
+
+// CheckAllowedFromNonExactPaths returns permissions corresponding to a
+// matching path with wildcards/globs. If bareMount is true, the path should
+// correspond to a mount prefix, and what is returned is either a non-nil set
+// of permissions from some allowed path underneath the mount (for use in mount
+// access checks), or nil indicating no non-deny permissions were found.
+func (a *ACL) CheckAllowedFromNonExactPaths(path string, bareMount bool) *ACLPermissions {
+	wcPathDescrs := make([]wcPathDescr, 0, len(a.segmentWildcardPaths)+1)
+
+	less := func(i, j int) bool {
+		// In the case of multiple matches, we use this priority order,
+		// which tries to most closely match longest-prefix:
+		//
+		// * First glob or wildcard position (prefer foo/a* over foo/+,
+		//   foo/bar/+/baz over foo/+/bar/baz)
+		// * Whether it's a prefix (prefer foo/+/bar over foo/+/ba*,
+		//   foo/+ over foo/*)
+		// * Number of wildcard segments (prefer foo/bar/+/baz over foo/+/+/baz)
+		// * Length check (prefer foo/+/bar/ba* over foo/+/bar/b*)
+		// * Lexicographical ordering (preferring less, arbitrarily)
+		//
+		// That final case (lexigraphical) should never really come up. It's more
+		// of a throwing-up-hands scenario akin to panic("should not be here")
+		// statements, but less panicky.
+
+		pdi, pdj := wcPathDescrs[i], wcPathDescrs[j]
+
+		// If the first wildcard (+) or glob (*) occurs earlier in pdi,
+		// pdi is lower priority
+		if pdi.firstWCOrGlob < pdj.firstWCOrGlob {
+			return true
+		} else if pdi.firstWCOrGlob > pdj.firstWCOrGlob {
+			return false
+		}
+
+		// If pdi ends in * and pdj doesn't, pdi is lower priority
+		if pdi.isPrefix && !pdj.isPrefix {
+			return true
+		} else if !pdi.isPrefix && pdj.isPrefix {
+			return false
+		}
+
+		// If pdi has more wc segs, pdi is lower priority
+		if pdi.wildcards > pdj.wildcards {
+			return true
+		} else if pdi.wildcards < pdj.wildcards {
+			return false
+		}
+
+		// If pdi is shorter, it is lower priority
+		if len(pdi.wcPath) < len(pdj.wcPath) {
+			return true
+		} else if len(pdi.wcPath) > len(pdj.wcPath) {
+			return false
+		}
+
+		// If pdi is smaller lexicographically, it is lower priority
+		if pdi.wcPath < pdj.wcPath {
+			return true
+		} else if pdi.wcPath > pdj.wcPath {
+			return false
+		}
+		return false
+	}
+
+	// Find a prefix rule if any.
+	{
+		prefix, raw, ok := a.prefixRules.LongestPrefix(path)
+		if ok {
+			if len(a.segmentWildcardPaths) == 0 {
+				return raw.(*ACLPermissions)
+			}
+			wcPathDescrs = append(wcPathDescrs, wcPathDescr{
+				firstWCOrGlob: len(prefix),
+				wcPath:        prefix,
+				isPrefix:      true,
+				perms:         raw.(*ACLPermissions),
+			})
+		}
+	}
+
+	if len(a.segmentWildcardPaths) == 0 {
+		return nil
+	}
+
+	pathParts := strings.Split(path, "/")
+
+SWCPATH:
+	for fullWCPath := range a.segmentWildcardPaths {
+		if fullWCPath == "" {
+			continue
+		}
+		pd := wcPathDescr{firstWCOrGlob: strings.Index(fullWCPath, "+")}
+
+		currWCPath := fullWCPath
+		if currWCPath[len(currWCPath)-1] == '*' {
+			pd.isPrefix = true
+			currWCPath = currWCPath[0 : len(currWCPath)-1]
+		}
+		pd.wcPath = currWCPath
+
+		splitCurrWCPath := strings.Split(currWCPath, "/")
+
+		if !bareMount && len(pathParts) < len(splitCurrWCPath) {
+			// check if the path coming in is shorter; if so it can't match
+			continue
+		}
+		if !bareMount && !pd.isPrefix && len(splitCurrWCPath) != len(pathParts) {
+			// If it's not a prefix we expect the same number of segments
+			continue
+		}
+
+		segments := make([]string, 0, len(splitCurrWCPath))
+		for i, aclPart := range splitCurrWCPath {
+			switch {
+			case aclPart == "+":
+				pd.wildcards++
+				segments = append(segments, pathParts[i])
+
+			case aclPart == pathParts[i]:
+				segments = append(segments, pathParts[i])
+
+			case pd.isPrefix && i == len(splitCurrWCPath)-1 && strings.HasPrefix(pathParts[i], aclPart):
+				segments = append(segments, pathParts[i:]...)
+
+			case !bareMount:
+				// Found a mismatch, give up on this segmentWildcardPath
+				continue SWCPATH
+			}
+
+			// -2 because we're always invoked with a trailing "/" in case bareMount.
+			if bareMount && i == len(pathParts)-2 {
+				joinedPath := strings.Join(segments, "/") + "/"
+				// Check the current joined path so far. If we find a prefix,
+				// check permissions. If they're defined but not deny, success.
+				if strings.HasPrefix(joinedPath, path) {
+					permissions := a.segmentWildcardPaths[fullWCPath].(*ACLPermissions)
+					if permissions.CapabilitiesBitmap&DenyCapabilityInt == 0 && permissions.CapabilitiesBitmap > 0 {
+						return permissions
+					}
+				}
+				continue SWCPATH
+			}
+		}
+		pd.perms = a.segmentWildcardPaths[fullWCPath].(*ACLPermissions)
+		wcPathDescrs = append(wcPathDescrs, pd)
+	}
+
+	if bareMount || len(wcPathDescrs) == 0 {
+		return nil
+	}
+
+	// We don't do this in the bare mount check because we don't care about
+	// priority, we only care about any capability at all.
+	sort.Slice(wcPathDescrs, less)
+
+	return wcPathDescrs[len(wcPathDescrs)-1].perms
+}
+
 func (c *Core) performPolicyChecks(ctx context.Context, acl *ACL, te *logical.TokenEntry, req *logical.Request, inEntity *identity.Entity, opts *PolicyCheckOpts) *AuthResults {
 	ret := new(AuthResults)
 

vault/acl_test.go

@@ -250,6 +250,13 @@ func testACLSingle(t *testing.T, ns *namespace.Namespace) {
 		{logical.ReadOperation, "test/segment/wildcard/at/foo/", true, false},
 		{logical.ReadOperation, "test/segment/wildcard/at/end", true, false},
 		{logical.ReadOperation, "test/segment/wildcard/at/end/", true, false},
+
+		// Path segment wildcards vs glob
+		{logical.ReadOperation, "1/2/3/4", false, false},
+		{logical.ReadOperation, "1/2/3", true, false},
+		{logical.UpdateOperation, "1/2/3", false, false},
+		{logical.UpdateOperation, "1/2/3/4", true, false},
+		{logical.CreateOperation, "1/2/3/4/5", true, false},
 	}
 
 	for _, tc := range tcases {
@@ -593,6 +600,197 @@ func testACLValuePermissions(t *testing.T, ns *namespace.Namespace) {
 	}
 }
 
+func TestACL_SegmentWildcardPriority(t *testing.T) {
+	ns := namespace.RootNamespace
+	ctx := namespace.ContextWithNamespace(context.Background(), ns)
+	type poltest struct {
+		policy string
+		path   string
+	}
+
+	// These test cases should each have a read rule and an update rule, where
+	// the update rule wins out due to being more specific.
+	poltests := []poltest{
+
+		{
+			// Verify edge conditions.  Here '*' is more specific both because
+			// of first wildcard position (0 vs -1/infinity) and #wildcards.
+			`
+path "+/*" { capabilities = ["read"] }
+path "*" { capabilities = ["update"] }
+`,
+			"foo/bar/bar/baz",
+		},
+		{
+			// Verify edge conditions.  Here '+/*' is less specific because of
+			// first wildcard position.
+			`
+path "+/*" { capabilities = ["read"] }
+path "foo/+/*" { capabilities = ["update"] }
+`,
+			"foo/bar/bar/baz",
+		},
+		{
+			// Verify that more wildcard segments is lower priority.
+			`
+path "foo/+/+/*" { capabilities = ["read"] }
+path "foo/+/bar/baz" { capabilities = ["update"] }
+`,
+			"foo/bar/bar/baz",
+		},
+		{
+			// Verify that more wildcard segments is lower priority.
+			`
+path "foo/+/+/baz" { capabilities = ["read"] }
+path "foo/+/bar/baz" { capabilities = ["update"] }
+`,
+			"foo/bar/bar/baz",
+		},
+		{
+			// Verify that first wildcard position is lower priority.
+			// '(' is used here because it is lexicographically smaller than "+"
+			`
+path "foo/+/(ar/baz" { capabilities = ["read"] }
+path "foo/(ar/+/baz" { capabilities = ["update"] }
+`,
+			"foo/(ar/(ar/baz",
+		},
+
+		{
+			// Verify that a glob has lower priority, even if the prefix is the
+			// same otherwise.
+			`
+path "foo/bar/+/baz*" { capabilities = ["read"] }
+path "foo/bar/+/baz" { capabilities = ["update"] }
+`,
+			"foo/bar/bar/baz",
+		},
+		{
+			// Verify that a shorter prefix has lower priority.
+			`
+path "foo/bar/+/b*" { capabilities = ["read"] }
+path "foo/bar/+/ba*" { capabilities = ["update"] }
+`,
+			"foo/bar/bar/baz",
+		},
+	}
+
+	for i, pt := range poltests {
+		policy, err := ParseACLPolicy(ns, pt.policy)
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		acl, err := NewACL(ctx, []*Policy{policy})
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		request := new(logical.Request)
+		request.Path = pt.path
+
+		request.Operation = logical.UpdateOperation
+		authResults := acl.AllowOperation(ctx, request, false)
+		if !authResults.Allowed {
+			t.Fatalf("bad: case %d %#v: %v", i, pt, authResults.Allowed)
+		}
+
+		request.Operation = logical.ReadOperation
+		authResults = acl.AllowOperation(ctx, request, false)
+		if authResults.Allowed {
+			t.Fatalf("bad: case %d %#v: %v", i, pt, authResults.Allowed)
+		}
+	}
+}
+
+func TestACL_SegmentWildcardPriority_BareMount(t *testing.T) {
+	ns := namespace.RootNamespace
+	ctx := namespace.ContextWithNamespace(context.Background(), ns)
+	type poltest struct {
+		policy    string
+		mountpath string
+		hasperms  bool
+	}
+	// These test cases should have one or more rules and a mount prefix.
+	// hasperms should be true if there are non-deny perms that apply
+	// to the mount prefix or something below it.
+	poltests := []poltest{
+		{
+			`path "+" { capabilities = ["read"] }`,
+			"foo/",
+			true,
+		},
+		{
+			`path "+/*" { capabilities = ["read"] }`,
+			"foo/",
+			true,
+		},
+		{
+			`path "foo/+/+/*" { capabilities = ["read"] }`,
+			"foo/",
+			true,
+		},
+		{
+			`path "foo/+/+/*" { capabilities = ["read"] }`,
+			"foo/bar/",
+			true,
+		},
+		{
+			`path "foo/+/+/*" { capabilities = ["read"] }`,
+			"foo/bar/bar/",
+			true,
+		},
+		{
+			`path "foo/+/+/*" { capabilities = ["read"] }`,
+			"foo/bar/bar/baz/",
+			true,
+		},
+		{
+			`path "foo/+/+/baz" { capabilities = ["read"] }`,
+			"foo/bar/bar/baz/",
+			true,
+		},
+		{
+			`path "foo/+/bar/baz" { capabilities = ["read"] }`,
+			"foo/bar/bar/baz/",
+			true,
+		},
+		{
+			`path "foo/bar/+/baz*" { capabilities = ["read"] }`,
+			"foo/bar/bar/baz/",
+			true,
+		},
+		{
+			`path "foo/bar/+/b*" { capabilities = ["read"] }`,
+			"foo/bar/bar/baz/",
+			true,
+		},
+		{
+			`path "foo/+" { capabilities = ["read"] }`,
+			"foo/",
+			true,
+		},
+	}
+
+	for i, pt := range poltests {
+		policy, err := ParseACLPolicy(ns, pt.policy)
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		acl, err := NewACL(ctx, []*Policy{policy})
+		if err != nil {
+			t.Fatalf("err: %v", err)
+		}
+
+		hasperms := nil != acl.CheckAllowedFromNonExactPaths(pt.mountpath, true)
+		if hasperms != pt.hasperms {
+			t.Fatalf("bad: case %d: %#v", i, pt)
+		}
+
+	}
+}
+
 // NOTE: this test doesn't catch any races ATM
 func TestACL_CreationRace(t *testing.T) {
 	policy, err := ParseACLPolicy(namespace.RootNamespace, valuePermissionsPolicy)
@@ -674,6 +872,15 @@ path "test/+/wildcard/+/*" {
 path "test/+/wildcardglob/+/end*" {
 	capabilities = ["read"]
 }
+path "1/2/*" {
+	capabilities = ["create"]
+}
+path "1/2/+" {
+	capabilities = ["read"]
+}
+path "1/2/+/+" {
+	capabilities = ["update"]
+}
 `
 
 var aclPolicy2 = `

vault/logical_system.go

@@ -2819,7 +2819,7 @@ func hasMountAccess(ctx context.Context, acl *ACL, path string) bool {
 		return false
 	}
 
-	// If an earlier policy is giving us access to the mount path then we can do
+	// If a policy is giving us direct access to the mount path then we can do
 	// a fast return.
 	capabilities := acl.Capabilities(ctx, ns.TrimmedPath(path))
 	if !strutil.StrListContains(capabilities, DenyCapability) {
@@ -2846,6 +2846,7 @@ func hasMountAccess(ctx context.Context, acl *ACL, path string) bool {
 			perms.CapabilitiesBitmap&UpdateCapabilityInt > 0:
 
 			aclCapabilitiesGiven = true
+
 			return true
 		}
 
@@ -2857,6 +2858,12 @@ func hasMountAccess(ctx context.Context, acl *ACL, path string) bool {
 		acl.prefixRules.WalkPrefix(path, walkFn)
 	}
 
+	if !aclCapabilitiesGiven {
+		if perms := acl.CheckAllowedFromNonExactPaths(path, true); perms != nil {
+			return true
+		}
+	}
+
 	return aclCapabilitiesGiven
 }
 

vault/logical_system_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/hashicorp/vault/audit"
 	"github.com/hashicorp/vault/helper/builtinplugins"
 	"github.com/hashicorp/vault/helper/consts"
+	"github.com/hashicorp/vault/helper/identity"
 	"github.com/hashicorp/vault/helper/jsonutil"
 	"github.com/hashicorp/vault/helper/namespace"
 	"github.com/hashicorp/vault/helper/salt"
@@ -2555,3 +2556,90 @@ func TestSystemBackend_OpenAPI(t *testing.T) {
 		t.Fatalf("expected to find path '/rotate'")
 	}
 }
+
+func TestSystemBackend_PathWildcardPreflight(t *testing.T) {
+	core, b, _ := testCoreSystemBackend(t)
+
+	ctx := namespace.RootContext(nil)
+
+	// Add another mount
+	me := &MountEntry{
+		Table:   mountTableType,
+		Path:    sanitizeMountPath("kv-v1"),
+		Type:    "kv",
+		Options: map[string]string{"version": "1"},
+	}
+	if err := core.mount(ctx, me); err != nil {
+		t.Fatal(err)
+	}
+
+	// Create the policy, designed to fail
+	rules := `path "foo" { capabilities = ["read"] }`
+	req := logical.TestRequest(t, logical.UpdateOperation, "policy/foo")
+	req.Data["rules"] = rules
+	resp, err := b.HandleRequest(ctx, req)
+	if err != nil {
+		t.Fatalf("err: %v %#v", err, resp)
+	}
+	if resp != nil && (resp.IsError() || len(resp.Data) > 0) {
+		t.Fatalf("bad: %#v", resp)
+	}
+
+	if err := core.identityStore.upsertEntity(ctx, &identity.Entity{
+		ID:            "abcd",
+		Name:          "abcd",
+		BucketKeyHash: "abcd",
+	}, nil, false); err != nil {
+		t.Fatal(err)
+	}
+
+	te := &logical.TokenEntry{
+		TTL:         300 * time.Second,
+		EntityID:    "abcd",
+		Policies:    []string{"default", "foo"},
+		NamespaceID: namespace.RootNamespaceID,
+	}
+	if err := core.tokenStore.create(ctx, te); err != nil {
+		t.Fatal(err)
+	}
+	t.Logf("token id: %s", te.ID)
+
+	if err := core.expiration.RegisterAuth(ctx, te, &logical.Auth{
+		LeaseOptions: logical.LeaseOptions{
+			TTL: te.TTL,
+		},
+		ClientToken: te.ID,
+		Accessor:    te.Accessor,
+		Orphan:      true,
+	}); err != nil {
+		t.Fatal(err)
+	}
+
+	// Check the mount access func
+	req = logical.TestRequest(t, logical.ReadOperation, "internal/ui/mounts/kv-v1/baz")
+	req.ClientToken = te.ID
+	resp, err = b.HandleRequest(ctx, req)
+	if err == nil || !strings.Contains(err.Error(), "permission denied") {
+		t.Fatalf("expected 403, got err: %v", err)
+	}
+
+	// Modify policy to pass
+	rules = `path "kv-v1/+" { capabilities = ["read"] }`
+	req = logical.TestRequest(t, logical.UpdateOperation, "policy/foo")
+	req.Data["rules"] = rules
+	resp, err = b.HandleRequest(ctx, req)
+	if err != nil {
+		t.Fatalf("err: %v %#v", err, resp)
+	}
+	if resp != nil && (resp.IsError() || len(resp.Data) > 0) {
+		t.Fatalf("bad: %#v", resp)
+	}
+
+	// Check the mount access func again
+	req = logical.TestRequest(t, logical.ReadOperation, "internal/ui/mounts/kv-v1/baz")
+	req.ClientToken = te.ID
+	resp, err = b.HandleRequest(ctx, req)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+}

vault/policy.go

@@ -339,7 +339,11 @@ func parsePaths(result *Policy, list *ast.ObjectList, performTemplating bool, en
 		// Ensure we are using the full request path internally
 		pc.Path = result.namespace.Path + pc.Path
 
-		if strings.Count(pc.Path, "/+") > 0 || strings.HasPrefix(pc.Path, "+/") {
+		if strings.Contains(pc.Path, "+*") {
+			return fmt.Errorf("path %q: invalid use of wildcards ('+*' is forbidden)", pc.Path)
+		}
+
+		if pc.Path == "+" || strings.Count(pc.Path, "/+") > 0 || strings.HasPrefix(pc.Path, "+/") {
 			pc.HasSegmentWildcards = true
 		}
 

vault/policy_test.go

@@ -414,3 +414,18 @@ path "/" {
 		t.Errorf("bad error: %s", err)
 	}
 }
+
+func TestPolicy_ParseBadSegmentWildcard(t *testing.T) {
+	_, err := ParseACLPolicy(namespace.RootNamespace, strings.TrimSpace(`
+path "foo/+*" {
+	capabilities = ["read"]
+}
+`))
+	if err == nil {
+		t.Fatalf("expected error")
+	}
+
+	if !strings.Contains(err.Error(), `path "foo/+*": invalid use of wildcards ('+*' is forbidden)`) {
+		t.Errorf("bad error: %s", err)
+	}
+}