From d65365af46e4ecbbcc186623b67f2e19bfbb31c1 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Thu, 29 Dec 2022 09:08:50 +0000 Subject: [PATCH 1/5] Fix `vault path-help` for selected paths with bad regexps See the comment being added in `sdk/framework/path.go` for the explanation of why this change is needed. --- sdk/framework/path.go | 11 +++++++++++ vault/logical_system_paths.go | 4 ++-- vault/logical_system_pprof.go | 2 +- vault/token_store.go | 2 +- 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/sdk/framework/path.go b/sdk/framework/path.go index 80f4d5dc6c3e..3f7a00932a2f 100644 --- a/sdk/framework/path.go +++ b/sdk/framework/path.go @@ -53,6 +53,17 @@ type Path struct { // This should be a valid regular expression. Named captures will be // exposed as fields that should map to a schema in Fields. If a named // capture is not a field in the Fields map, then it will be ignored. + // + // The pattern will automatically have a ^ prepending and a $ appended before + // use, if these are not already present, so these may be omitted for clarity. + // + // If a ListOperation is being defined, the pattern must end with /? to match + // a trailing slash optionally, as ListOperations are always processed with a + // trailing slash added to the path if not already present. The match must not + // require the presence of a trailing slash, as HelpOperations, even for a + // path which only implements ListOperation, are processed without a trailing + // slash - so failure to make the trailing slash optional will break the + // `vault path-help` command for the path. Pattern string // Fields is the mapping of data fields to a schema describing that diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index 53a3c20d34f8..6cb57661ba3e 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -114,7 +114,7 @@ func (b *SystemBackend) configPaths() []*framework.Path { }, { - Pattern: "config/ui/headers/$", + Pattern: "config/ui/headers/?$", Operations: map[logical.Operation]framework.OperationHandler{ logical.ListOperation: &framework.PathOperation{ @@ -542,7 +542,7 @@ func (b *SystemBackend) statusPaths() []*framework.Path { HelpDescription: strings.TrimSpace(sysHelp["ha-status"][1]), }, { - Pattern: "version-history/$", + Pattern: "version-history/?$", Operations: map[logical.Operation]framework.OperationHandler{ logical.ListOperation: &framework.PathOperation{ Callback: b.handleVersionHistoryList, diff --git a/vault/logical_system_pprof.go b/vault/logical_system_pprof.go index ce7fc4b27a44..c3a719451edf 100644 --- a/vault/logical_system_pprof.go +++ b/vault/logical_system_pprof.go @@ -14,7 +14,7 @@ import ( func (b *SystemBackend) pprofPaths() []*framework.Path { return []*framework.Path{ { - Pattern: "pprof/$", + Pattern: "pprof/?$", Operations: map[logical.Operation]framework.OperationHandler{ logical.ReadOperation: &framework.PathOperation{ diff --git a/vault/token_store.go b/vault/token_store.go index f8e7b63b2416..376edcbe461d 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -148,7 +148,7 @@ func (ts *TokenStore) paths() []*framework.Path { }, { - Pattern: "accessors/$", + Pattern: "accessors/?$", Callbacks: map[logical.Operation]framework.OperationFunc{ logical.ListOperation: ts.tokenStoreAccessorList, From 8f30ede0ce6d10b2c47abd44ff4e5741ceb41fd2 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Thu, 29 Dec 2022 09:36:05 +0000 Subject: [PATCH 2/5] Grammar fix and add changelog --- changelog/18571.txt | 3 +++ sdk/framework/path.go | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changelog/18571.txt diff --git a/changelog/18571.txt b/changelog/18571.txt new file mode 100644 index 000000000000..dd811d9fd441 --- /dev/null +++ b/changelog/18571.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth/token, sys: Fix path-help being unavailable for some list-only endpoints +``` diff --git a/sdk/framework/path.go b/sdk/framework/path.go index 3f7a00932a2f..e81b54fbdf04 100644 --- a/sdk/framework/path.go +++ b/sdk/framework/path.go @@ -54,7 +54,7 @@ type Path struct { // exposed as fields that should map to a schema in Fields. If a named // capture is not a field in the Fields map, then it will be ignored. // - // The pattern will automatically have a ^ prepending and a $ appended before + // The pattern will automatically have a ^ prepended and a $ appended before // use, if these are not already present, so these may be omitted for clarity. // // If a ListOperation is being defined, the pattern must end with /? to match From 7c1dac02abac13216f4942da86de76cf7e2fc558 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Thu, 27 Apr 2023 10:33:47 +0100 Subject: [PATCH 3/5] Also fix hardcoded expectations in a new test --- api/plugin_helpers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/plugin_helpers.go b/api/plugin_helpers.go index 2d6416d70a1c..f6f64777bbdc 100644 --- a/api/plugin_helpers.go +++ b/api/plugin_helpers.go @@ -40,7 +40,7 @@ const ( // path matches that path or not (useful specifically for the paths that // contain templated fields.) var sudoPaths = map[string]*regexp.Regexp{ - "/auth/token/accessors/": regexp.MustCompile(`^/auth/token/accessors/?$`), + "/auth/token/accessors": regexp.MustCompile(`^/auth/token/accessors/?$`), "/pki/root": regexp.MustCompile(`^/pki/root$`), "/pki/root/sign-self-issued": regexp.MustCompile(`^/pki/root/sign-self-issued$`), "/sys/audit": regexp.MustCompile(`^/sys/audit$`), @@ -50,7 +50,7 @@ var sudoPaths = map[string]*regexp.Regexp{ "/sys/config/auditing/request-headers": regexp.MustCompile(`^/sys/config/auditing/request-headers$`), "/sys/config/auditing/request-headers/{header}": regexp.MustCompile(`^/sys/config/auditing/request-headers/.+$`), "/sys/config/cors": regexp.MustCompile(`^/sys/config/cors$`), - "/sys/config/ui/headers/": regexp.MustCompile(`^/sys/config/ui/headers/?$`), + "/sys/config/ui/headers": regexp.MustCompile(`^/sys/config/ui/headers/?$`), "/sys/config/ui/headers/{header}": regexp.MustCompile(`^/sys/config/ui/headers/.+$`), "/sys/leases": regexp.MustCompile(`^/sys/leases$`), "/sys/leases/lookup/": regexp.MustCompile(`^/sys/leases/lookup/?$`), From 188ceb69a520f0f0a00e9872180199a4495db19d Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Fri, 30 Jun 2023 16:19:15 +0100 Subject: [PATCH 4/5] Add a couple more testcases, and some comments. --- api/plugin_helpers.go | 4 +++- api/plugin_helpers_test.go | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/api/plugin_helpers.go b/api/plugin_helpers.go index f98cb341ab62..5bb566300536 100644 --- a/api/plugin_helpers.go +++ b/api/plugin_helpers.go @@ -249,7 +249,9 @@ func SudoPaths() map[string]*regexp.Regexp { return sudoPaths } -// Determine whether the given path requires the sudo capability +// Determine whether the given path requires the sudo capability. +// Note that this uses hardcoded static path information, so will return incorrect results for paths in namespaces, +// or for secret engines mounted at non-default paths. func IsSudoPath(path string) bool { // Return early if the path is any of the non-templated sudo paths. if _, ok := sudoPaths[path]; ok { diff --git a/api/plugin_helpers_test.go b/api/plugin_helpers_test.go index 7b3ddbf8154a..8dae885295e1 100644 --- a/api/plugin_helpers_test.go +++ b/api/plugin_helpers_test.go @@ -12,10 +12,12 @@ func TestIsSudoPath(t *testing.T) { path string expected bool }{ + // Testing: Not a real endpoint { "/not/in/sudo/paths/list", false, }, + // Testing: sys/raw/{path} { "/sys/raw/single-node-path", true, @@ -28,26 +30,43 @@ func TestIsSudoPath(t *testing.T) { "/sys/raw/WEIRD(but_still_valid!)p4Th?🗿笑", true, }, + // Testing: sys/auth/{path}/tune { "/sys/auth/path/in/middle/tune", true, }, + // Testing: sys/plugins/catalog/{type} and sys/plugins/catalog/{name} (regexs overlap) { "/sys/plugins/catalog/some-type", true, }, + // Testing: Not a real endpoint { "/sys/plugins/catalog/some/type/or/name/with/slashes", false, }, + // Testing: sys/plugins/catalog/{type}/{name} { "/sys/plugins/catalog/some-type/some-name", true, }, + // Testing: Not a real endpoint { "/sys/plugins/catalog/some-type/some/name/with/slashes", false, }, + // Testing: auth/token/accessors (an example of a sudo path that only accepts list operations) + // It is matched as sudo without the trailing slash... + { + "/auth/token/accessors", + true, + }, + // ...and also with it. + // (Although at the time of writing, the only caller of IsSudoPath always removes trailing slashes.) + { + "/auth/token/accessors/", + true, + }, } for _, tc := range testCases { From dc64d9a01c7770d97109741bb796553742804b15 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Fri, 30 Jun 2023 16:22:06 +0100 Subject: [PATCH 5/5] Tweak spelling in comment --- api/plugin_helpers_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/plugin_helpers_test.go b/api/plugin_helpers_test.go index 8dae885295e1..2e97d44cb5b9 100644 --- a/api/plugin_helpers_test.go +++ b/api/plugin_helpers_test.go @@ -35,7 +35,7 @@ func TestIsSudoPath(t *testing.T) { "/sys/auth/path/in/middle/tune", true, }, - // Testing: sys/plugins/catalog/{type} and sys/plugins/catalog/{name} (regexs overlap) + // Testing: sys/plugins/catalog/{type} and sys/plugins/catalog/{name} (regexes overlap) { "/sys/plugins/catalog/some-type", true,