From 25540ad222d42f83db2e5da0108a80f80a62dcb3 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Fri, 21 Jul 2023 18:50:07 +0100 Subject: [PATCH] Fix regexes for `sys/raw/` and `sys/leases/lookup/` to match prevailing conventions (#21760) * Fix regexes for `sys/raw/` and `sys/leases/lookup/` to match prevailing conventions There are several endpoints in Vault which take an arbitrary path as the last parameter. Many of these are defined in terms of the `framework.MatchAllRegex` helper. Some were not, and were defined using custom regexes which gave rise to multiple OpenAPI endpoints - one with the path parameter, and one without. We need to fix these definitions, because they give rise to a very unnatural result when used to generate a client API - for example, you end up with `LeasesLookUp()` which is only capable of being used to list the very top level of the hierarchical collection of leases, and `LeasesLookUpWithPrefix(prefix)` which must be used for all deeper levels. This PR changes the regexes used for `sys/raw/` and `sys/leases/lookup/` to be consistent with the approach used for other well-known similar endpoints, such as `cubbyhole/`, `kv-v1/` and `kv-v2/metadata/`. This PR does have a very small compatibility issue, which I think is tolerable: prior to this change, `sys/raw` with no trailing slash was considered a valid endpoint, and now it will no longer be. One way to observe this is to try `vault path-help sys/raw` - before this change, it would work, after, it will not. You would have to instead use `vault path-help sys/raw/foobar` to see the help. I also considered whether losing the ability to read/write/delete `sys/raw` would be an issue. In each case, the precise HTTP result code will change, but each of these were meaningless operations that make no sense - you cannot read/write/delete a "file" at the "root directory" of the underlying Vault storage. In fact, during testing, I discovered that currently, `vault write sys/raw x=y` when using Raft storage, will permanently break the Vault instance - it causes a panic within the Raft FSM, which re-occurs immediately on restarting the server! This PR also closes off that footgun / DoS vector. None of these issues apply to `sys/leases/lookup/`, as the existing regex in that case was already not matching the path without the trailing slash. * changelog * Realign hardcoded sudo paths with updated OpenAPI spec --- api/sudo_paths.go | 6 ++---- changelog/21760.txt | 3 +++ vault/logical_raw.go | 11 +---------- vault/logical_system_paths.go | 3 +-- 4 files changed, 7 insertions(+), 16 deletions(-) create mode 100644 changelog/21760.txt diff --git a/api/sudo_paths.go b/api/sudo_paths.go index 493e68c284..fb7113a0f3 100644 --- a/api/sudo_paths.go +++ b/api/sudo_paths.go @@ -32,15 +32,13 @@ var sudoPaths = map[string]*regexp.Regexp{ // This entry is a bit wrong... sys/leases/lookup does NOT require sudo. But sys/leases/lookup/ with a trailing // slash DOES require sudo. But the part of the Vault CLI that uses this logic doesn't pass operation-appropriate // trailing slashes, it always strips them off, so we end up giving the wrong answer for one of these. - "/sys/leases/lookup": regexp.MustCompile(`^/sys/leases/lookup/?$`), - "/sys/leases/lookup/{prefix}": regexp.MustCompile(`^/sys/leases/lookup/.+$`), + "/sys/leases/lookup/{prefix}": regexp.MustCompile(`^/sys/leases/lookup(?:/.+)?$`), "/sys/leases/revoke-force/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-force/.+$`), "/sys/leases/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-prefix/.+$`), "/sys/plugins/catalog/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[^/]+$`), "/sys/plugins/catalog/{type}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+$`), "/sys/plugins/catalog/{type}/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+/[^/]+$`), - "/sys/raw": regexp.MustCompile(`^/sys/raw$`), - "/sys/raw/{path}": regexp.MustCompile(`^/sys/raw/.+$`), + "/sys/raw/{path}": regexp.MustCompile(`^/sys/raw(?:/.+)?$`), "/sys/remount": regexp.MustCompile(`^/sys/remount$`), "/sys/revoke-force/{prefix}": regexp.MustCompile(`^/sys/revoke-force/.+$`), "/sys/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/revoke-prefix/.+$`), diff --git a/changelog/21760.txt b/changelog/21760.txt new file mode 100644 index 0000000000..2285cda446 --- /dev/null +++ b/changelog/21760.txt @@ -0,0 +1,3 @@ +```release-note:improvement +core: Fix regexes for `sys/raw/` and `sys/leases/lookup/` to match prevailing conventions +``` diff --git a/vault/logical_raw.go b/vault/logical_raw.go index ba4822d95b..ba221327a8 100644 --- a/vault/logical_raw.go +++ b/vault/logical_raw.go @@ -296,7 +296,7 @@ func (b *RawBackend) existenceCheck(ctx context.Context, request *logical.Reques func rawPaths(prefix string, r *RawBackend) []*framework.Path { return []*framework.Path{ { - Pattern: prefix + "(raw/?$|raw/(?P.+))", + Pattern: prefix + "raw/" + framework.MatchAllRegex("path"), Fields: map[string]*framework.FieldSchema{ "path": { @@ -322,7 +322,6 @@ func rawPaths(prefix string, r *RawBackend) []*framework.Path { DisplayAttrs: &framework.DisplayAttributes{ OperationPrefix: "raw", OperationVerb: "read", - OperationSuffix: "|path", }, Responses: map[int][]framework.Response{ http.StatusOK: {{ @@ -342,7 +341,6 @@ func rawPaths(prefix string, r *RawBackend) []*framework.Path { DisplayAttrs: &framework.DisplayAttributes{ OperationPrefix: "raw", OperationVerb: "write", - OperationSuffix: "|path", }, Responses: map[int][]framework.Response{ http.StatusOK: {{ @@ -353,11 +351,6 @@ func rawPaths(prefix string, r *RawBackend) []*framework.Path { }, logical.CreateOperation: &framework.PathOperation{ Callback: r.handleRawWrite, - DisplayAttrs: &framework.DisplayAttributes{ - OperationPrefix: "raw", - OperationVerb: "write", - OperationSuffix: "|path", - }, Responses: map[int][]framework.Response{ http.StatusNoContent: {{ Description: "OK", @@ -370,7 +363,6 @@ func rawPaths(prefix string, r *RawBackend) []*framework.Path { DisplayAttrs: &framework.DisplayAttributes{ OperationPrefix: "raw", OperationVerb: "delete", - OperationSuffix: "|path", }, Responses: map[int][]framework.Response{ http.StatusNoContent: {{ @@ -384,7 +376,6 @@ func rawPaths(prefix string, r *RawBackend) []*framework.Path { DisplayAttrs: &framework.DisplayAttributes{ OperationPrefix: "raw", OperationVerb: "list", - OperationSuffix: "|path", }, Responses: map[int][]framework.Response{ http.StatusOK: {{ diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index f90b50c0ca..ccb4d20819 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -2696,12 +2696,11 @@ func (b *SystemBackend) capabilitiesPaths() []*framework.Path { func (b *SystemBackend) leasePaths() []*framework.Path { return []*framework.Path{ { - Pattern: "leases/lookup/(?P.+?)?", + Pattern: "leases/lookup/" + framework.MatchAllRegex("prefix"), DisplayAttrs: &framework.DisplayAttributes{ OperationPrefix: "leases", OperationVerb: "look-up", - OperationSuffix: "|with-prefix", }, Fields: map[string]*framework.FieldSchema{