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 `{<path-of-mount>_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

@@ -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 `{<type>_mount_path}`
+```

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)
 	}
 

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

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("")

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)
 		}
 

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

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"},
 	}