OpenAPI defects introduced with parameterized mount path change

[maxb]

I have noticed there is a big change in the OpenAPI generator in 1.13-dev, introduced in #17926.

The change has some issues:

Changelog entry

The change has no changelog entry, despite being very visible to anyone who is using the existing OpenAPI document.

(OK, I see there was a changelog entry in previous incarnations of the PR, but then it was reverted, and when re-introduced, the changelog entry was not included.)

~~The original changelog entry was: ~~

  ```release-note:improvement
  openapi: Add {mount_path} parameter to secret & auth paths in the generated openapi.json spec.
  ```

Ideally this would be expanded upon:

• The parameter is not actually {mount_path}, it is {<mount-point>_mount_path}, e.g. for a mount at foo-bar/, the parameter would be {foo-bar_mount_path}
• The change has modified the Vault API by deleting the generic_mount_paths parameter from the sys/internal/specs/openapi path - shouldn't an HTTP change be more visibly mentioned?

EDIT: #18663 includes a changelog entry which I think goes far enough that it's not worth figuring out how to retroactively insert another one.

Removal of generic_mount_paths parameter incomplete

I opened #18558 to fix.

EDIT: Resolved by reverting to re-introduce the functionality.

Introduction of overly broad regexp into CLI -output-policy processing

Following this change, the -output-policy CLI flag will now suggest that the sudo capability is needed for any path at all that ends in /root, including, for example, some ad-hoc user data in a KV secrets engine. Arguably this is due to a pre-existing defect in -output-policy relying too much on assumptions, though. I've also reported this in an issue about -output-policy - #18559 .

EDIT: Resolved by revert.

Inconsistent handling of singleton mounts

The change to the OpenAPI singles out sys/ and identity/ for special handling, keeping those paths as literals in the OpenAPI document, when everything else is parameterised. I do realise that this was a pre-existing conditional in the code before this PR, but by forcibly opting everyone in to generic_mount_paths, it has now become much more prominent.

If sys/ and identity/ are going to get special handling, shouldn't the other two special singleton mounts, auth/token/ and cubbyhole/ get the same treatment?

EDIT: Resolution proposed in #18663

Inconsistent handling of KV secret engines

If I mount a KV secrets engine at, for example, kv-v2/, I end up with an OpenAPI document which contains /{kv-v2_mount_path}/ in its paths, but secret_mount_path in its defined parameters. They don't match.

This is because these two bits of code are inconsistent:

vault/sdk/framework/openapi.go

Lines 268 to 271 in 159b60a

	defaultMountPath := requestResponsePrefix
	if requestResponsePrefix == "kv" {
	defaultMountPath = "secret"
	}

vault/vault/logical_system.go

Lines 4577 to 4584 in 159b60a

	var docPath string
	if mount == "kv/" {
	docPath = fmt.Sprintf("/%s{secret_mount_path}/%s", mountPrefix, path)
	} else if mount != "sys/" && mount != "identity/" {
	docPath = fmt.Sprintf("/%s{%s_mount_path}/%s", mountPrefix, strings.TrimRight(mount, "/"), path)
	} else {
	docPath = fmt.Sprintf("/%s%s%s", mountPrefix, mount, path)
	}

In the first, the engine type is being checked (yes, requestResponsePrefix is actually set to the plugin type, and not the plugin mount path... that is confusing too.)

In the second, the engine path is being checked.

Hence, the conditions are different.

I suggest the best fix here would be to just delete the kv -> secret special case entirely - I can't think of a good reason for it to exist.

EDIT: Resolved by removal of that code.

Inconsistent insertion of <mount-point>_mount_path parameter

The addition of the <mount-point>_mount_path parameter to the OpenAPI parameters collection is handled in sdk/framework/ code.
The patching of the operation path to include the /{<mount-point>_mount_path}/ template is handled in Vault core code!

This mismatch causes multiple problems.

The patching of the operation path only occurs in the code path for the sys/internal/specs/openapi endpoint. If, instead, you are retrieving an OpenAPI document for a single backend, or single path on a backend, via the ?help=1 route, each endpoint in the response includes this bogus addition:

        "parameters": [
          {
            "name": "_mount_path",
            "description": "Path where the backend was mounted; the endpoint path will be offset by the mount path",
            "in": "path",
            "schema": {
              "type": "string",
              "default": ""
            }
          },

a parameter called _mount_path.

The other problem caused by this, is that since half of the implementation is in the sdk/framework/ code, external plugins need to be rebuilt against a newer sdk version to pick up the change!

For both of these reasons, I think it is critical that the addition of the parameter definition to the OpenAPI document be moved from the sdk/framework/ code to happen instead in vault/logical_system.go, in the same place as where the paths are prefixed.

EDIT: Resolution proposed in #18663

Lack of sanitization of esoteric characters in mount paths

Suppose I have a secrets engine mounted at foo/bar/baz/ - the current OpenAPI will end up with a parameter named {foo/bar/baz_mount_path}. Now, admittedly I can't find anything in the OpenAPI spec to say which characters are permitted in path template variables - but I imagine many people would intuitively expect parameter names to not contain special characters in this way.

EDIT: The comment added in #18663 clarifying that generic_mount_paths is only really useful in a particular niche use case makes this far less important, to the point I'm happy to consider this closed.

[maxb]

A further issue:

The path parameters are being marked as not required:

vault/sdk/framework/openapi.go

Line 282 in 159b60a

Required: false,

This is against the OpenAPI spec:

If the parameter location is "path", this property is REQUIRED and its value MUST be true.

and causes validators such as https://pypi.org/project/openapi-spec-validator/ to deem the OpenAPI document invalid.

EDIT: Resolved by revert (#18617).

[averche]

Hi @maxb, thank you again for a very detailed report! You bring up a lot of good points and suggestions.

For a bit of background, the motivation for this change was to support a single source-of-truth OpenAPI document generated for built-in plugins using this script. The document would then be used to generate a few vault client libraries. The script itself is very hacky and probably not the solution we want to have in the end. We have been entertaining other ideas such as re-implementing the script in Go. The code would iterate over all the plugins without mounting them or calling the OpenAPI endpoint. However, this solution is still only in the planning stage.

Changelog entry

Yes, somehow that got overlooked with the PR being resubmitted a few times. If we decide to stick with this approach, I suppose it makes sense to create another dummy PR just to mention this change.

Removal of generic_mount_paths parameter incomplete
I opened #18558 to fix.

Thank you for spotting and fixing it!

Introduction of overly broad regexp into CLI -output-policy processing

Yes, this was an unintended side effect, though we should probably think about a better solution for the -output-policy sudo logic as you mentioned in #18559, perhaps remove the dependency on openapi there, if possible.

Inconsistent handling of singleton mounts

I think the intention was to have an inclusion list (all the built-in auth & secrets plugins) rather than exclusion list (/sys and /identity). However, the former was more difficult to maintain. I'm not sure if this logic should apply to other plugins.

Inconsistent handling of KV secret engines
I suggest the best fix here would be to just delete the kv -> secret special case entirely - I can't think of a good reason for it to exist.

I agree, this entry is not very useful and causes a lot of issues. I think I've done precisely what you suggested in #18321.

Inconsistent insertion of _mount_path parameter

This is definitely one of the biggest issues with the change. I agree that the implementation for both parts should be in vault/logical_system.go

Lack of sanitization of esoteric characters in mount paths

Yes, that's a very good point!

Solutions

There are a couple options that I'm entertaining to address all these issues:

1. Revert Add mount path into the default generated openapi.json spec (UI) #17926 / Remove generic_mount_paths field #18558 and change the gen_openapi.sh script to set generic_mount_paths=true. Some other changes would need to be made to fix the mount path logic but they should not affect anyone not using generic_mount_paths flag.

2. Keep the mount path logic but isolate it to only the built-in plugins (rather than only excluding /sys and /internal) as well as address the individual points you raised above (e.g. move the parameter code out of the sdk package and into vault/logical_system.go)

I'm leaning towards the first solution. The more I think about it, the more I feel like the logic to generate the library-ready OpenAPI document should be somewhat decoupled from the openapi endpoint. As I mentioned at the beginning, we may not even need to use this endpoint in the future. What do you think?

[maxb]

I agree that the requirements for an OpenAPI document for driving Swagger UI a.k.a. API Explorer, or other automation tooling, may not be entirely compatible with the requirements for an OpenAPI document to drive client library auto-generation, and therefore allowing for an option to generate two different flavours depending on the intended target, is a useful way to break that deadlock.

Additionally, reverting to an optional approach means anyone with tooling that expects the path conventions used in earlier versions of Vault, has a fully-compatible experience migrating to Vault 1.13.

I would consider moving the parameter addition code out of the sdk package as something that should be done anyway, regardless of the chosen solution.

Here is another thing to consider: what experience do we want to present to users who browse to the built in API Explorer within the Vault UI? Should they be provided with an experience that includes simpler paths (as with Vault 1.12 and earlier), or one that includes generic mount paths with placeholders, in the UI?

I can see possible arguments in both directions, but I am weakly leaning towards that the simpler paths (as with Vault 1.12 and earlier) are better:

• Better for a beginner user, because the appearance of the {whatever_mount_path} placeholder in nearly every path is yet another surprise complication to take in along with the rest of the API.

• Better for the advanced user, because once you've come to understand the concept of mounting secret engines and auth methods, the idea that the chunk of the API can be replicated at a different path subtree is easy to understand.

I will wait for you to decide whether to go ahead with the revert, as that will move a lot of code around - but afterwards I'd be happy to write a PR for the "moving the parameter addition code out of the sdk package" part, if that would be useful.

[averche]

After some discussion with the team, we have agreed to revert the changes and start applying the necessary fixes after the reverts.

I would consider moving the parameter addition code out of the sdk package as something that should be done anyway, regardless of the chosen solution.

Agreed.

Here is another thing to consider: what experience do we want to present to users who browse to the built in API Explorer within the Vault UI? Should they be provided with an experience that includes simpler paths (as with Vault 1.12 and earlier), or one that includes generic mount paths with placeholders, in the UI?

I can see possible arguments in both directions, but I am weakly leaning towards that the simpler paths (as with Vault 1.12 and earlier) are better:

That's a good point to consider. I also agree that simpler paths would be a better default behaviour in this context. The {whatever_mount_path} is more of a generic placeholder for libraries, whereas the user's UI is specific to what they have mounted in their Vault. When a user has mounted their secret/auth engines at arbitrary locations, they will see those reflected in the simple openapi paths that get generated.

On the other hand we also support -mount-path in certain CLI commands so it may become a more established concept going forward. I think we can start with simple mount paths and, at a later point, add a toggle in the UI that allows users to switch to generic mount paths (if that's something that users might be interested in).

I will wait for you to decide whether to go ahead with the revert, as that will move a lot of code around - but afterwards I'd be happy to write a PR for the "moving the parameter addition code out of the sdk package" part, if that would be useful.

Thank you! Your help will be very appreciated.

[averche]

The following 2 changes have been reverted:

• db8efac (Add mount path into the default generated openapi.json spec (UI) #17926)
• 79c8f62 (Remove generic_mount_paths field #18558)

[maxb]

I will wait for you to decide whether to go ahead with the revert, as that will move a lot of code around - but afterwards I'd be happy to write a PR for the "moving the parameter addition code out of the sdk package" part, if that would be useful.

Thank you! Your help will be very appreciated.

I have opened #18663 for your consideration

[maxb]

Everything I raised here is now sufficiently dealt with - closing.