From e6718ff462bf39de8f886f73f96512f7a20655db Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Mon, 10 Jul 2023 18:07:10 +0100 Subject: [PATCH 1/9] OpenAPI: Separate ListOperation from ReadOperation Historically, since Vault's ReadOperation and ListOperation both map to the HTTP GET method, their representation in the generated OpenAPI has been a bit confusing. This was partially mitigated some time ago, by making the `list` query parameter express whether it was required or optional - but only in a way useful to human readers - the human had to know, for example, that the schema of the response body would change depending on whether `list` was selected. Now that there is an effort underway to automatically generate API clients from the OpenAPI spec, we have a need to fix this more comprehensively. Fortunately, we do have a means to do so - since Vault has opinionated treatment of trailing slashes, linked to operations being list or not, we can use an added trailing slash on the URL path to separate list operations in the OpenAPI spec. This PR implements that, and then fixes an operation ID which becomes duplicated, with this change applied. See also hashicorp/vault-client-go#174, a bug which will be fixed by this work. --- sdk/framework/openapi.go | 89 ++++++++++++++++++++++++----------- vault/logical_system_paths.go | 2 +- 2 files changed, 63 insertions(+), 28 deletions(-) diff --git a/sdk/framework/openapi.go b/sdk/framework/openapi.go index 6ef8a8414df6..957bfcf3ec85 100644 --- a/sdk/framework/openapi.go +++ b/sdk/framework/openapi.go @@ -107,13 +107,12 @@ type OASLicense struct { } type OASPathItem struct { - Description string `json:"description,omitempty"` - Parameters []OASParameter `json:"parameters,omitempty"` - Sudo bool `json:"x-vault-sudo,omitempty" mapstructure:"x-vault-sudo"` - Unauthenticated bool `json:"x-vault-unauthenticated,omitempty" mapstructure:"x-vault-unauthenticated"` - CreateSupported bool `json:"x-vault-createSupported,omitempty" mapstructure:"x-vault-createSupported"` - DisplayNavigation bool `json:"x-vault-displayNavigation,omitempty" mapstructure:"x-vault-displayNavigation"` - DisplayAttrs *DisplayAttributes `json:"x-vault-displayAttrs,omitempty" mapstructure:"x-vault-displayAttrs"` + Description string `json:"description,omitempty"` + Parameters []OASParameter `json:"parameters,omitempty"` + Sudo bool `json:"x-vault-sudo,omitempty" mapstructure:"x-vault-sudo"` + Unauthenticated bool `json:"x-vault-unauthenticated,omitempty" mapstructure:"x-vault-unauthenticated"` + CreateSupported bool `json:"x-vault-createSupported,omitempty" mapstructure:"x-vault-createSupported"` + DisplayAttrs *DisplayAttributes `json:"x-vault-displayAttrs,omitempty" mapstructure:"x-vault-displayAttrs"` Get *OASOperation `json:"get,omitempty"` Post *OASOperation `json:"post,omitempty"` @@ -309,6 +308,7 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc * // Process each supported operation by building up an Operation object // with descriptions, properties and examples from the framework.Path data. + var listOperation *OASOperation for opType, opHandler := range operations { props := opHandler.Properties() if props.Unpublished || forceUnpublished { @@ -324,11 +324,6 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc * } } - // If both List and Read are defined, only process Read. - if opType == logical.ListOperation && operations[logical.ReadOperation] != nil { - continue - } - op := NewOASOperation() operationID := constructOperationID( @@ -408,9 +403,10 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc * } } - // LIST is represented as GET with a `list` query parameter. + // LIST is represented as GET with a `list` query parameter. Code later on in this function will assign + // list operations to a path with an extra trailing slash, ensuring they do not collide with read + // operations. if opType == logical.ListOperation { - // Only accepts List (due to the above skipping of ListOperations that also have ReadOperations) op.Parameters = append(op.Parameters, OASParameter{ Name: "list", Description: "Must be set to `true`", @@ -418,14 +414,6 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc * In: "query", Schema: &OASSchema{Type: "string", Enum: []interface{}{"true"}}, }) - } else if opType == logical.ReadOperation && operations[logical.ListOperation] != nil { - // Accepts both Read and List - op.Parameters = append(op.Parameters, OASParameter{ - Name: "list", - Description: "Return a list if `true`", - In: "query", - Schema: &OASSchema{Type: "string"}, - }) } // Add tags based on backend type @@ -521,18 +509,65 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc * switch opType { case logical.CreateOperation, logical.UpdateOperation: pi.Post = op - case logical.ReadOperation, logical.ListOperation: + case logical.ReadOperation: pi.Get = op case logical.DeleteOperation: pi.Delete = op + case logical.ListOperation: + listOperation = op } } - openAPIPath := "/" + path - if doc.Paths[openAPIPath] != nil { - backend.Logger().Warn("OpenAPI spec generation: multiple framework.Path instances generated the same path; last processed wins", "path", openAPIPath) + // Write the regular, non-list, OpenAPI path to the OpenAPI document, UNLESS we generated a ListOperation, and + // NO OTHER operation types. In that fairly common case (there are lots of list-only endpoints), we avoid + // writing a redundant OpenAPI path for (e.g.) "auth/token/accessors" with no operations, only to then write + // one for "auth/token/accessors/" immediately below. + // + // On the other hand, we do still write the OpenAPI path here if we generated ZERO operation types - this serves + // to provide documentation to a human that an endpoint exists, even if it has no invokable OpenAPI operations. + // Examples of this include kv-v2's ".*" endpoint (regex cannot be translated to OpenAPI parameters), and the + // auth/oci/login endpoint (implements ResolveRoleOperation only, only callable from inside Vault). + generateNonListOpenAPIPath := listOperation == nil || pi.Get != nil || pi.Post != nil || pi.Delete != nil + if generateNonListOpenAPIPath { + openAPIPath := "/" + path + if doc.Paths[openAPIPath] != nil { + backend.Logger().Warn( + "OpenAPI spec generation: multiple framework.Path instances generated the same path; "+ + "last processed wins", "path", openAPIPath) + } + doc.Paths[openAPIPath] = &pi + } + + // If there is a ListOperation, write it to a separate OpenAPI path in the document. + if listOperation != nil { + // Typically, we will append a slash here to disambiguate from the path written immediately above. + // However, if we skipped writing the path above (and so don't need to worry about disambiguating from it), + // AND the path already contains a trailing slash, we want to avoid doubling it. + if generateNonListOpenAPIPath || !strings.HasSuffix(path, "/") { + path += "/" + } + + listPathItem := OASPathItem{ + Description: pi.Description, + Parameters: pi.Parameters, + DisplayAttrs: pi.DisplayAttrs, + + // Since the path may now have an extra slash on the end, we need to recalculate the special path + // matches, as the sudo or unauthenticated status may be changed as a result! + Sudo: specialPathMatch(path, sudoPaths), + Unauthenticated: specialPathMatch(path, unauthPaths), + + Get: listOperation, + } + + openAPIPath := "/" + path + if doc.Paths[openAPIPath] != nil { + backend.Logger().Warn( + "OpenAPI spec generation: multiple framework.Path instances generated the same path; "+ + "last processed wins", "path", openAPIPath) + } + doc.Paths[openAPIPath] = &listPathItem } - doc.Paths[openAPIPath] = &pi } return nil diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index ce525d5b9134..8dd7fc784d1b 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -3658,7 +3658,7 @@ func (b *SystemBackend) policyPaths() []*framework.Path { DisplayAttrs: &framework.DisplayAttributes{ OperationPrefix: "policies", - OperationVerb: "list", + OperationSuffix: "acl-policy-list2", // this endpoint duplicates /sys/policies/acl }, Operations: map[logical.Operation]framework.OperationHandler{ From 408de4245fa1d964bd462880377d5ff384a6b0ce Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Mon, 10 Jul 2023 18:48:08 +0100 Subject: [PATCH 2/9] Set further DisplayAttrs in auth/github plugin To mask out more duplicate read/list functionality, now being separately generated to OpenAPI client libraries as a result of this change. --- builtin/credential/github/backend.go | 30 +++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/builtin/credential/github/backend.go b/builtin/credential/github/backend.go index f8bbcc403c61..20f9a8cda838 100644 --- a/builtin/credential/github/backend.go +++ b/builtin/credential/github/backend.go @@ -8,7 +8,7 @@ import ( "net/url" "github.com/google/go-github/github" - cleanhttp "github.com/hashicorp/go-cleanhttp" + "github.com/hashicorp/go-cleanhttp" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" "golang.org/x/oauth2" @@ -43,6 +43,20 @@ func Backend() *backend { OperationPrefix: operationPrefixGithub, OperationSuffix: "team-mapping", } + teamMapPaths[0].Operations = map[logical.Operation]framework.OperationHandler{ + logical.ListOperation: &framework.PathOperation{ + Callback: teamMapPaths[0].Callbacks[logical.ListOperation], + Summary: teamMapPaths[0].HelpSynopsis, + }, + logical.ReadOperation: &framework.PathOperation{ + Callback: teamMapPaths[0].Callbacks[logical.ReadOperation], + Summary: teamMapPaths[0].HelpSynopsis, + DisplayAttrs: &framework.DisplayAttributes{ + OperationSuffix: "teams2", // The ReadOperation is redundant with the ListOperation + }, + }, + } + teamMapPaths[0].Callbacks = nil b.UserMap = &framework.PolicyMap{ PathMap: framework.PathMap{ @@ -61,6 +75,20 @@ func Backend() *backend { OperationPrefix: operationPrefixGithub, OperationSuffix: "user-mapping", } + userMapPaths[0].Operations = map[logical.Operation]framework.OperationHandler{ + logical.ListOperation: &framework.PathOperation{ + Callback: userMapPaths[0].Callbacks[logical.ListOperation], + Summary: userMapPaths[0].HelpSynopsis, + }, + logical.ReadOperation: &framework.PathOperation{ + Callback: userMapPaths[0].Callbacks[logical.ReadOperation], + Summary: userMapPaths[0].HelpSynopsis, + DisplayAttrs: &framework.DisplayAttributes{ + OperationSuffix: "users2", // The ReadOperation is redundant with the ListOperation + }, + }, + } + userMapPaths[0].Callbacks = nil allPaths := append(teamMapPaths, userMapPaths...) b.Backend = &framework.Backend{ From 1185ef86ec981e43ef56d319017ac268931ccc62 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Tue, 11 Jul 2023 07:33:50 +0100 Subject: [PATCH 3/9] Apply requested changes to operation IDs I'm not totally convinced its worth the extra lines of code, but equally, I don't have strong feelings about it, so I'll just make the change. --- builtin/credential/github/backend.go | 2 ++ vault/logical_system_paths.go | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/builtin/credential/github/backend.go b/builtin/credential/github/backend.go index 20f9a8cda838..f3669bbfd4db 100644 --- a/builtin/credential/github/backend.go +++ b/builtin/credential/github/backend.go @@ -52,6 +52,7 @@ func Backend() *backend { Callback: teamMapPaths[0].Callbacks[logical.ReadOperation], Summary: teamMapPaths[0].HelpSynopsis, DisplayAttrs: &framework.DisplayAttributes{ + OperationVerb: "list", OperationSuffix: "teams2", // The ReadOperation is redundant with the ListOperation }, }, @@ -84,6 +85,7 @@ func Backend() *backend { Callback: userMapPaths[0].Callbacks[logical.ReadOperation], Summary: userMapPaths[0].HelpSynopsis, DisplayAttrs: &framework.DisplayAttributes{ + OperationVerb: "list", OperationSuffix: "users2", // The ReadOperation is redundant with the ListOperation }, }, diff --git a/vault/logical_system_paths.go b/vault/logical_system_paths.go index 8dd7fc784d1b..f90b50c0cac2 100644 --- a/vault/logical_system_paths.go +++ b/vault/logical_system_paths.go @@ -3658,7 +3658,7 @@ func (b *SystemBackend) policyPaths() []*framework.Path { DisplayAttrs: &framework.DisplayAttributes{ OperationPrefix: "policies", - OperationSuffix: "acl-policy-list2", // this endpoint duplicates /sys/policies/acl + OperationVerb: "list", }, Operations: map[logical.Operation]framework.OperationHandler{ @@ -3678,6 +3678,9 @@ func (b *SystemBackend) policyPaths() []*framework.Path { }, }}, }, + DisplayAttrs: &framework.DisplayAttributes{ + OperationSuffix: "acl-policies2", // this endpoint duplicates sys/policies/acl + }, }, logical.ListOperation: &framework.PathOperation{ Callback: b.handlePoliciesList(PolicyTypeACL), @@ -3695,6 +3698,9 @@ func (b *SystemBackend) policyPaths() []*framework.Path { }, }}, }, + DisplayAttrs: &framework.DisplayAttributes{ + OperationSuffix: "acl-policies3", // this endpoint duplicates sys/policies/acl + }, }, }, From 898bc362d6703495567e1a2512a5943184d82edd Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Tue, 11 Jul 2023 08:26:06 +0100 Subject: [PATCH 4/9] Adjust logic to prevent any possibility of generating OpenAPI paths with doubled final slashes Even in the edge case of improper use of regex patterns and operations. --- sdk/framework/openapi.go | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/sdk/framework/openapi.go b/sdk/framework/openapi.go index 957bfcf3ec85..d92b14c68ea9 100644 --- a/sdk/framework/openapi.go +++ b/sdk/framework/openapi.go @@ -518,6 +518,20 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc * } } + // The conventions enforced by the Vault HTTP routing code make it impossible to match a path with a trailing + // slash to anything other than a ListOperation. Catch mistakes in path definition, to enforce that if both of + // the two following blocks of code (non-list, and list) write an OpenAPI path to the output document, then the + // first one will definitely not have a trailing slash. + originalPathHasTrailingSlash := strings.HasSuffix(path, "/") + if originalPathHasTrailingSlash && (pi.Get != nil || pi.Post != nil || pi.Delete != nil) { + backend.Logger().Warn( + "OpenAPI spec generation: discarding impossible-to-invoke non-list operations from path with "+ + "required trailing slash; this is a bug in the backend code", "path", path) + pi.Get = nil + pi.Post = nil + pi.Delete = nil + } + // Write the regular, non-list, OpenAPI path to the OpenAPI document, UNLESS we generated a ListOperation, and // NO OTHER operation types. In that fairly common case (there are lots of list-only endpoints), we avoid // writing a redundant OpenAPI path for (e.g.) "auth/token/accessors" with no operations, only to then write @@ -527,8 +541,7 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc * // to provide documentation to a human that an endpoint exists, even if it has no invokable OpenAPI operations. // Examples of this include kv-v2's ".*" endpoint (regex cannot be translated to OpenAPI parameters), and the // auth/oci/login endpoint (implements ResolveRoleOperation only, only callable from inside Vault). - generateNonListOpenAPIPath := listOperation == nil || pi.Get != nil || pi.Post != nil || pi.Delete != nil - if generateNonListOpenAPIPath { + if listOperation == nil || pi.Get != nil || pi.Post != nil || pi.Delete != nil { openAPIPath := "/" + path if doc.Paths[openAPIPath] != nil { backend.Logger().Warn( @@ -540,10 +553,11 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc * // If there is a ListOperation, write it to a separate OpenAPI path in the document. if listOperation != nil { - // Typically, we will append a slash here to disambiguate from the path written immediately above. - // However, if we skipped writing the path above (and so don't need to worry about disambiguating from it), - // AND the path already contains a trailing slash, we want to avoid doubling it. - if generateNonListOpenAPIPath || !strings.HasSuffix(path, "/") { + // Append a slash here to disambiguate from the path written immediately above. + // However, if the path already contains a trailing slash, we want to avoid doubling it, and it is + // guaranteed (through the interaction of logic in the last two blocks) that the block immediately above + // will NOT have written a path to the OpenAPI document. + if !originalPathHasTrailingSlash { path += "/" } From aeb4289c4b9b56a351cd326bc3473fdd6fb6eb21 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Tue, 11 Jul 2023 08:35:03 +0100 Subject: [PATCH 5/9] changelog --- changelog/21723.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/21723.txt diff --git a/changelog/21723.txt b/changelog/21723.txt new file mode 100644 index 000000000000..cefe5e1c5ad3 --- /dev/null +++ b/changelog/21723.txt @@ -0,0 +1,3 @@ +```release-note:improvement +openapi: List operations are now given first-class representation in the OpenAPI document, rather than sometimes being overlaid with a read operation at the same path +``` From d037477eddbacde225a867fa64e2df091861a75f Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Tue, 11 Jul 2023 23:23:30 +0100 Subject: [PATCH 6/9] Fix TestSudoPaths to pass again... which snowballed a bit... Once I looked hard at it, I found it was missing several sudo paths, which led to additional bug fixing elsewhere. I might need to pull some parts of this change out into a separate PR for ease of review... --- api/plugin_helpers.go | 38 ++++++++++------- vault/external_tests/api/sudo_paths_test.go | 45 ++++++++++----------- vault/logical_system.go | 5 +++ vault/token_store.go | 16 ++------ 4 files changed, 53 insertions(+), 51 deletions(-) diff --git a/api/plugin_helpers.go b/api/plugin_helpers.go index 5bb566300536..2506a9f041e0 100644 --- a/api/plugin_helpers.go +++ b/api/plugin_helpers.go @@ -41,6 +41,7 @@ const ( // contain templated fields.) var sudoPaths = map[string]*regexp.Regexp{ "/auth/token/accessors": regexp.MustCompile(`^/auth/token/accessors/?$`), + "/auth/token/revoke-orphan": regexp.MustCompile(`^/auth/token/revoke-orphan$`), "/pki/root": regexp.MustCompile(`^/pki/root$`), "/pki/root/sign-self-issued": regexp.MustCompile(`^/pki/root/sign-self-issued$`), "/sys/audit": regexp.MustCompile(`^/sys/audit$`), @@ -52,28 +53,33 @@ var sudoPaths = map[string]*regexp.Regexp{ "/sys/config/cors": regexp.MustCompile(`^/sys/config/cors$`), "/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/?$`), - "/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/remount": regexp.MustCompile(`^/sys/remount$`), - "/sys/revoke-force/{prefix}": regexp.MustCompile(`^/sys/revoke-force/.+$`), - "/sys/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/revoke-prefix/.+$`), - "/sys/rotate": regexp.MustCompile(`^/sys/rotate$`), "/sys/internal/inspect/router/{tag}": regexp.MustCompile(`^/sys/internal/inspect/router/.+$`), + "/sys/leases": regexp.MustCompile(`^/sys/leases$`), + // 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/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/remount": regexp.MustCompile(`^/sys/remount$`), + "/sys/revoke-force/{prefix}": regexp.MustCompile(`^/sys/revoke-force/.+$`), + "/sys/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/revoke-prefix/.+$`), + "/sys/rotate": regexp.MustCompile(`^/sys/rotate$`), + "/sys/seal": regexp.MustCompile(`^/sys/seal$`), + "/sys/step-down": regexp.MustCompile(`^/sys/step-down$`), // enterprise-only paths "/sys/replication/dr/primary/secondary-token": regexp.MustCompile(`^/sys/replication/dr/primary/secondary-token$`), "/sys/replication/performance/primary/secondary-token": regexp.MustCompile(`^/sys/replication/performance/primary/secondary-token$`), "/sys/replication/primary/secondary-token": regexp.MustCompile(`^/sys/replication/primary/secondary-token$`), "/sys/replication/reindex": regexp.MustCompile(`^/sys/replication/reindex$`), - "/sys/storage/raft/snapshot-auto/config/": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/?$`), + "/sys/storage/raft/snapshot-auto/config": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/?$`), "/sys/storage/raft/snapshot-auto/config/{name}": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/[^/]+$`), } @@ -252,6 +258,8 @@ func SudoPaths() map[string]*regexp.Regexp { // 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. +// Expects to receive a path with an initial slash, but no trailing slashes, as the Vault CLI (the only known and +// expected user of this function) sanitizes its paths that way. 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/vault/external_tests/api/sudo_paths_test.go b/vault/external_tests/api/sudo_paths_test.go index 0ca470f1d87c..f2590992a86f 100644 --- a/vault/external_tests/api/sudo_paths_test.go +++ b/vault/external_tests/api/sudo_paths_test.go @@ -5,19 +5,13 @@ package api import ( "fmt" + "strings" "testing" log "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/api" - "github.com/hashicorp/vault/audit" - auditFile "github.com/hashicorp/vault/builtin/audit/file" - credUserpass "github.com/hashicorp/vault/builtin/credential/userpass" - "github.com/hashicorp/vault/builtin/logical/database" - "github.com/hashicorp/vault/builtin/logical/pki" - "github.com/hashicorp/vault/builtin/logical/transit" "github.com/hashicorp/vault/helper/builtinplugins" "github.com/hashicorp/vault/sdk/helper/jsonutil" - "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/vault" ) @@ -33,24 +27,13 @@ func TestSudoPaths(t *testing.T) { EnableRaw: true, EnableIntrospection: true, Logger: log.NewNullLogger(), - CredentialBackends: map[string]logical.Factory{ - "userpass": credUserpass.Factory, - }, - AuditBackends: map[string]audit.Factory{ - "file": auditFile.Factory, - }, - LogicalBackends: map[string]logical.Factory{ - "database": database.Factory, - "generic-leased": vault.LeasedPassthroughBackendFactory, - "pki": pki.Factory, - "transit": transit.Factory, - }, - BuiltinRegistry: builtinplugins.Registry, + BuiltinRegistry: builtinplugins.Registry, } client, _, closer := testVaultServerCoreConfig(t, coreConfig) defer closer() - for credBackendName := range coreConfig.CredentialBackends { + // At present there are no auth methods with sudo paths, except for the automatically mounted token backend + for _, credBackendName := range []string{} { err := client.Sys().EnableAuthWithOptions(credBackendName, &api.EnableAuthOptions{ Type: credBackendName, }) @@ -59,7 +42,8 @@ func TestSudoPaths(t *testing.T) { } } - for logicalBackendName := range coreConfig.LogicalBackends { + // Each secrets engine that contains sudo paths (other than automatically mounted ones) must be mounted here + for _, logicalBackendName := range []string{"pki"} { err := client.Sys().Mount(logicalBackendName, &api.MountInput{ Type: logicalBackendName, }) @@ -77,11 +61,24 @@ func TestSudoPaths(t *testing.T) { // check for missing paths for path := range sudoPathsFromSpec { - if _, ok := sudoPathsInCode[path]; !ok { + pathTrimmed := strings.TrimRight(path, "/") + if _, ok := sudoPathsInCode[pathTrimmed]; !ok { t.Fatalf( "A path in the OpenAPI spec is missing from the static list of "+ "sudo paths in the api module (%s). Please reconcile the two "+ - "accordingly.", path) + "accordingly.", pathTrimmed) + } + } + + // check for extra paths + for path := range sudoPathsInCode { + if _, ok := sudoPathsFromSpec[path]; !ok { + if _, ok := sudoPathsFromSpec[path+"/"]; !ok { + t.Fatalf( + "A path in the static list of sudo paths in the api module "+ + "is not marked as a sudo path in the OpenAPI spec (%s). Please reconcile the two "+ + "accordingly.", path) + } } } } diff --git a/vault/logical_system.go b/vault/logical_system.go index a332276b9381..79124863d327 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -120,6 +120,11 @@ func NewSystemBackend(core *Core, logger log.Logger) *SystemBackend { "storage/raft/snapshot-auto/config/*", "leases", "internal/inspect/*", + // sys/seal and sys/step-down actually have their sudo requirement enforced through hardcoding + // PolicyCheckOpts.RootPrivsRequired in dedicated calls to Core.performPolicyChecks, but we still need + // to declare them here so that the generated OpenAPI spec gets their sudo status correct. + "seal", + "step-down", }, Unauthenticated: []string{ diff --git a/vault/token_store.go b/vault/token_store.go index d25aa629df2d..3631b3e04ec8 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -783,8 +783,8 @@ func NewTokenStore(ctx context.Context, logger log.Logger, core *Core, config *l PathsSpecial: &logical.Paths{ Root: []string{ - "revoke-orphan/*", - "accessors*", + "revoke-orphan", + "accessors/", }, // Most token store items are local since tokens are local, but a @@ -3285,8 +3285,8 @@ func (ts *TokenStore) revokeCommon(ctx context.Context, req *logical.Request, da return nil, nil } -// handleRevokeOrphan handles the auth/token/revoke-orphan/id path for revocation of tokens -// in a way that leaves child tokens orphaned. Normally, using sys/revoke/leaseID will revoke +// handleRevokeOrphan handles the auth/token/revoke-orphan path for revocation of tokens +// in a way that leaves child tokens orphaned. Normally, using sys/leases/revoke/{lease_id} will revoke // the token and all children. func (ts *TokenStore) handleRevokeOrphan(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { // Parse the id @@ -3295,14 +3295,6 @@ func (ts *TokenStore) handleRevokeOrphan(ctx context.Context, req *logical.Reque return logical.ErrorResponse("missing token ID"), logical.ErrInvalidRequest } - // Check if the client token has sudo/root privileges for the requested path - isSudo := ts.System().(extendedSystemView).SudoPrivilege(ctx, req.MountPoint+req.Path, req.ClientToken) - - if !isSudo { - return logical.ErrorResponse("root or sudo privileges required to revoke and orphan"), - logical.ErrInvalidRequest - } - // Do a lookup. Among other things, that will ensure that this is either // running in the same namespace or a parent. te, err := ts.Lookup(ctx, id) From 01cb30bcff07f1480ee8dbfd97750fbccfe43cab Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Tue, 11 Jul 2023 23:48:43 +0100 Subject: [PATCH 7/9] Fix other tests --- builtin/logical/pki/backend_test.go | 15 ++--- builtin/logical/ssh/backend_test.go | 5 +- sdk/framework/testdata/operations.json | 65 +++++++++++++++++---- sdk/framework/testdata/operations_list.json | 2 +- vault/logical_system_test.go | 2 +- 5 files changed, 67 insertions(+), 22 deletions(-) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 308d661d6698..4a5132b553c6 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -6767,8 +6767,8 @@ func TestProperAuthing(t *testing.T) { "cert/unified-delta-crl": shouldBeUnauthedReadList, "cert/unified-delta-crl/raw": shouldBeUnauthedReadList, "cert/unified-delta-crl/raw/pem": shouldBeUnauthedReadList, - "certs": shouldBeAuthed, - "certs/revoked": shouldBeAuthed, + "certs/": shouldBeAuthed, + "certs/revoked/": shouldBeAuthed, "certs/revocation-queue": shouldBeAuthed, "certs/unified-revoked": shouldBeAuthed, "config/acme": shouldBeAuthed, @@ -6817,7 +6817,7 @@ func TestProperAuthing(t *testing.T) { "issuer/default/sign-verbatim": shouldBeAuthed, "issuer/default/sign-verbatim/test": shouldBeAuthed, "issuer/default/sign/test": shouldBeAuthed, - "issuers": shouldBeUnauthedReadList, + "issuers/": shouldBeUnauthedReadList, "issuers/generate/intermediate/exported": shouldBeAuthed, "issuers/generate/intermediate/internal": shouldBeAuthed, "issuers/generate/intermediate/existing": shouldBeAuthed, @@ -6829,7 +6829,7 @@ func TestProperAuthing(t *testing.T) { "issuers/import/cert": shouldBeAuthed, "issuers/import/bundle": shouldBeAuthed, "key/default": shouldBeAuthed, - "keys": shouldBeAuthed, + "keys/": shouldBeAuthed, "keys/generate/internal": shouldBeAuthed, "keys/generate/exported": shouldBeAuthed, "keys/generate/kms": shouldBeAuthed, @@ -6839,7 +6839,7 @@ func TestProperAuthing(t *testing.T) { "revoke": shouldBeAuthed, "revoke-with-key": shouldBeAuthed, "roles/test": shouldBeAuthed, - "roles": shouldBeAuthed, + "roles/": shouldBeAuthed, "root": shouldBeAuthed, "root/generate/exported": shouldBeAuthed, "root/generate/internal": shouldBeAuthed, @@ -6864,7 +6864,7 @@ func TestProperAuthing(t *testing.T) { "unified-crl/delta/pem": shouldBeUnauthedReadList, "unified-ocsp": shouldBeUnauthedWriteOnly, "unified-ocsp/dGVzdAo=": shouldBeUnauthedReadList, - "eab": shouldBeAuthed, + "eab/": shouldBeAuthed, "eab/" + eabKid: shouldBeAuthed, } @@ -6953,7 +6953,8 @@ func TestProperAuthing(t *testing.T) { handler, present := paths[raw_path] if !present { - t.Fatalf("OpenAPI reports PKI mount contains %v->%v but was not tested to be authed or authed.", openapi_path, raw_path) + t.Fatalf("OpenAPI reports PKI mount contains %v -> %v but was not tested to be authed or not authed.", + openapi_path, raw_path) } openapi_data := raw_data.(map[string]interface{}) diff --git a/builtin/logical/ssh/backend_test.go b/builtin/logical/ssh/backend_test.go index 13f9f73624af..a33c6224585c 100644 --- a/builtin/logical/ssh/backend_test.go +++ b/builtin/logical/ssh/backend_test.go @@ -2765,7 +2765,7 @@ func TestProperAuthing(t *testing.T) { "public_key": shouldBeUnauthedReadList, "roles/test-ca": shouldBeAuthed, "roles/test-otp": shouldBeAuthed, - "roles": shouldBeAuthed, + "roles/": shouldBeAuthed, "sign/test-ca": shouldBeAuthed, "tidy/dynamic-keys": shouldBeAuthed, "verify": shouldBeUnauthedWriteOnly, @@ -2809,7 +2809,8 @@ func TestProperAuthing(t *testing.T) { handler, present := paths[raw_path] if !present { - t.Fatalf("OpenAPI reports SSH mount contains %v->%v but was not tested to be authed or authed.", openapi_path, raw_path) + t.Fatalf("OpenAPI reports SSH mount contains %v -> %v but was not tested to be authed or not authed.", + openapi_path, raw_path) } openapi_data := raw_data.(map[string]interface{}) diff --git a/sdk/framework/testdata/operations.json b/sdk/framework/testdata/operations.json index 7fca0e265014..91bd64d27059 100644 --- a/sdk/framework/testdata/operations.json +++ b/sdk/framework/testdata/operations.json @@ -47,17 +47,7 @@ "200": { "description": "OK" } - }, - "parameters": [ - { - "name": "list", - "description": "Return a list if `true`", - "in": "query", - "schema": { - "type": "string" - } - } - ] + } }, "post": { "operationId": "kv-write-foo-id", @@ -82,6 +72,59 @@ } } } + }, + "/foo/{id}/": { + "description": "Synopsis", + "x-vault-sudo": true, + "x-vault-displayAttrs": { + "navigation": true + }, + "parameters": [ + { + "name": "format", + "description": "a query param", + "in": "query", + "schema": { + "type": "string" + } + }, + { + "name": "id", + "description": "id path parameter", + "in": "path", + "schema": { + "type": "string" + }, + "required": true + } + ], + "get": { + "operationId": "kv-list-foo-id", + "tags": [ + "secrets" + ], + "summary": "List Summary", + "description": "List Description", + "responses": { + "200": { + "description": "OK" + } + }, + "parameters": [ + { + "name": "list", + "description": "Must be set to `true`", + "required": true, + "in": "query", + "schema": { + "type": "string", + "enum": [ + "true" + ] + } + } + ] + } } }, "components": { diff --git a/sdk/framework/testdata/operations_list.json b/sdk/framework/testdata/operations_list.json index a08208b24fa4..d7bc50187c78 100644 --- a/sdk/framework/testdata/operations_list.json +++ b/sdk/framework/testdata/operations_list.json @@ -10,7 +10,7 @@ } }, "paths": { - "/foo/{id}": { + "/foo/{id}/": { "description": "Synopsis", "x-vault-sudo": true, "x-vault-displayAttrs": { diff --git a/vault/logical_system_test.go b/vault/logical_system_test.go index 5ee5b3fdf221..cea1c8afbc6c 100644 --- a/vault/logical_system_test.go +++ b/vault/logical_system_test.go @@ -4103,7 +4103,7 @@ func TestSystemBackend_OpenAPI(t *testing.T) { }{ {path: "/auth/token/lookup", tag: "auth"}, {path: "/cubbyhole/{path}", tag: "secrets"}, - {path: "/identity/group/id", tag: "identity"}, + {path: "/identity/group/id/", tag: "identity"}, {path: expectedSecretPrefix + "^.*$", unpublished: true}, {path: "/sys/policy", tag: "system"}, } From 1d4680a4a5c5632f953b482b7abfb09b434d689a Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Wed, 12 Jul 2023 00:17:46 +0100 Subject: [PATCH 8/9] More test fixing --- vault/logical_system_test.go | 2 ++ vault/token_store_test.go | 7 ++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/vault/logical_system_test.go b/vault/logical_system_test.go index cea1c8afbc6c..1ba6f8122db5 100644 --- a/vault/logical_system_test.go +++ b/vault/logical_system_test.go @@ -68,6 +68,8 @@ func TestSystemBackend_RootPaths(t *testing.T) { "storage/raft/snapshot-auto/config/*", "leases", "internal/inspect/*", + "seal", + "step-down", } b := testSystemBackend(t) diff --git a/vault/token_store_test.go b/vault/token_store_test.go index 5495cdae3c7e..943bc5bddfa4 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -6,6 +6,7 @@ package vault import ( "context" "encoding/json" + "errors" "fmt" "path" "reflect" @@ -2389,9 +2390,9 @@ func TestTokenStore_HandleRequest_RevokeOrphan_NonRoot(t *testing.T) { "token": "child", } req.ClientToken = "child" - resp, err := ts.HandleRequest(namespace.RootContext(nil), req) - if err != logical.ErrInvalidRequest { - t.Fatalf("did not get error when non-root revoking itself with orphan flag; resp is %#v", resp) + resp, err := c.HandleRequest(namespace.RootContext(nil), req) + if !errors.Is(err, logical.ErrPermissionDenied) { + t.Fatalf("did not get expected error when non-root revoking itself with orphan flag; resp is %#v; err is %#v", resp, err) } time.Sleep(200 * time.Millisecond) From 51f7a038714c38f857744afe8ac3c4eff736f1f0 Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Wed, 12 Jul 2023 00:31:13 +0100 Subject: [PATCH 9/9] Undo scope creep - back away from fixing sudo paths not shown as such in OpenAPI, at least within this PR Just add TODO comments for now. --- api/plugin_helpers.go | 8 ++++---- vault/logical_system.go | 5 ----- vault/logical_system_test.go | 2 -- vault/token_store.go | 16 ++++++++++++---- vault/token_store_test.go | 7 +++---- 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/api/plugin_helpers.go b/api/plugin_helpers.go index 2506a9f041e0..b96cb9becf78 100644 --- a/api/plugin_helpers.go +++ b/api/plugin_helpers.go @@ -40,8 +40,8 @@ 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/revoke-orphan": regexp.MustCompile(`^/auth/token/revoke-orphan$`), + "/auth/token/accessors": regexp.MustCompile(`^/auth/token/accessors/?$`), + // TODO /auth/token/revoke-orphan requires sudo but isn't represented as such in the OpenAPI spec "/pki/root": regexp.MustCompile(`^/pki/root$`), "/pki/root/sign-self-issued": regexp.MustCompile(`^/pki/root/sign-self-issued$`), "/sys/audit": regexp.MustCompile(`^/sys/audit$`), @@ -71,8 +71,8 @@ var sudoPaths = map[string]*regexp.Regexp{ "/sys/revoke-force/{prefix}": regexp.MustCompile(`^/sys/revoke-force/.+$`), "/sys/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/revoke-prefix/.+$`), "/sys/rotate": regexp.MustCompile(`^/sys/rotate$`), - "/sys/seal": regexp.MustCompile(`^/sys/seal$`), - "/sys/step-down": regexp.MustCompile(`^/sys/step-down$`), + // TODO /sys/seal requires sudo but isn't represented as such in the OpenAPI spec + // TODO /sys/step-down requires sudo but isn't represented as such in the OpenAPI spec // enterprise-only paths "/sys/replication/dr/primary/secondary-token": regexp.MustCompile(`^/sys/replication/dr/primary/secondary-token$`), diff --git a/vault/logical_system.go b/vault/logical_system.go index 79124863d327..a332276b9381 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -120,11 +120,6 @@ func NewSystemBackend(core *Core, logger log.Logger) *SystemBackend { "storage/raft/snapshot-auto/config/*", "leases", "internal/inspect/*", - // sys/seal and sys/step-down actually have their sudo requirement enforced through hardcoding - // PolicyCheckOpts.RootPrivsRequired in dedicated calls to Core.performPolicyChecks, but we still need - // to declare them here so that the generated OpenAPI spec gets their sudo status correct. - "seal", - "step-down", }, Unauthenticated: []string{ diff --git a/vault/logical_system_test.go b/vault/logical_system_test.go index 1ba6f8122db5..cea1c8afbc6c 100644 --- a/vault/logical_system_test.go +++ b/vault/logical_system_test.go @@ -68,8 +68,6 @@ func TestSystemBackend_RootPaths(t *testing.T) { "storage/raft/snapshot-auto/config/*", "leases", "internal/inspect/*", - "seal", - "step-down", } b := testSystemBackend(t) diff --git a/vault/token_store.go b/vault/token_store.go index 3631b3e04ec8..d25aa629df2d 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -783,8 +783,8 @@ func NewTokenStore(ctx context.Context, logger log.Logger, core *Core, config *l PathsSpecial: &logical.Paths{ Root: []string{ - "revoke-orphan", - "accessors/", + "revoke-orphan/*", + "accessors*", }, // Most token store items are local since tokens are local, but a @@ -3285,8 +3285,8 @@ func (ts *TokenStore) revokeCommon(ctx context.Context, req *logical.Request, da return nil, nil } -// handleRevokeOrphan handles the auth/token/revoke-orphan path for revocation of tokens -// in a way that leaves child tokens orphaned. Normally, using sys/leases/revoke/{lease_id} will revoke +// handleRevokeOrphan handles the auth/token/revoke-orphan/id path for revocation of tokens +// in a way that leaves child tokens orphaned. Normally, using sys/revoke/leaseID will revoke // the token and all children. func (ts *TokenStore) handleRevokeOrphan(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { // Parse the id @@ -3295,6 +3295,14 @@ func (ts *TokenStore) handleRevokeOrphan(ctx context.Context, req *logical.Reque return logical.ErrorResponse("missing token ID"), logical.ErrInvalidRequest } + // Check if the client token has sudo/root privileges for the requested path + isSudo := ts.System().(extendedSystemView).SudoPrivilege(ctx, req.MountPoint+req.Path, req.ClientToken) + + if !isSudo { + return logical.ErrorResponse("root or sudo privileges required to revoke and orphan"), + logical.ErrInvalidRequest + } + // Do a lookup. Among other things, that will ensure that this is either // running in the same namespace or a parent. te, err := ts.Lookup(ctx, id) diff --git a/vault/token_store_test.go b/vault/token_store_test.go index 943bc5bddfa4..5495cdae3c7e 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -6,7 +6,6 @@ package vault import ( "context" "encoding/json" - "errors" "fmt" "path" "reflect" @@ -2390,9 +2389,9 @@ func TestTokenStore_HandleRequest_RevokeOrphan_NonRoot(t *testing.T) { "token": "child", } req.ClientToken = "child" - resp, err := c.HandleRequest(namespace.RootContext(nil), req) - if !errors.Is(err, logical.ErrPermissionDenied) { - t.Fatalf("did not get expected error when non-root revoking itself with orphan flag; resp is %#v; err is %#v", resp, err) + resp, err := ts.HandleRequest(namespace.RootContext(nil), req) + if err != logical.ErrInvalidRequest { + t.Fatalf("did not get error when non-root revoking itself with orphan flag; resp is %#v", resp) } time.Sleep(200 * time.Millisecond)