From 2abf916ddb1365754bd4c515fe7a9f48faa0981b Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Wed, 13 Oct 2021 11:51:20 -0500 Subject: [PATCH] Add support to parameterize unauthenticated paths (#12668) * store unauthenticated path wildcards in map * working unauthenticated paths with basic unit tests * refactor wildcard logic * add parseUnauthenticatedPaths unit tests * use parseUnauthenticatedPaths when reloading backend * add more wildcard test cases * update special paths doc; add changelog * remove buggy prefix check; add test cases * prevent false positives for prefix matches If we ever encounter a mismatched segment, break and set a flag to prevent false positives for prefix matches. If it is a match we need to do a prefix check. But we should not return unless HasPrefix also evaluates to true. Otherwise we should let the for loop continue to check other possibilities and only return false once all wildcard paths have been evaluated. * refactor switch and add more test cases * remove comment leftover from debug session * add more wildcard path validation and test cases * update changelong; feature -> improvement * simplify wildcard segment matching logic * refactor wildcard matching into func * fix glob matching, add more wildcard validation, refactor * refactor common wildcard errors to func * move doc comment to logical.Paths * optimize wildcard paths storage with pre-split slices * fix comment typo * fix test case after changing wildcard paths storage type * move prefix check to parseUnauthenticatedPaths * tweak regex, remove unneeded array copy, refactor * add test case around wildcard and glob matching --- changelog/12668.txt | 3 + sdk/framework/backend.go | 6 +- sdk/logical/logical.go | 4 ++ vault/identity_store.go | 2 + vault/plugin_reload.go | 6 +- vault/router.go | 139 +++++++++++++++++++++++++++++++++--- vault/router_test.go | 150 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 295 insertions(+), 15 deletions(-) create mode 100644 changelog/12668.txt diff --git a/changelog/12668.txt b/changelog/12668.txt new file mode 100644 index 000000000..7006da572 --- /dev/null +++ b/changelog/12668.txt @@ -0,0 +1,3 @@ +```release-note:improvement +sdk/framework: The '+' wildcard is now supported for parameterizing unauthenticated paths. +``` diff --git a/sdk/framework/backend.go b/sdk/framework/backend.go index c2c3f1810..d498dd4ee 100644 --- a/sdk/framework/backend.go +++ b/sdk/framework/backend.go @@ -41,10 +41,8 @@ type Backend struct { // paths, including adding or removing, is not allowed once the // backend is in use). // - // PathsSpecial is the list of path patterns that denote the - // paths above that require special privileges. These can't be - // regular expressions, it is either exact match or prefix match. - // For prefix match, append '*' as a suffix. + // PathsSpecial is the list of path patterns that denote the paths above + // that require special privileges. Paths []*Path PathsSpecial *logical.Paths diff --git a/sdk/logical/logical.go b/sdk/logical/logical.go index db8831535..cec2d19c0 100644 --- a/sdk/logical/logical.go +++ b/sdk/logical/logical.go @@ -117,6 +117,10 @@ type Paths struct { Root []string // Unauthenticated are the paths that can be accessed without any auth. + // These can't be regular expressions, it is either exact match, a prefix + // match and/or a wildcard match. For prefix match, append '*' as a suffix. + // For a wildcard match, use '+' in the segment to match any identifier + // (e.g. 'foo/+/bar'). Note that '+' can't be adjacent to a non-slash. Unauthenticated []string // LocalStorage are paths (prefixes) that are local to this instance; this diff --git a/vault/identity_store.go b/vault/identity_store.go index 98a5350d9..18e3a215a 100644 --- a/vault/identity_store.go +++ b/vault/identity_store.go @@ -89,6 +89,8 @@ func NewIdentityStore(ctx context.Context, core *Core, config *logical.BackendCo PathsSpecial: &logical.Paths{ Unauthenticated: []string{ "oidc/.well-known/*", + "oidc/provider/+/.well-known/*", + "oidc/provider/+/token", }, }, PeriodicFunc: func(ctx context.Context, req *logical.Request) error { diff --git a/vault/plugin_reload.go b/vault/plugin_reload.go index 732d60bfa..e38b5341a 100644 --- a/vault/plugin_reload.go +++ b/vault/plugin_reload.go @@ -188,7 +188,11 @@ func (c *Core) reloadBackendCommon(ctx context.Context, entry *MountEntry, isAut paths := backend.SpecialPaths() if paths != nil { re.rootPaths.Store(pathsToRadix(paths.Root)) - re.loginPaths.Store(pathsToRadix(paths.Unauthenticated)) + loginPathsEntry, err := parseUnauthenticatedPaths(paths.Unauthenticated) + if err != nil { + return err + } + re.loginPaths.Store(loginPathsEntry) } } diff --git a/vault/router.go b/vault/router.go index 6d61d7f0e..6426e4eb8 100644 --- a/vault/router.go +++ b/vault/router.go @@ -3,6 +3,7 @@ package vault import ( "context" "fmt" + "regexp" "strings" "sync" "sync/atomic" @@ -22,6 +23,9 @@ var deniedPassthroughRequestHeaders = []string{ consts.AuthHeaderName, } +// matches when '+' is next to a non-slash char +var wcAdjacentNonSlashRegEx = regexp.MustCompile(`\+[^/]|[^/]\+`).MatchString + // Router is used to do prefix based routing of a request to a logical backend type Router struct { l sync.RWMutex @@ -59,6 +63,19 @@ type routeEntry struct { l sync.RWMutex } +type wildcardPath struct { + // this sits in the hot path of requests so we are micro-optimizing by + // storing pre-split slices of path segments + segments []string + isPrefix bool +} + +// loginPathsEntry is used to hold the routeEntry loginPaths +type loginPathsEntry struct { + paths *radix.Tree + wildcardPaths []wildcardPath +} + type ValidateMountResponse struct { MountType string `json:"mount_type" structs:"mount_type" mapstructure:"mount_type"` MountAccessor string `json:"mount_accessor" structs:"mount_accessor" mapstructure:"mount_accessor"` @@ -137,7 +154,11 @@ func (r *Router) Mount(backend logical.Backend, prefix string, mountEntry *Mount storageView: storageView, } re.rootPaths.Store(pathsToRadix(paths.Root)) - re.loginPaths.Store(pathsToRadix(paths.Unauthenticated)) + loginPathsEntry, err := parseUnauthenticatedPaths(paths.Unauthenticated) + if err != nil { + return err + } + re.loginPaths.Store(loginPathsEntry) switch { case prefix == "": @@ -782,6 +803,10 @@ func (r *Router) RootPath(ctx context.Context, path string) bool { } // LoginPath checks if the given path is used for logins +// Matching Priority +// 1. prefix +// 2. exact +// 3. wildcard func (r *Router) LoginPath(ctx context.Context, path string) bool { ns, err := namespace.FromContext(ctx) if err != nil { @@ -802,20 +827,114 @@ func (r *Router) LoginPath(ctx context.Context, path string) bool { remain := strings.TrimPrefix(adjustedPath, mount) // Check the loginPaths of this backend - loginPaths := re.loginPaths.Load().(*radix.Tree) - match, raw, ok := loginPaths.LongestPrefix(remain) - if !ok { + pe := re.loginPaths.Load().(*loginPathsEntry) + match, raw, ok := pe.paths.LongestPrefix(remain) + if !ok && len(pe.wildcardPaths) == 0 { + // no match found return false } - prefixMatch := raw.(bool) - // Handle the prefix match case - if prefixMatch { - return strings.HasPrefix(remain, match) + if ok { + prefixMatch := raw.(bool) + if prefixMatch { + // Handle the prefix match case + return strings.HasPrefix(remain, match) + } + if match == remain { + // Handle the exact match case + return true + } } - // Handle the exact match case - return match == remain + // check Login Paths containing wildcards + reqPathParts := strings.Split(remain, "/") + for _, w := range pe.wildcardPaths { + if pathMatchesWildcardPath(reqPathParts, w.segments, w.isPrefix) { + return true + } + } + return false +} + +// pathMatchesWildcardPath returns true if the path made up of the path slice +// matches the given wildcard path slice +func pathMatchesWildcardPath(path, wcPath []string, isPrefix bool) bool { + if len(wcPath) == 0 { + return false + } + + if len(path) < len(wcPath) { + // check if the path coming in is shorter; if so it can't match + return false + } + if !isPrefix && len(wcPath) != len(path) { + // If it's not a prefix we expect the same number of segments + return false + } + + for i, wcPathPart := range wcPath { + switch { + case wcPathPart == "+": + case wcPathPart == path[i]: + case isPrefix && i == len(wcPath)-1 && strings.HasPrefix(path[i], wcPathPart): + default: + // we encountered segments that did not match + return false + } + } + return true +} + +func wildcardError(path, msg string) error { + return fmt.Errorf("path %q: invalid use of wildcards %s", path, msg) +} + +func isValidUnauthenticatedPath(path string) (bool, error) { + switch { + case strings.Count(path, "*") > 1: + return false, wildcardError(path, "(multiple '*' is forbidden)") + case strings.Contains(path, "+*"): + return false, wildcardError(path, "('+*' is forbidden)") + case strings.Contains(path, "*") && path[len(path)-1] != '*': + return false, wildcardError(path, "('*' is only allowed at the end of a path)") + case wcAdjacentNonSlashRegEx(path): + return false, wildcardError(path, "('+' is not allowed next to a non-slash)") + } + return true, nil +} + +// parseUnauthenticatedPaths converts a list of special paths to a +// loginPathsEntry +func parseUnauthenticatedPaths(paths []string) (*loginPathsEntry, error) { + var tempPaths []string + tempWildcardPaths := make([]wildcardPath, 0) + for _, path := range paths { + if ok, err := isValidUnauthenticatedPath(path); !ok { + return nil, err + } + + if strings.Contains(path, "+") { + // Paths with wildcards are not stored in the radix tree because + // the radix tree does not handle wildcards in the middle of strings. + isPrefix := false + if path[len(path)-1] == '*' { + isPrefix = true + path = path[0 : len(path)-1] + } + // We are micro-optimizing by storing pre-split slices of path segments + wcPath := wildcardPath{segments: strings.Split(path, "/"), isPrefix: isPrefix} + tempWildcardPaths = append(tempWildcardPaths, wcPath) + } else { + // accumulate paths that do not contain wildcards + // to be stored in the radix tree + tempPaths = append(tempPaths, path) + } + } + + return &loginPathsEntry{ + paths: pathsToRadix(tempPaths), + wildcardPaths: tempWildcardPaths, + }, nil } // pathsToRadix converts a list of special paths to a radix tree. diff --git a/vault/router_test.go b/vault/router_test.go index b20b69894..842d75f62 100644 --- a/vault/router_test.go +++ b/vault/router_test.go @@ -348,6 +348,15 @@ func TestRouter_LoginPath(t *testing.T) { Login: []string{ "login", "oauth/*", + "glob1*", + "+/wildcard/glob2*", + "end1/+", + "end2/+/", + "end3/+/*", + "middle1/+/bar", + "middle2/+/+/bar", + "+/begin", + "+/around/+/", }, } err = r.Mount(n, "auth/foo/", &MountEntry{UUID: meUUID, Accessor: "authfooaccessor", NamespaceID: namespace.RootNamespaceID, namespace: namespace.RootNamespace}, view) @@ -363,8 +372,70 @@ func TestRouter_LoginPath(t *testing.T) { {"random", false}, {"auth/foo/bar", false}, {"auth/foo/login", true}, + {"auth/foo/login/", false}, {"auth/foo/oauth", false}, + {"auth/foo/oauth/", true}, {"auth/foo/oauth/redirect", true}, + {"auth/foo/oauth/redirect/", true}, + {"auth/foo/oauth/redirect/bar", true}, + {"auth/foo/glob1", true}, + {"auth/foo/glob1/", true}, + {"auth/foo/glob1/redirect", true}, + + // Wildcard cases + + // "+/wildcard/glob2*" + {"auth/foo/bar/wildcard/glo", false}, + {"auth/foo/bar/wildcard/glob2", true}, + {"auth/foo/bar/wildcard/glob2222", true}, + {"auth/foo/bar/wildcard/glob2/", true}, + {"auth/foo/bar/wildcard/glob2/baz", true}, + + // "end1/+" + {"auth/foo/end1", false}, + {"auth/foo/end1/", true}, + {"auth/foo/end1/bar", true}, + {"auth/foo/end1/bar/", false}, + {"auth/foo/end1/bar/baz", false}, + // "end2/+/" + {"auth/foo/end2", false}, + {"auth/foo/end2/", false}, + {"auth/foo/end2/bar", false}, + {"auth/foo/end2/bar/", true}, + {"auth/foo/end2/bar/baz", false}, + // "end3/+/*" + {"auth/foo/end3", false}, + {"auth/foo/end3/", false}, + {"auth/foo/end3/bar", false}, + {"auth/foo/end3/bar/", true}, + {"auth/foo/end3/bar/baz", true}, + {"auth/foo/end3/bar/baz/", true}, + {"auth/foo/end3/bar/baz/qux", true}, + {"auth/foo/end3/bar/baz/qux/qoo", true}, + {"auth/foo/end3/bar/baz/qux/qoo/qaa", true}, + // "middle1/+/bar", + {"auth/foo/middle1/bar", false}, + {"auth/foo/middle1/bar/", false}, + {"auth/foo/middle1/bar/qux", false}, + {"auth/foo/middle1/bar/bar", true}, + {"auth/foo/middle1/bar/bar/", false}, + // "middle2/+/+/bar", + {"auth/foo/middle2/bar", false}, + {"auth/foo/middle2/bar/", false}, + {"auth/foo/middle2/bar/baz", false}, + {"auth/foo/middle2/bar/baz/", false}, + {"auth/foo/middle2/bar/baz/bar", true}, + {"auth/foo/middle2/bar/baz/bar/", false}, + // "+/begin" + {"auth/foo/bar/begin", true}, + {"auth/foo/bar/begin/", false}, + {"auth/foo/begin", false}, + // "+/around/+/" + {"auth/foo/bar/around", false}, + {"auth/foo/bar/around/", false}, + {"auth/foo/bar/around/baz", false}, + {"auth/foo/bar/around/baz/", true}, + {"auth/foo/bar/around/baz/qux", false}, } for _, tc := range tcases { @@ -477,3 +548,82 @@ func TestPathsToRadix(t *testing.T) { t.Fatalf("bad: %v (sub/bar)", raw) } } + +func TestParseUnauthenticatedPaths(t *testing.T) { + // inputs + paths := []string{ + "foo", + "foo/*", + "sub/bar*", + } + wildcardPaths := []string{ + "end/+", + "+/begin/*", + "middle/+/bar*", + } + allPaths := append(paths, wildcardPaths...) + + p, err := parseUnauthenticatedPaths(allPaths) + if err != nil { + t.Fatal(err) + } + + // outputs + wildcardPathsEntry := []wildcardPath{ + {segments: []string{"end", "+"}, isPrefix: false}, + {segments: []string{"+", "begin", ""}, isPrefix: true}, + {segments: []string{"middle", "+", "bar"}, isPrefix: true}, + } + expected := &loginPathsEntry{ + paths: pathsToRadix(paths), + wildcardPaths: wildcardPathsEntry, + } + + if !reflect.DeepEqual(expected, p) { + t.Fatalf("expected: %#v\n actual: %#v\n", expected, p) + } +} + +func TestParseUnauthenticatedPaths_Error(t *testing.T) { + type tcase struct { + paths []string + err string + } + tcases := []tcase{ + { + []string{"/foo/+*"}, + "path \"/foo/+*\": invalid use of wildcards ('+*' is forbidden)", + }, + { + []string{"/foo/*/*"}, + "path \"/foo/*/*\": invalid use of wildcards (multiple '*' is forbidden)", + }, + { + []string{"*/foo/*"}, + "path \"*/foo/*\": invalid use of wildcards (multiple '*' is forbidden)", + }, + { + []string{"*/foo/"}, + "path \"*/foo/\": invalid use of wildcards ('*' is only allowed at the end of a path)", + }, + { + []string{"/foo+"}, + "path \"/foo+\": invalid use of wildcards ('+' is not allowed next to a non-slash)", + }, + { + []string{"/+foo"}, + "path \"/+foo\": invalid use of wildcards ('+' is not allowed next to a non-slash)", + }, + { + []string{"/++"}, + "path \"/++\": invalid use of wildcards ('+' is not allowed next to a non-slash)", + }, + } + + for _, tc := range tcases { + _, err := parseUnauthenticatedPaths(tc.paths) + if err == nil || err != nil && !strings.Contains(err.Error(), tc.err) { + t.Fatalf("bad: path: %s expect: %v got %v", tc.paths, tc.err, err) + } + } +}