From 4c5f583f39ebaf2a8e9808d7477753e21356cd9b Mon Sep 17 00:00:00 2001 From: Max Bowsher Date: Wed, 18 Jan 2023 04:07:11 +0000 Subject: [PATCH] OpenAPI `generic_mount_paths` follow-up (#18663) * OpenAPI `generic_mount_paths` follow-up An incremental improvement within larger context discussed in #18560. * Following the revert in #18617, re-introduce the change from `{mountPath}` to `{_mount_path}`; this is needed, as otherwise paths from multiple plugins would clash - e.g. almost every auth method would provide a conflicting definition for `auth/{mountPath}/login`, and the last one written into the map would win. * Move the half of the functionality that was in `sdk/framework/` to `vault/logical_system.go` with the rest; this is needed, as `sdk/framework/` gets compiled in to externally built plugins, and therefore there may be version skew between it and the Vault main code. Implementing the `generic_mount_paths` feature entirely on one side of this boundary frees us from problems caused by this. * Update the special exception that recognizes `system` and `identity` as singleton mounts to also include the other two singleton mounts, `cubbyhole` and `auth/token`. * Include a comment that documents to restricted circumstances in which the `generic_mount_paths` option makes sense to use: // Note that for this to actually be useful, you have to be using it with // a Vault instance in which you have mounted one of each secrets engine // and auth method of types you are interested in, at paths which identify // their type, and for the KV secrets engine you will probably want to // mount separate kv-v1 and kv-v2 mounts to include the documentation for // each of those APIs. * Fix tests Also remove comment "// TODO update after kv repo update" which was added 4 years ago in #5687 - the implied update has not happened. * Add changelog * Update 18663.txt --- changelog/18663.txt | 3 +++ sdk/framework/backend.go | 9 +------ sdk/framework/openapi.go | 21 +++------------ sdk/framework/openapi_test.go | 10 +++---- sdk/framework/path.go | 2 +- vault/logical_system.go | 50 +++++++++++++++++++++++++++++------ vault/logical_system_test.go | 8 +++--- 7 files changed, 59 insertions(+), 44 deletions(-) create mode 100644 changelog/18663.txt diff --git a/changelog/18663.txt b/changelog/18663.txt new file mode 100644 index 000000000..941b2715e --- /dev/null +++ b/changelog/18663.txt @@ -0,0 +1,3 @@ +```release-note:improvement +openapi: generic_mount_paths: Move implementation fully into server, rather than partially in plugin framework; recognize all 4 singleton mounts (auth/token, cubbyhole, identity, system) rather than just 2; change parameter from `{mountPath}` to `{_mount_path}` +``` diff --git a/sdk/framework/backend.go b/sdk/framework/backend.go index 33c965e26..489509721 100644 --- a/sdk/framework/backend.go +++ b/sdk/framework/backend.go @@ -539,13 +539,6 @@ func (b *Backend) handleRootHelp(req *logical.Request) (*logical.Response, error // names in the OAS document. requestResponsePrefix := req.GetString("requestResponsePrefix") - // Generic mount paths will primarily be used for code generation purposes. - // This will result in dynamic mount paths being placed instead of - // hardcoded default paths. For example /auth/approle/login would be replaced - // with /auth/{mountPath}/login. This will be replaced for all secrets - // engines and auth methods that are enabled. - genericMountPaths, _ := req.Get("genericMountPaths").(bool) - // Build OpenAPI response for the entire backend vaultVersion := "unknown" if b.System() != nil { @@ -557,7 +550,7 @@ func (b *Backend) handleRootHelp(req *logical.Request) (*logical.Response, error } doc := NewOASDocument(vaultVersion) - if err := documentPaths(b, requestResponsePrefix, genericMountPaths, doc); err != nil { + if err := documentPaths(b, requestResponsePrefix, doc); err != nil { b.Logger().Warn("error generating OpenAPI", "error", err) } diff --git a/sdk/framework/openapi.go b/sdk/framework/openapi.go index 8ea820631..050571830 100644 --- a/sdk/framework/openapi.go +++ b/sdk/framework/openapi.go @@ -215,9 +215,9 @@ var ( ) // documentPaths parses all paths in a framework.Backend into OpenAPI paths. -func documentPaths(backend *Backend, requestResponsePrefix string, genericMountPaths bool, doc *OASDocument) error { +func documentPaths(backend *Backend, requestResponsePrefix string, doc *OASDocument) error { for _, p := range backend.Paths { - if err := documentPath(p, backend.SpecialPaths(), requestResponsePrefix, genericMountPaths, backend.BackendType, doc); err != nil { + if err := documentPath(p, backend.SpecialPaths(), requestResponsePrefix, backend.BackendType, doc); err != nil { return err } } @@ -226,7 +226,7 @@ func documentPaths(backend *Backend, requestResponsePrefix string, genericMountP } // documentPath parses a framework.Path into one or more OpenAPI paths. -func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix string, genericMountPaths bool, backendType logical.BackendType, doc *OASDocument) error { +func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix string, backendType logical.BackendType, doc *OASDocument) error { var sudoPaths []string var unauthPaths []string @@ -265,21 +265,6 @@ func documentPath(p *Path, specialPaths *logical.Paths, requestResponsePrefix st // Body fields will be added to individual operations. pathFields, bodyFields := splitFields(p.Fields, path) - if genericMountPaths && requestResponsePrefix != "system" && requestResponsePrefix != "identity" { - // Add mount path as a parameter - p := OASParameter{ - Name: "mountPath", - Description: "Path that the backend was mounted at", - In: "path", - Schema: &OASSchema{ - Type: "string", - }, - Required: true, - } - - pi.Parameters = append(pi.Parameters, p) - } - for name, field := range pathFields { location := "path" required := true diff --git a/sdk/framework/openapi_test.go b/sdk/framework/openapi_test.go index 47b445271..f37c6ecb9 100644 --- a/sdk/framework/openapi_test.go +++ b/sdk/framework/openapi_test.go @@ -270,7 +270,7 @@ func TestOpenAPI_SpecialPaths(t *testing.T) { Root: test.rootPaths, Unauthenticated: test.unauthPaths, } - err := documentPath(&path, sp, "kv", false, logical.TypeLogical, doc) + err := documentPath(&path, sp, "kv", logical.TypeLogical, doc) if err != nil { t.Fatal(err) } @@ -528,11 +528,11 @@ func TestOpenAPI_OperationID(t *testing.T) { for _, context := range []string{"", "bar"} { doc := NewOASDocument("version") - err := documentPath(path1, nil, "kv", false, logical.TypeLogical, doc) + err := documentPath(path1, nil, "kv", logical.TypeLogical, doc) if err != nil { t.Fatal(err) } - err = documentPath(path2, nil, "kv", false, logical.TypeLogical, doc) + err = documentPath(path2, nil, "kv", logical.TypeLogical, doc) if err != nil { t.Fatal(err) } @@ -592,7 +592,7 @@ func TestOpenAPI_CustomDecoder(t *testing.T) { } docOrig := NewOASDocument("version") - err := documentPath(p, nil, "kv", false, logical.TypeLogical, docOrig) + err := documentPath(p, nil, "kv", logical.TypeLogical, docOrig) if err != nil { t.Fatal(err) } @@ -655,7 +655,7 @@ func testPath(t *testing.T, path *Path, sp *logical.Paths, expectedJSON string) t.Helper() doc := NewOASDocument("dummyversion") - if err := documentPath(path, sp, "kv", false, logical.TypeLogical, doc); err != nil { + if err := documentPath(path, sp, "kv", logical.TypeLogical, doc); err != nil { t.Fatal(err) } doc.CreateOperationIDs("") diff --git a/sdk/framework/path.go b/sdk/framework/path.go index 963bba33e..e6221af92 100644 --- a/sdk/framework/path.go +++ b/sdk/framework/path.go @@ -342,7 +342,7 @@ func (p *Path) helpCallback(b *Backend) OperationFunc { } } doc := NewOASDocument(vaultVersion) - if err := documentPath(p, b.SpecialPaths(), requestResponsePrefix, false, b.BackendType, doc); err != nil { + if err := documentPath(p, b.SpecialPaths(), requestResponsePrefix, b.BackendType, doc); err != nil { b.Logger().Warn("error generating OpenAPI", "error", err) } diff --git a/vault/logical_system.go b/vault/logical_system.go index 98767cae7..057a1f1b5 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -4496,10 +4496,20 @@ func (b *SystemBackend) pathInternalOpenAPI(ctx context.Context, req *logical.Re context := d.Get("context").(string) - // Set up target document and convert to map[string]interface{} which is what will - // be received from plugin backends. + // Set up target document doc := framework.NewOASDocument(version.Version) + // Generic mount paths will primarily be used for code generation purposes. + // This will result in parameterized mount paths being returned instead of + // hardcoded actual paths. For example /auth/my-auth-method/login would be + // replaced with /auth/{my-auth-method_mount_path}/login. + // + // Note that for this to actually be useful, you have to be using it with + // a Vault instance in which you have mounted one of each secrets engine + // and auth method of types you are interested in, at paths which identify + // their type, and for the KV secrets engine you will probably want to + // mount separate kv-v1 and kv-v2 mounts to include the documentation for + // each of those APIs. genericMountPaths, _ := d.Get("generic_mount_paths").(bool) procMountGroup := func(group, mountPrefix string) error { @@ -4519,7 +4529,7 @@ func (b *SystemBackend) pathInternalOpenAPI(ctx context.Context, req *logical.Re req := &logical.Request{ Operation: logical.HelpOperation, Storage: req.Storage, - Data: map[string]interface{}{"requestResponsePrefix": pluginType, "genericMountPaths": genericMountPaths}, + Data: map[string]interface{}{"requestResponsePrefix": pluginType}, } resp, err := backend.HandleRequest(ctx, req) @@ -4557,6 +4567,19 @@ func (b *SystemBackend) pathInternalOpenAPI(ctx context.Context, req *logical.Re tag = "identity" } + // When set to the empty string, mountPathParameterName means not to use a parameter at all; + // the one variable combines both boolean, and value-to-use-if-true semantics. + mountPathParameterName := "" + if genericMountPaths { + isSingletonMount := (group == "auth" && pluginType == "token") || + (group == "secret" && + (pluginType == "system" || pluginType == "identity" || pluginType == "cubbyhole")) + + if !isSingletonMount { + mountPathParameterName = strings.TrimRight(mount, "/") + "_mount_path" + } + } + // Merge backend paths with existing document for path, obj := range backendDoc.Paths { path := strings.TrimPrefix(path, "/") @@ -4573,12 +4596,23 @@ func (b *SystemBackend) pathInternalOpenAPI(ctx context.Context, req *logical.Re } } - if genericMountPaths && mount != "sys/" && mount != "identity/" { - s := fmt.Sprintf("/%s{mountPath}/%s", mountPrefix, path) - doc.Paths[s] = obj - } else { - doc.Paths["/"+mountPrefix+mount+path] = obj + mountForOpenAPI := mount + + if mountPathParameterName != "" { + mountForOpenAPI = "{" + mountPathParameterName + "}/" + + obj.Parameters = append(obj.Parameters, framework.OASParameter{ + Name: mountPathParameterName, + Description: "Path that the backend was mounted at", + In: "path", + Schema: &framework.OASSchema{ + Type: "string", + }, + Required: true, + }) } + + doc.Paths["/"+mountPrefix+mountForOpenAPI+path] = obj } // Merge backend schema components diff --git a/vault/logical_system_test.go b/vault/logical_system_test.go index 75a6b67f9..a1c4981da 100644 --- a/vault/logical_system_test.go +++ b/vault/logical_system_test.go @@ -3631,10 +3631,10 @@ func TestSystemBackend_OASGenericMount(t *testing.T) { path string tag string }{ - {"/auth/{mountPath}/lookup", "auth"}, - {"/{mountPath}/{path}", "secrets"}, + {"/auth/token/lookup", "auth"}, + {"/cubbyhole/{path}", "secrets"}, {"/identity/group/id", "identity"}, - {"/{mountPath}/.*", "secrets"}, + {"/{secret_mount_path}/.*", "secrets"}, {"/sys/policy", "system"}, } @@ -3719,7 +3719,7 @@ func TestSystemBackend_OpenAPI(t *testing.T) { {"/auth/token/lookup", "auth"}, {"/cubbyhole/{path}", "secrets"}, {"/identity/group/id", "identity"}, - {"/secret/.*", "secrets"}, // TODO update after kv repo update + {"/secret/.*", "secrets"}, {"/sys/policy", "system"}, }