From 9d863a92ce25c39c73812dc06bdd1cef38d96211 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Tue, 31 Jan 2023 21:27:39 +0000 Subject: [PATCH] Fix multiple OpenAPI generation issues with new AST-based generator (#18554) * Regexp metacharacter `.` should be escaped when used literally The paths including `/.well-known/` in the Vault API could currently technically be invoked with any random character in place of the dot. * Replace implementation of OpenAPI path translator with regexp AST-based one * Add changelog * Typo fix from PR review - thanks! Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> * Add comment based on review feedback * Change style of error handling as suggested in code review * Make a further tweak to the handling of the error case * Add more tests, testing cases which fail with the previous implementation * Resolve issue with a test, and improve comment --------- Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> --- changelog/18554.txt | 3 + sdk/framework/openapi.go | 293 ++++++++++++++++++-------- sdk/framework/openapi_test.go | 147 ++++++------- vault/identity_store_oidc.go | 4 +- vault/identity_store_oidc_provider.go | 4 +- vault/logical_system_test.go | 246 ++++++++++----------- 6 files changed, 384 insertions(+), 313 deletions(-) create mode 100644 changelog/18554.txt diff --git a/changelog/18554.txt b/changelog/18554.txt new file mode 100644 index 000000000..68d1d8433 --- /dev/null +++ b/changelog/18554.txt @@ -0,0 +1,3 @@ +```release-note:bug +openapi: Fix many incorrect details in generated API spec, by using better techniques to parse path regexps +``` diff --git a/sdk/framework/openapi.go b/sdk/framework/openapi.go index 050571830..692062edc 100644 --- a/sdk/framework/openapi.go +++ b/sdk/framework/openapi.go @@ -1,9 +1,11 @@ package framework import ( + "errors" "fmt" "reflect" "regexp" + "regexp/syntax" "sort" "strconv" "strings" @@ -194,24 +196,13 @@ var OASStdRespNoContent = &OASResponse{ Description: "empty body", } -// Regex for handling optional and named parameters in paths, and string cleanup. +// Regex for handling fields in paths, and string cleanup. // Predefined here to avoid substantial recompilation. -// Capture optional path elements in ungreedy (?U) fashion -// Both "(leases/)?renew" and "(/(?P.+))?" formats are detected -var optRe = regexp.MustCompile(`(?U)\([^(]*\)\?|\(/\(\?P<[^(]*\)\)\?`) - var ( - altFieldsGroupRe = regexp.MustCompile(`\(\?P<\w+>\w+(\|\w+)+\)`) // Match named groups that limit options, e.g. "(?a|b|c)" - altFieldsRe = regexp.MustCompile(`\w+(\|\w+)+`) // Match an options set, e.g. "a|b|c" - altRe = regexp.MustCompile(`\((.*)\|(.*)\)`) // Capture alternation elements, e.g. "(raw/?$|raw/(?P.+))" - altRootsRe = regexp.MustCompile(`^\(([\w\-_]+(?:\|[\w\-_]+)+)\)(/.*)$`) // Pattern starting with alts, e.g. "(root1|root2)/(?Pregex)" - cleanCharsRe = regexp.MustCompile("[()^$?]") // Set of regex characters that will be stripped during cleaning - cleanSuffixRe = regexp.MustCompile(`/\?\$?$`) // Path suffix patterns that will be stripped during cleaning - nonWordRe = regexp.MustCompile(`[^\w]+`) // Match a sequence of non-word characters - pathFieldsRe = regexp.MustCompile(`{(\w+)}`) // Capture OpenAPI-style named parameters, e.g. "lookup/{urltoken}", - reqdRe = regexp.MustCompile(`\(?\?P<(\w+)>[^)]*\)?`) // Capture required parameters, e.g. "(?Pregex)" - wsRe = regexp.MustCompile(`\s+`) // Match whitespace, to be compressed during cleaning + nonWordRe = regexp.MustCompile(`[^\w]+`) // Match a sequence of non-word characters + pathFieldsRe = regexp.MustCompile(`{(\w+)}`) // Capture OpenAPI-style named parameters, e.g. "lookup/{urltoken}", + wsRe = regexp.MustCompile(`\s+`) // Match whitespace, to be compressed during cleaning ) // documentPaths parses all paths in a framework.Backend into OpenAPI paths. @@ -236,7 +227,22 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st } // Convert optional parameters into distinct patterns to be processed independently. - paths := expandPattern(p.Pattern) + forceUnpublished := false + paths, err := expandPattern(p.Pattern) + if err != nil { + if errors.Is(err, errUnsupportableRegexpOperationForOpenAPI) { + // Pattern cannot be transformed into sensible OpenAPI paths. In this case, we override the later + // processing to use the regexp, as is, as the path, and behave as if Unpublished was set on every + // operation (meaning the operations will not be represented in the OpenAPI document). + // + // This allows a human reading the OpenAPI document to notice that, yes, a path handler does exist, + // even though it was not able to contribute actual OpenAPI operations. + forceUnpublished = true + paths = []string{p.Pattern} + } else { + return err + } + } for _, path := range paths { // Construct a top level PathItem which will be populated as the path is processed. @@ -305,7 +311,7 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st // with descriptions, properties and examples from the framework.Path data. for opType, opHandler := range operations { props := opHandler.Properties() - if props.Unpublished { + if props.Unpublished || forceUnpublished { continue } @@ -554,85 +560,198 @@ func specialPathMatch(path string, specialPaths []string) bool { // expandPattern expands a regex pattern by generating permutations of any optional parameters // and changing named parameters into their {openapi} equivalents. -func expandPattern(pattern string) []string { - var paths []string - - // Determine if the pattern starts with an alternation for multiple roots - // example (root1|root2)/(?Pregex) -> match['(root1|root2)/(?Pregex)','root1|root2','/(?Pregex)'] - match := altRootsRe.FindStringSubmatch(pattern) - if len(match) == 3 { - var expandedRoots []string - for _, root := range strings.Split(match[1], "|") { - expandedRoots = append(expandedRoots, expandPattern(root+match[2])...) - } - return expandedRoots +func expandPattern(pattern string) ([]string, error) { + // Happily, the Go regexp library exposes its underlying "parse to AST" functionality, so we can rely on that to do + // the hard work of interpreting the regexp syntax. + rx, err := syntax.Parse(pattern, syntax.Perl) + if err != nil { + // This should be impossible to reach, since regexps have previously been compiled with MustCompile in + // Backend.init. + panic(err) } - // GenericNameRegex adds a regex that complicates our parsing. It is much easier to - // detect and remove it now than to compensate for in the other regexes. + paths, err := collectPathsFromRegexpAST(rx) + if err != nil { + return nil, err + } + + return paths, nil +} + +type pathCollector struct { + strings.Builder + conditionalSlashAppendedAtLength int +} + +// collectPathsFromRegexpAST performs a depth-first recursive walk through a regexp AST, collecting an OpenAPI-style +// path as it goes. +// +// Each time it encounters alternation (a|b) or an optional part (a?), it forks its processing to produce additional +// results, to account for each possibility. Note: This does mean that an input pattern with lots of these regexp +// features can produce a lot of different OpenAPI endpoints. At the time of writing, the most complex known example is +// +// "issuer/" + framework.GenericNameRegex(issuerRefParam) + "/crl(/pem|/der|/delta(/pem|/der)?)?" +// +// in the PKI secrets engine which expands to 6 separate paths. +// +// Each named capture group - i.e. (?Psomething here) - is replaced with an OpenAPI parameter - i.e. {name} - and +// the subtree of regexp AST inside the parameter is completely skipped. +func collectPathsFromRegexpAST(rx *syntax.Regexp) ([]string, error) { + pathCollectors, err := collectPathsFromRegexpASTInternal(rx, []*pathCollector{{}}) + if err != nil { + return nil, err + } + paths := make([]string, 0, len(pathCollectors)) + for _, collector := range pathCollectors { + if collector.conditionalSlashAppendedAtLength != collector.Len() { + paths = append(paths, collector.String()) + } + } + return paths, nil +} + +var errUnsupportableRegexpOperationForOpenAPI = errors.New("path regexp uses an operation that cannot be translated to an OpenAPI pattern") + +func collectPathsFromRegexpASTInternal(rx *syntax.Regexp, appendingTo []*pathCollector) ([]*pathCollector, error) { + var err error + + // Depending on the type of this regexp AST node (its Op, i.e. operation), figure out whether it contributes any + // characters to the URL path, and whether we need to recurse through child AST nodes. // - // example: (?P\\w(([\\w-.]+)?\\w)?) -> (?P) - base := GenericNameRegex("") - start := strings.Index(base, ">") - end := strings.LastIndex(base, ")") - regexToRemove := "" - if start != -1 && end != -1 && end > start { - regexToRemove = base[start+1 : end] - } - pattern = strings.ReplaceAll(pattern, regexToRemove, "") + // Each element of the appendingTo slice tracks a separate path, defined by the alternatives chosen when traversing + // the | and ? conditional regexp features, and new elements are added as each of these features are traversed. + // + // To share this slice across multiple recursive calls of this function, it is passed down as a parameter to each + // recursive call, potentially modified throughout this switch block, and passed back up as a return value at the + // end of this function - the parent call uses the return value to update its own local variable. + switch rx.Op { - // Simplify named fields that have limited options, e.g. (?Pa|b|c) -> (.+) - pattern = altFieldsGroupRe.ReplaceAllStringFunc(pattern, func(s string) string { - return altFieldsRe.ReplaceAllString(s, ".+") - }) + // These AST operations are leaf nodes (no children), that match zero characters, so require no processing at all + case syntax.OpEmptyMatch: // e.g. (?:) + case syntax.OpBeginLine: // i.e. ^ when (?m) + case syntax.OpEndLine: // i.e. $ when (?m) + case syntax.OpBeginText: // i.e. \A, or ^ when (?-m) + case syntax.OpEndText: // i.e. \z, or $ when (?-m) + case syntax.OpWordBoundary: // i.e. \b + case syntax.OpNoWordBoundary: // i.e. \B - // Initialize paths with the original pattern or the halves of an - // alternation, which is also present in some patterns. - matches := altRe.FindAllStringSubmatch(pattern, -1) - if len(matches) > 0 { - paths = []string{matches[0][1], matches[0][2]} - } else { - paths = []string{pattern} - } - - // Expand all optional regex elements into two paths. This approach is really only useful up to 2 optional - // groups, but we probably don't want to deal with the exponential increase beyond that anyway. - for i := 0; i < len(paths); i++ { - p := paths[i] - - // match is a 2-element slice that will have a start and end index - // for the left-most match of a regex of form: (lease/)? - match := optRe.FindStringIndex(p) - - if match != nil { - // create a path that includes the optional element but without - // parenthesis or the '?' character. - paths[i] = p[:match[0]] + p[match[0]+1:match[1]-2] + p[match[1]:] - - // create a path that excludes the optional element. - paths = append(paths, p[:match[0]]+p[match[1]:]) - i-- - } - } - - // Replace named parameters (?P) with {foo} - var replacedPaths []string - - for _, path := range paths { - result := reqdRe.FindAllStringSubmatch(path, -1) - if result != nil { - for _, p := range result { - par := p[1] - path = strings.Replace(path, p[0], fmt.Sprintf("{%s}", par), 1) + // OpConcat simply represents multiple parts of the pattern appearing one after the other, so just recurse through + // those pieces. + case syntax.OpConcat: + for _, child := range rx.Sub { + appendingTo, err = collectPathsFromRegexpASTInternal(child, appendingTo) + if err != nil { + return nil, err } } - // Final cleanup - path = cleanSuffixRe.ReplaceAllString(path, "") - path = cleanCharsRe.ReplaceAllString(path, "") - replacedPaths = append(replacedPaths, path) + + // OpLiteral is a literal string in the pattern - append it to the paths we are building. + case syntax.OpLiteral: + for _, collector := range appendingTo { + collector.WriteString(string(rx.Rune)) + } + + // OpAlternate, i.e. a|b, means we clone all of the pathCollector instances we are currently accumulating paths + // into, and independently recurse through each alternate option. + case syntax.OpAlternate: // i.e | + var totalAppendingTo []*pathCollector + lastIndex := len(rx.Sub) - 1 + for index, child := range rx.Sub { + var childAppendingTo []*pathCollector + if index == lastIndex { + // Optimization: last time through this loop, we can simply re-use the existing set of pathCollector + // instances, as we no longer need to preserve them unmodified to make further copies of. + childAppendingTo = appendingTo + } else { + for _, collector := range appendingTo { + newCollector := new(pathCollector) + newCollector.WriteString(collector.String()) + newCollector.conditionalSlashAppendedAtLength = collector.conditionalSlashAppendedAtLength + childAppendingTo = append(childAppendingTo, newCollector) + } + } + childAppendingTo, err = collectPathsFromRegexpASTInternal(child, childAppendingTo) + if err != nil { + return nil, err + } + totalAppendingTo = append(totalAppendingTo, childAppendingTo...) + } + appendingTo = totalAppendingTo + + // OpQuest, i.e. a?, is much like an alternation between exactly two options, one of which is the empty string. + case syntax.OpQuest: + child := rx.Sub[0] + var childAppendingTo []*pathCollector + for _, collector := range appendingTo { + newCollector := new(pathCollector) + newCollector.WriteString(collector.String()) + newCollector.conditionalSlashAppendedAtLength = collector.conditionalSlashAppendedAtLength + childAppendingTo = append(childAppendingTo, newCollector) + } + childAppendingTo, err = collectPathsFromRegexpASTInternal(child, childAppendingTo) + if err != nil { + return nil, err + } + appendingTo = append(appendingTo, childAppendingTo...) + + // Many Vault path patterns end with `/?` to accept paths that end with or without a slash. Our current + // convention for generating the OpenAPI is to strip away these slashes. To do that, this very special case + // detects when we just appended a single conditional slash, and records the length of the path at this point, + // so we can later discard this path variant, if nothing else is appended to it later. + if child.Op == syntax.OpLiteral && string(child.Rune) == "/" { + for _, collector := range childAppendingTo { + collector.conditionalSlashAppendedAtLength = collector.Len() + } + } + + // OpCapture, i.e. ( ) or (?P ), a capturing group + case syntax.OpCapture: + if rx.Name == "" { + // In Vault, an unnamed capturing group is not actually used for capturing. + // We treat it exactly the same as OpConcat. + for _, child := range rx.Sub { + appendingTo, err = collectPathsFromRegexpASTInternal(child, appendingTo) + if err != nil { + return nil, err + } + } + } else { + // A named capturing group is replaced with the OpenAPI parameter syntax, and the regexp inside the group + // is NOT added to the OpenAPI path. + for _, builder := range appendingTo { + builder.WriteRune('{') + builder.WriteString(rx.Name) + builder.WriteRune('}') + } + } + + // Any other kind of operation is a problem, and will trigger an error, resulting in the pattern being left out of + // the OpenAPI entirely - that's better than generating a path which is incorrect. + // + // The Op types we expect to hit the default condition are: + // + // OpCharClass - i.e. [something] + // OpAnyCharNotNL - i.e. . + // OpAnyChar - i.e. (?s:.) + // OpStar - i.e. * + // OpPlus - i.e. + + // OpRepeat - i.e. {N}, {N,M}, etc. + // + // In any of these conditions, there is no sensible translation of the path to OpenAPI syntax. (Note, this only + // applies to these appearing outside of a named capture group, otherwise they are handled in the previous case.) + // + // At the time of writing, the only pattern in the builtin Vault plugins that hits this codepath is the ".*" + // pattern in the KVv2 secrets engine, which is not a valid path, but rather, is a catch-all used to implement + // custom error handling behaviour to guide users who attempt to treat a KVv2 as a KVv1. It is already marked as + // Unpublished, so is withheld from the OpenAPI anyway. + // + // For completeness, one other Op type exists, OpNoMatch, which is never generated by syntax.Parse - only by + // subsequent Simplify in preparation to Compile, which is not used here. + default: + return nil, errUnsupportableRegexpOperationForOpenAPI } - return replacedPaths + return appendingTo, nil } // schemaType is a subset of the JSON Schema elements used as a target diff --git a/sdk/framework/openapi_test.go b/sdk/framework/openapi_test.go index f37c6ecb9..3d5789e9d 100644 --- a/sdk/framework/openapi_test.go +++ b/sdk/framework/openapi_test.go @@ -18,75 +18,6 @@ import ( ) func TestOpenAPI_Regex(t *testing.T) { - t.Run("Required", func(t *testing.T) { - tests := []struct { - input string - captures []string - }{ - {`/foo/bar/(?P.*)`, []string{"val"}}, - {`/foo/bar/` + GenericNameRegex("val"), []string{"val"}}, - {`/foo/bar/` + GenericNameRegex("first") + "/b/" + GenericNameRegex("second"), []string{"first", "second"}}, - {`/foo/bar`, []string{}}, - } - - for _, test := range tests { - result := reqdRe.FindAllStringSubmatch(test.input, -1) - if len(result) != len(test.captures) { - t.Fatalf("Capture error (%s): expected %d matches, actual: %d", test.input, len(test.captures), len(result)) - } - - for i := 0; i < len(result); i++ { - if result[i][1] != test.captures[i] { - t.Fatalf("Capture error (%s): expected %s, actual: %s", test.input, test.captures[i], result[i][1]) - } - } - } - }) - t.Run("Optional", func(t *testing.T) { - input := "foo/(maybe/)?bar" - expStart := len("foo/") - expEnd := len(input) - len("bar") - - match := optRe.FindStringIndex(input) - if diff := deep.Equal(match, []int{expStart, expEnd}); diff != nil { - t.Fatal(diff) - } - - input = "/foo/maybe/bar" - match = optRe.FindStringIndex(input) - if match != nil { - t.Fatalf("Expected nil match (%s), got %+v", input, match) - } - }) - t.Run("Alternation", func(t *testing.T) { - input := `(raw/?$|raw/(?P.+))` - - matches := altRe.FindAllStringSubmatch(input, -1) - exp1 := "raw/?$" - exp2 := "raw/(?P.+)" - if matches[0][1] != exp1 || matches[0][2] != exp2 { - t.Fatalf("Capture error. Expected %s and %s, got %v", exp1, exp2, matches[0][1:]) - } - - input = `/foo/bar/` + GenericNameRegex("val") - - matches = altRe.FindAllStringSubmatch(input, -1) - if matches != nil { - t.Fatalf("Expected nil match (%s), got %+v", input, matches) - } - }) - t.Run("Alternation Fields", func(t *testing.T) { - input := `/foo/bar/(?Pauth|database|secret)/(?Pa|b)` - - act := altFieldsGroupRe.ReplaceAllStringFunc(input, func(s string) string { - return altFieldsRe.ReplaceAllString(s, ".+") - }) - - exp := "/foo/bar/(?P.+)/(?P.+)" - if act != exp { - t.Fatalf("Replace error. Expected %s, got %v", exp, act) - } - }) t.Run("Path fields", func(t *testing.T) { input := `/foo/bar/{inner}/baz/{outer}` @@ -111,21 +42,6 @@ func TestOpenAPI_Regex(t *testing.T) { regex *regexp.Regexp output string }{ - { - input: `ab?cde^fg(hi?j$k`, - regex: cleanCharsRe, - output: "abcdefghijk", - }, - { - input: `abcde/?`, - regex: cleanSuffixRe, - output: "abcde", - }, - { - input: `abcde/?$`, - regex: cleanSuffixRe, - output: "abcde", - }, { input: `abcde`, regex: wsRe, @@ -152,62 +68,99 @@ func TestOpenAPI_ExpandPattern(t *testing.T) { inPattern string outPathlets []string }{ + // A simple string without regexp metacharacters passes through as is {"rekey/backup", []string{"rekey/backup"}}, + // A trailing regexp anchor metacharacter is removed {"rekey/backup$", []string{"rekey/backup"}}, + // As is a leading one + {"^rekey/backup", []string{"rekey/backup"}}, + // Named capture groups become OpenAPI parameters {"auth/(?P.+?)/tune$", []string{"auth/{path}/tune"}}, {"auth/(?P.+?)/tune/(?P.*?)$", []string{"auth/{path}/tune/{more}"}}, + // Even if the capture group contains very complex regexp structure inside it + {"something/(?P(a|b(c|d))|e+|f{1,3}[ghi-k]?.*)", []string{"something/{something}"}}, + // A question-mark results in a result without and with the optional path part {"tools/hash(/(?P.+))?", []string{ "tools/hash", "tools/hash/{urlalgorithm}", }}, + // Multiple question-marks evaluate each possible combination {"(leases/)?renew(/(?P.+))?", []string{ "leases/renew", "leases/renew/{url_lease_id}", "renew", "renew/{url_lease_id}", }}, + // GenericNameRegex is one particular way of writing a named capture group, so behaves the same {`config/ui/headers/` + GenericNameRegex("header"), []string{"config/ui/headers/{header}"}}, + // The question-mark behaviour is still works when the question-mark is directly applied to a named capture group {`leases/lookup/(?P.+?)?`, []string{ "leases/lookup/", "leases/lookup/{prefix}", }}, + // Optional trailing slashes at the end of the path get stripped - even if appearing deep inside an alternation {`(raw/?$|raw/(?P.+))`, []string{ "raw", "raw/{path}", }}, + // OptionalParamRegex is also another way of writing a named capture group, that is optional {"lookup" + OptionalParamRegex("urltoken"), []string{ "lookup", "lookup/{urltoken}", }}, + // Optional trailign slashes at the end of the path get stripped in simpler cases too {"roles/?$", []string{ "roles", }}, {"roles/?", []string{ "roles", }}, + // Non-optional trailing slashes remain... although don't do this, it breaks HelpOperation! + // (Existing real examples of this pattern being fixed via https://github.com/hashicorp/vault/pull/18571) {"accessors/$", []string{ "accessors/", }}, + // GenericNameRegex and OptionalParamRegex still work when concatenated {"verify/" + GenericNameRegex("name") + OptionalParamRegex("urlalgorithm"), []string{ "verify/{name}", "verify/{name}/{urlalgorithm}", }}, + // Named capture groups that specify enum-like parameters work as expected {"^plugins/catalog/(?Pauth|database|secret)/(?P.+)$", []string{ "plugins/catalog/{type}/{name}", }}, {"^plugins/catalog/(?Pauth|database|secret)/?$", []string{ "plugins/catalog/{type}", }}, + // Alternations between various literal path segments work {"(pathOne|pathTwo)/", []string{"pathOne/", "pathTwo/"}}, {"(pathOne|pathTwo)/" + GenericNameRegex("name"), []string{"pathOne/{name}", "pathTwo/{name}"}}, { "(pathOne|path-2|Path_3)/" + GenericNameRegex("name"), []string{"Path_3/{name}", "path-2/{name}", "pathOne/{name}"}, }, + // They still work when combined with GenericNameWithAtRegex + {"(creds|sts)/" + GenericNameWithAtRegex("name"), []string{ + "creds/{name}", + "sts/{name}", + }}, + // And when they're somewhere other than the start of the pattern + {"keys/generate/(internal|exported|kms)", []string{ + "keys/generate/exported", + "keys/generate/internal", + "keys/generate/kms", + }}, + // If a plugin author makes their list operation support both singular and plural forms, the OpenAPI notices + {"rolesets?/?", []string{"roleset", "rolesets"}}, + // Complex nested alternation and question-marks are correctly interpreted + {"crl(/pem|/delta(/pem)?)?", []string{"crl", "crl/delta", "crl/delta/pem", "crl/pem"}}, } for i, test := range tests { - out := expandPattern(test.inPattern) + out, err := expandPattern(test.inPattern) + if err != nil { + t.Fatal(err) + } sort.Strings(out) if !reflect.DeepEqual(out, test.outPathlets) { t.Fatalf("Test %d: Expected %v got %v", i, test.outPathlets, out) @@ -215,6 +168,30 @@ func TestOpenAPI_ExpandPattern(t *testing.T) { } } +func TestOpenAPI_ExpandPattern_ReturnsError(t *testing.T) { + tests := []struct { + inPattern string + outError error + }{ + // None of these regexp constructs are allowed outside of named capture groups + {"[a-z]", errUnsupportableRegexpOperationForOpenAPI}, + {".", errUnsupportableRegexpOperationForOpenAPI}, + {"a+", errUnsupportableRegexpOperationForOpenAPI}, + {"a*", errUnsupportableRegexpOperationForOpenAPI}, + // So this pattern, which is a combination of two of the above isn't either - this pattern occurs in the KV + // secrets engine for its catch-all error handler, which provides a helpful hint to people treating a KV v2 as + // a KV v1. + {".*", errUnsupportableRegexpOperationForOpenAPI}, + } + + for i, test := range tests { + _, err := expandPattern(test.inPattern) + if err != test.outError { + t.Fatalf("Test %d: Expected %q got %q", i, test.outError, err) + } + } +} + func TestOpenAPI_SplitFields(t *testing.T) { fields := map[string]*FieldSchema{ "a": {Description: "path"}, diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 627dd3828..377af0fd5 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -216,7 +216,7 @@ func oidcPaths(i *IdentityStore) []*framework.Path { HelpDescription: "List all named OIDC keys", }, { - Pattern: "oidc/.well-known/openid-configuration/?$", + Pattern: "oidc/\\.well-known/openid-configuration/?$", Callbacks: map[logical.Operation]framework.OperationFunc{ logical.ReadOperation: i.pathOIDCDiscovery, }, @@ -224,7 +224,7 @@ func oidcPaths(i *IdentityStore) []*framework.Path { HelpDescription: "Query this path to retrieve the configured OIDC Issuer and Keys endpoints, response types, subject types, and signing algorithms used by the OIDC backend.", }, { - Pattern: "oidc/.well-known/keys/?$", + Pattern: "oidc/\\.well-known/keys/?$", Callbacks: map[logical.Operation]framework.OperationFunc{ logical.ReadOperation: i.pathOIDCReadPublicKeys, }, diff --git a/vault/identity_store_oidc_provider.go b/vault/identity_store_oidc_provider.go index 8de2376f5..39107f6a5 100644 --- a/vault/identity_store_oidc_provider.go +++ b/vault/identity_store_oidc_provider.go @@ -389,7 +389,7 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path { HelpDescription: "List all configured OIDC providers in the identity backend.", }, { - Pattern: "oidc/provider/" + framework.GenericNameRegex("name") + "/.well-known/openid-configuration", + Pattern: "oidc/provider/" + framework.GenericNameRegex("name") + "/\\.well-known/openid-configuration", Fields: map[string]*framework.FieldSchema{ "name": { Type: framework.TypeString, @@ -405,7 +405,7 @@ func oidcProviderPaths(i *IdentityStore) []*framework.Path { HelpDescription: "Query this path to retrieve the configured OIDC Issuer and Keys endpoints, response types, subject types, and signing algorithms used by the OIDC backend.", }, { - Pattern: "oidc/provider/" + framework.GenericNameRegex("name") + "/.well-known/keys", + Pattern: "oidc/provider/" + framework.GenericNameRegex("name") + "/\\.well-known/keys", Fields: map[string]*framework.FieldSchema{ "name": { Type: framework.TypeString, diff --git a/vault/logical_system_test.go b/vault/logical_system_test.go index 7903ad67f..2a0e3cf4f 100644 --- a/vault/logical_system_test.go +++ b/vault/logical_system_test.go @@ -3618,158 +3618,130 @@ func TestSystemBackend_InternalUIMount(t *testing.T) { } } -func TestSystemBackend_OASGenericMount(t *testing.T) { - _, b, rootToken := testCoreSystemBackend(t) - var oapi map[string]interface{} - - // Check that default paths are present with a root token - req := logical.TestRequest(t, logical.ReadOperation, "internal/specs/openapi") - req.Data["generic_mount_paths"] = true - req.ClientToken = rootToken - resp, err := b.HandleRequest(namespace.RootContext(nil), req) - if err != nil { - t.Fatalf("err: %v", err) - } - - body := resp.Data["http_raw_body"].([]byte) - err = jsonutil.DecodeJSON(body, &oapi) - if err != nil { - t.Fatalf("err: %v", err) - } - - doc, err := framework.NewOASDocumentFromMap(oapi) - if err != nil { - t.Fatal(err) - } - - pathSamples := []struct { - path string - tag string - }{ - {"/auth/token/lookup", "auth"}, - {"/cubbyhole/{path}", "secrets"}, - {"/identity/group/id", "identity"}, - {"/{secret_mount_path}/.*", "secrets"}, - {"/sys/policy", "system"}, - } - - for _, path := range pathSamples { - if doc.Paths[path.path] == nil { - t.Fatalf("didn't find expected path '%s'.", path) - } - tag := doc.Paths[path.path].Get.Tags[0] - if tag != path.tag { - t.Fatalf("path: %s; expected tag: %s, actual: %s", path.path, tag, path.tag) - } - } - - // Simple check of response size (which is much larger than most - // Vault responses), mainly to catch mass omission of expected path data. - const minLen = 70000 - if len(body) < minLen { - t.Fatalf("response size too small; expected: min %d, actual: %d", minLen, len(body)) - } -} - func TestSystemBackend_OpenAPI(t *testing.T) { _, b, rootToken := testCoreSystemBackend(t) - var oapi map[string]interface{} // Ensure no paths are reported if there is no token - req := logical.TestRequest(t, logical.ReadOperation, "internal/specs/openapi") - resp, err := b.HandleRequest(namespace.RootContext(nil), req) - if err != nil { - t.Fatalf("err: %v", err) - } + { + req := logical.TestRequest(t, logical.ReadOperation, "internal/specs/openapi") + resp, err := b.HandleRequest(namespace.RootContext(nil), req) + if err != nil { + t.Fatalf("err: %v", err) + } - body := resp.Data["http_raw_body"].([]byte) - err = jsonutil.DecodeJSON(body, &oapi) - if err != nil { - t.Fatalf("err: %v", err) - } - exp := map[string]interface{}{ - "openapi": framework.OASVersion, - "info": map[string]interface{}{ - "title": "HashiCorp Vault API", - "description": "HTTP API that gives you full access to Vault. All API routes are prefixed with `/v1/`.", - "version": version.GetVersion().Version, - "license": map[string]interface{}{ - "name": "Mozilla Public License 2.0", - "url": "https://www.mozilla.org/en-US/MPL/2.0", + body := resp.Data["http_raw_body"].([]byte) + var oapi map[string]interface{} + err = jsonutil.DecodeJSON(body, &oapi) + if err != nil { + t.Fatalf("err: %v", err) + } + exp := map[string]interface{}{ + "openapi": framework.OASVersion, + "info": map[string]interface{}{ + "title": "HashiCorp Vault API", + "description": "HTTP API that gives you full access to Vault. All API routes are prefixed with `/v1/`.", + "version": version.GetVersion().Version, + "license": map[string]interface{}{ + "name": "Mozilla Public License 2.0", + "url": "https://www.mozilla.org/en-US/MPL/2.0", + }, + }, + "paths": map[string]interface{}{}, + "components": map[string]interface{}{ + "schemas": map[string]interface{}{}, }, - }, - "paths": map[string]interface{}{}, - "components": map[string]interface{}{ - "schemas": map[string]interface{}{}, - }, - } - - if diff := deep.Equal(oapi, exp); diff != nil { - t.Fatal(diff) - } - - // Check that default paths are present with a root token - req = logical.TestRequest(t, logical.ReadOperation, "internal/specs/openapi") - req.ClientToken = rootToken - resp, err = b.HandleRequest(namespace.RootContext(nil), req) - if err != nil { - t.Fatalf("err: %v", err) - } - - body = resp.Data["http_raw_body"].([]byte) - err = jsonutil.DecodeJSON(body, &oapi) - if err != nil { - t.Fatalf("err: %v", err) - } - - doc, err := framework.NewOASDocumentFromMap(oapi) - if err != nil { - t.Fatal(err) - } - - pathSamples := []struct { - path string - tag string - }{ - {"/auth/token/lookup", "auth"}, - {"/cubbyhole/{path}", "secrets"}, - {"/identity/group/id", "identity"}, - {"/secret/.*", "secrets"}, - {"/sys/policy", "system"}, - } - - for _, path := range pathSamples { - if doc.Paths[path.path] == nil { - t.Fatalf("didn't find expected path %q.", path) } - tag := doc.Paths[path.path].Get.Tags[0] - if tag != path.tag { - t.Fatalf("path: %s; expected tag: %s, actual: %s", path.path, tag, path.tag) + + if diff := deep.Equal(oapi, exp); diff != nil { + t.Fatal(diff) } } - // Simple check of response size (which is much larger than most - // Vault responses), mainly to catch mass omission of expected path data. - const minLen = 70000 - if len(body) < minLen { - t.Fatalf("response size too small; expected: min %d, actual: %d", minLen, len(body)) + // Check that default paths are present with a root token (with and without generic_mount_paths) + for _, genericMountPaths := range []bool{false, true} { + req := logical.TestRequest(t, logical.ReadOperation, "internal/specs/openapi") + if genericMountPaths { + req.Data["generic_mount_paths"] = true + } + req.ClientToken = rootToken + resp, err := b.HandleRequest(namespace.RootContext(nil), req) + if err != nil { + t.Fatalf("err: %v", err) + } + + body := resp.Data["http_raw_body"].([]byte) + var oapi map[string]interface{} + err = jsonutil.DecodeJSON(body, &oapi) + if err != nil { + t.Fatalf("err: %v", err) + } + + doc, err := framework.NewOASDocumentFromMap(oapi) + if err != nil { + t.Fatal(err) + } + + expectedSecretPrefix := "/secret/" + if genericMountPaths { + expectedSecretPrefix = "/{secret_mount_path}/" + } + + pathSamples := []struct { + path string + tag string + unpublished bool + }{ + {path: "/auth/token/lookup", tag: "auth"}, + {path: "/cubbyhole/{path}", tag: "secrets"}, + {path: "/identity/group/id", tag: "identity"}, + {path: expectedSecretPrefix + "^.*$", unpublished: true}, + {path: "/sys/policy", tag: "system"}, + } + + for _, path := range pathSamples { + if doc.Paths[path.path] == nil { + t.Fatalf("didn't find expected path %q.", path.path) + } + getOperation := doc.Paths[path.path].Get + if getOperation == nil && !path.unpublished { + t.Fatalf("path: %s; expected a get operation, but it was absent", path.path) + } + if getOperation != nil && path.unpublished { + t.Fatalf("path: %s; expected absent get operation, but it was present", path.path) + } + if !path.unpublished { + tag := getOperation.Tags[0] + if tag != path.tag { + t.Fatalf("path: %s; expected tag: %s, actual: %s", path.path, tag, path.tag) + } + } + } + + // Simple check of response size (which is much larger than most + // Vault responses), mainly to catch mass omission of expected path data. + const minLen = 70000 + if len(body) < minLen { + t.Fatalf("response size too small; expected: min %d, actual: %d", minLen, len(body)) + } } // Test path-help response - req = logical.TestRequest(t, logical.HelpOperation, "rotate") - req.ClientToken = rootToken - resp, err = b.HandleRequest(namespace.RootContext(nil), req) - if err != nil { - t.Fatalf("err: %v", err) - } + { + req := logical.TestRequest(t, logical.HelpOperation, "rotate") + req.ClientToken = rootToken + resp, err := b.HandleRequest(namespace.RootContext(nil), req) + if err != nil { + t.Fatalf("err: %v", err) + } - doc = resp.Data["openapi"].(*framework.OASDocument) - if len(doc.Paths) != 1 { - t.Fatalf("expected 1 path, actual: %d", len(doc.Paths)) - } + doc := resp.Data["openapi"].(*framework.OASDocument) + if len(doc.Paths) != 1 { + t.Fatalf("expected 1 path, actual: %d", len(doc.Paths)) + } - if doc.Paths["/rotate"] == nil { - t.Fatalf("expected to find path '/rotate'") + if doc.Paths["/rotate"] == nil { + t.Fatalf("expected to find path '/rotate'") + } } }