Fix multiple OpenAPI generation issues with new AST-based generator #18554

maxb · 2022-12-26T10:50:24Z

Currently, the Vault OpenAPI generator attempts to use a variety of ad-hoc regexps, to simplify the path regexps that are the canonical specification of URL paths in Vault backends, and convert them into OpenAPI URL patterns. Regexps are not well suited to parsing regexps, and this approach is fragile and incomplete.

Happily, the Go regexp library provides easy access to its string-to-AST parser, so we can just ask it to parse the regexps for us, and then walk the resulting AST for a more reliable translation from regexps to OpenAPI patterns - this PR makes that change.

Whilst developing this change, it came to light that there are incorrectly unescaped full stops in some existing patterns - so the dot in /.well-known/ could in fact be any character. These are now properly escaped.

As a result of these changes, the OpenAPI document changes in the following ways:

In the AWS secrets engine, the /creds/{name} endpoint was being incorrectly documented as /creds - fixed (closes #18488).

In the GCP secrets engine, the /roleset and /static-account endpoints were missing - fixed.

In the KV v2 secrets engine, the /.* endpoint, which already had no visible operations since they are all marked as Unpublished, is now removed from the OpenAPI document entirely, since '/.* is not a valid OpenAPI path pattern.

In the PKI secrets engine, widespread inaccuracies are fixed (closes #18484):

The following endpoints, which were incorrectly undocumented, are added:

/cert/crl
/cert/delta-crl
/crl/delta
/crl/delta/pem
/crl/pem
/issuer/{issuer_ref}
/issuer/{issuer_ref}/crl
/issuer/{issuer_ref}/crl/delta
/issuer/{issuer_ref}/crl/delta/der
/issuer/{issuer_ref}/crl/delta/pem
/issuer/{issuer_ref}/crl/der
/issuer/{issuer_ref}/crl/pem
/issuer/{issuer_ref}/der
/issuer/{issuer_ref}/json
/issuer/{issuer_ref}/pem
/issuers/import/bundle
/issuers/import/cert
/keys/generate/exported
/keys/generate/internal
/keys/generate/kms

The following endpoints, which were documented but don't actually exist, are removed:

//delta
//delta/pem
//der
//json
//pem
/bundle
/cert
/delta-crl
/internal|exported
/kms
/{issuer_ref}/crl/pem|/der|/delta/pem
/{issuer_ref}/der|/pem

In the RADIUS auth method, /login endpoint no longer documents an incorrect urlusername field in the request body.

In the /sys/plugins/catalog/{type}/{name} endpoint, the type parameter is now no longer also incorrectly included in the request body.

In the /sys/tools/hash endpoints, the urlalgorithm parameter is now no longer incorrectly included in the request body.

In the /sys/tools/random and /transit/random endpoints, the source and urlbytes parameters are now no longer incorrectly included in the request body.

In various /transit/ endpoints, the urlalgorithm parameter is now no longer incorrectly included in the request body.

In the /transit/restore endpoints, the name parameter is now no longer incorrectly included in the request body.

These "parameter is now no longer incorrectly included in the request body" fixes are helpful, but accidental, and dependent on another bug - the request schema for multiple expansions for the same endpoint overwrites each other, with last generated wins, and the generation is simply happening in a more convenient order in this version of the code.

The paths including `/.well-known/` in the Vault API could currently technically be invoked with any random character in place of the dot.

…ed one

maxb · 2022-12-26T11:00:33Z

sdk/framework/openapi.go

+	if err != nil {
+		// Pattern cannot be transformed into sensible OpenAPI paths, so drop it entirely.
+		// See lengthier comment inside collectPathsFromRegexpASTInternal below for more detailed discussion.
+		return nil


I was undecided as to whether I should add trace-level logging at this point.

On the plus side: It makes it easier for plugin developers to find out this condition is being hit.

On the minus side: It is additional logging which is uninteresting to server operators, as there is nothing that can be done to change it other than rebuilding the Vault or plugin binary, and it will trigger every time sys/internal/specs/openapi or any relevant HelpOperation is invoked.

One alternative approach here would be to move the check to Backend.init, which is already set up to panic on initialisation if a backend author writes an empty or uncompilable path pattern - this could be expanded to include impossible-to-OpenAPI patterns not also marked as Unpublished. (I furthermore have another open PR, #18492, which seeks to add a check for CreateOperation being defined without an ExistenceCheck at the same point.)

Perhaps a reasonable compromise could be added to change this error:

return nil, fmt.Errorf("pattern could not be translated for OpenAPI due to %v operation", rx.Op)

into a named error and then capturing it here with if errors.Is(...) { return nil }. The advantage of this approach is that the code will be explicit about the intention of skipping specific regex operations. If collectPathsFromRegexpASTInternal is modified in the future to return other errors, those will be propagated correctly.

sdk/framework/openapi.go

lursu · 2023-01-10T15:45:54Z

sdk/framework/openapi_test.go

@@ -18,75 +18,6 @@ import (
 )

 func TestOpenAPI_Regex(t *testing.T) {
-	t.Run("Required", func(t *testing.T) {


Are all of these tests just not doing anything any more, or is there a need for more test cases to be added back in place of these?

All of the tests being deleted, test the behaviour of various ad-hoc regexps, which were being used to parse other regexps (!).

They have all been deleted, and their job replaced by regexp/syntax from the Go standard library.

The new code that has been written to process the output of the stdlib's regexp/syntax is already well-exercised by the existing TestOpenAPI_ExpandPattern - although, I could add some additional inputs and expected outputs to that test, which would fail with the old implementation, but pass with this rewritten one.

I could add some additional inputs and expected outputs to that test, which would fail with the old implementation, but pass with this rewritten one.

These would be awesome! I would also love to see some of the edge cases tested like ".*" that you mentioned in the comments.

averche

Overall I love this approach, I think it will be a great improvement to the stability of the generated OpenAPI output!

sdk/framework/openapi.go

averche · 2023-01-12T22:17:35Z

sdk/framework/openapi.go

+	if err != nil {
+		// Pattern cannot be transformed into sensible OpenAPI paths, so drop it entirely.
+		// See lengthier comment inside collectPathsFromRegexpASTInternal below for more detailed discussion.
+		return nil


Perhaps a reasonable compromise could be added to change this error:

return nil, fmt.Errorf("pattern could not be translated for OpenAPI due to %v operation", rx.Op)

into a named error and then capturing it here with if errors.Is(...) { return nil }. The advantage of this approach is that the code will be explicit about the intention of skipping specific regex operations. If collectPathsFromRegexpASTInternal is modified in the future to return other errors, those will be propagated correctly.

sdk/framework/openapi.go

averche · 2023-01-12T22:26:33Z

sdk/framework/openapi_test.go

@@ -18,75 +18,6 @@ import (
 )

 func TestOpenAPI_Regex(t *testing.T) {
-	t.Run("Required", func(t *testing.T) {


I could add some additional inputs and expected outputs to that test, which would fail with the old implementation, but pass with this rewritten one.

These would be awesome! I would also love to see some of the edge cases tested like ".*" that you mentioned in the comments.

Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>

…regexp-syntax

…tion

maxb · 2023-01-25T08:22:55Z

vault/logical_system_test.go

-		t.Fatalf("response size too small; expected: min %d, actual: %d", minLen, len(body))
-	}
-}
-


Note for reviewer: this test is being removed because it was a partial copy of the next test, just changed to have generic_mount_paths=true.

Since I needed to make changes, and didn't want to have to update both copies of the duplicated code, I resolved the duplication.

maxb · 2023-01-25T08:24:22Z

vault/logical_system_test.go

 func TestSystemBackend_OpenAPI(t *testing.T) {
 	_, b, rootToken := testCoreSystemBackend(t)
-	var oapi map[string]interface{}


Note for reviewer: You're going to want to view this part of the diff ignoring whitespace.

I noticed there was potential cross-talk between local variables in different parts of this function, so I introduced enclosing scopes so each part of the function is defining its own local variables.

maxb · 2023-01-25T08:32:49Z

vault/logical_system_test.go

+			{path: "/auth/token/lookup", tag: "auth"},
+			{path: "/cubbyhole/{path}", tag: "secrets"},
+			{path: "/identity/group/id", tag: "identity"},
+			{path: expectedSecretPrefix + "^.*$", unpublished: true},


Note for reviewer: This secret thing here is actually neither a KV v1 nor a KV v2, it's actually the builtin passthrough backend within the Vault core, which has its own path regexp which is different to the real KV v1 that users use.

You can see here that the old code generated a path of .* (because it strips out the anchor metacharacters), whereas the new code leaves them in, giving ^.*$ - I think that's an accidental improvement, as it makes it even more clear this is a weird edge case.

Since the new code forces operations whose regexps can't be successfully translated to OpenAPI patterns to be unpublished, the test expectations arae being updated accordingly.

maxb · 2023-01-25T08:33:49Z

All feedback addressed and CircleCI is now happy.

averche

LGTM 👍 This is an awesome contribution! Thanks for fixing all the tests & adding new ones.

Skipping most OpenAPI tests as fix on 1.13 wasn't backported. See also: #18554 Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

…o release/1.12.x (#19712) * Split (un,)authenticated issuer fetch endpoints Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Add tests to validate endpoint authentication status Skipping most OpenAPI tests as fix on 1.13 wasn't backported. See also: #18554 Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> --------- Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> Co-authored-by: Alexander Scheel <alex.scheel@hashicorp.com>

…o release/1.11.x (#19711) * Split (un,)authenticated issuer fetch endpoints Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Add tests to validate endpoint authentication status Skipping most OpenAPI tests as fix on 1.13 wasn't backported. See also: #18554 Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> --------- Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> Co-authored-by: Alexander Scheel <alex.scheel@hashicorp.com>

…18554) * Regexp metacharacter `.` should be escaped when used literally The paths including `/.well-known/` in the Vault API could currently technically be invoked with any random character in place of the dot. * Replace implementation of OpenAPI path translator with regexp AST-based one * Add changelog * Typo fix from PR review - thanks! Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> * Add comment based on review feedback * Change style of error handling as suggested in code review * Make a further tweak to the handling of the error case * Add more tests, testing cases which fail with the previous implementation * Resolve issue with a test, and improve comment --------- Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>

… generator into release/1.12.x (#19718) * Fix multiple OpenAPI generation issues with new AST-based generator (#18554) * Regexp metacharacter `.` should be escaped when used literally The paths including `/.well-known/` in the Vault API could currently technically be invoked with any random character in place of the dot. * Replace implementation of OpenAPI path translator with regexp AST-based one * Add changelog * Typo fix from PR review - thanks! Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> * Add comment based on review feedback * Change style of error handling as suggested in code review * Make a further tweak to the handling of the error case * Add more tests, testing cases which fail with the previous implementation * Resolve issue with a test, and improve comment --------- Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> * fix test for generic mount paths --------- Co-authored-by: Max Bowsher <maxbowsher@gmail.com> Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> Co-authored-by: Anton Averchenkov <anton.averchenkov@hashicorp.com>

… generator into release/1.11.x (#19717) * Fix multiple OpenAPI generation issues with new AST-based generator (#18554) * Regexp metacharacter `.` should be escaped when used literally The paths including `/.well-known/` in the Vault API could currently technically be invoked with any random character in place of the dot. * Replace implementation of OpenAPI path translator with regexp AST-based one * Add changelog * Typo fix from PR review - thanks! Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> * Add comment based on review feedback * Change style of error handling as suggested in code review * Make a further tweak to the handling of the error case * Add more tests, testing cases which fail with the previous implementation * Resolve issue with a test, and improve comment --------- Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> * Fix test --------- Co-authored-by: Max Bowsher <maxbowsher@gmail.com> Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com> Co-authored-by: Anton Averchenkov <anton.averchenkov@hashicorp.com>

maxb added 2 commits December 24, 2022 14:22

Regexp metacharacter . should be escaped when used literally

6a4bf8b

The paths including `/.well-known/` in the Vault API could currently technically be invoked with any random character in place of the dot.

Replace implementation of OpenAPI path translator with regexp AST-bas…

3b24b5a

…ed one

maxb commented Dec 26, 2022

View reviewed changes

sdk/framework/openapi.go Outdated Show resolved Hide resolved

Add changelog

de69c3d

maxb mentioned this pull request Jan 6, 2023

Improve OpenAPI request/response naming strategy #18321

Closed

averche requested review from dhuckins, lursu, averche and AnPucel January 9, 2023 22:29

lursu reviewed Jan 10, 2023

View reviewed changes

averche reviewed Jan 12, 2023

View reviewed changes

maxb and others added 8 commits January 13, 2023 09:20

Typo fix from PR review - thanks!

7005f1a

Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>

Merge remote-tracking branch 'upstream/main' into openapi-generation-…

85976b2

…regexp-syntax

Add comment based on review feedback

8d7fb45

Merge remote-tracking branch 'upstream/main' into openapi-generation-…

759d69c

…regexp-syntax

Change style of error handling as suggested in code review

ad19b0d

Make a further tweak to the handling of the error case

8194c82

Add more tests, testing cases which fail with the previous implementa…

61afb41

…tion

Resolve issue with a test, and improve comment

78b6554

maxb commented Jan 25, 2023

View reviewed changes

averche approved these changes Jan 30, 2023

View reviewed changes

averche added this to the 1.13.0-rc1 milestone Jan 30, 2023

averche merged commit fd9cadb into hashicorp:main Jan 31, 2023

maxb deleted the openapi-generation-regexp-syntax branch February 1, 2023 05:57

cipherboy added a commit that referenced this pull request Mar 23, 2023

Add tests to validate endpoint authentication status

7f35122

Skipping most OpenAPI tests as fix on 1.13 wasn't backported. See also: #18554 Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

cipherboy added a commit that referenced this pull request Mar 23, 2023

Add tests to validate endpoint authentication status

f556898

Skipping most OpenAPI tests as fix on 1.13 wasn't backported. See also: #18554 Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

averche added backport/1.11.x labels Mar 23, 2023

This was referenced Mar 23, 2023

Backport of Fix multiple OpenAPI generation issues with new AST-based generator into release/1.12.x #19718

Merged

Backport of Fix multiple OpenAPI generation issues with new AST-based generator into release/1.11.x #19717

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix multiple OpenAPI generation issues with new AST-based generator #18554

Fix multiple OpenAPI generation issues with new AST-based generator #18554

maxb commented Dec 26, 2022 •

edited

Loading

maxb Dec 26, 2022

averche Jan 12, 2023

maxb Jan 25, 2023

lursu Jan 10, 2023

maxb Jan 10, 2023

averche Jan 12, 2023

maxb Jan 25, 2023

averche left a comment

averche Jan 12, 2023

averche Jan 12, 2023

maxb Jan 25, 2023

maxb Jan 25, 2023

maxb Jan 25, 2023

maxb commented Jan 25, 2023

averche left a comment

Fix multiple OpenAPI generation issues with new AST-based generator #18554

Fix multiple OpenAPI generation issues with new AST-based generator #18554

Conversation

maxb commented Dec 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

averche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxb commented Jan 25, 2023

averche left a comment

Choose a reason for hiding this comment

maxb commented Dec 26, 2022 •

edited

Loading