Fix sudo paths missing from OpenAPI and docs #21772

maxb · 2023-07-12T07:35:23Z

Various sudo (a.k.a. root-protected) paths are implemented in
non-standard ways, and as a result:

are not declared as x-vault-sudo in the OpenAPI spec
and as a result of that, are not included in the hardcoded patterns
powering the Vault CLI -output-policy flag
and in some cases are missing from the table of all sudo paths in the
docs too

Fix these problems by:

Adding seal and step-down to the list of root paths for the system
backend. They don't need to be there for enforcement, as those two
special endpoints bypass the standard request handling code, but they
do need to be there for the OpenAPI generator to be able to know they
require sudo.

The way in which those two endpoints do things differently can be
observed in the code search results for RootPrivsRequired:
https://github.com/search?q=repo%3Ahashicorp%2Fvault%20RootPrivsRequired&type=code
Fix the implementation of auth/token/revoke-orphan to implement
endpoint sudo requirements in the standard way. Currently, it has an
incorrect path declared in the special paths metadata, and then
compensates with custom code throwing an error within the request
handler function itself.

Various sudo (a.k.a. root-protected) paths are implemented in non-standard ways, and as a result: * are not declared as x-vault-sudo in the OpenAPI spec * and as a result of that, are not included in the hardcoded patterns powering the Vault CLI `-output-policy` flag * and in some cases are missing from the table of all sudo paths in the docs too Fix these problems by: * Adding `seal` and `step-down` to the list of root paths for the system backend. They don't need to be there for enforcement, as those two special endpoints bypass the standard request handling code, but they do need to be there for the OpenAPI generator to be able to know they require sudo. The way in which those two endpoints do things differently can be observed in the code search results for `RootPrivsRequired`: https://github.com/search?q=repo%3Ahashicorp%2Fvault%20RootPrivsRequired&type=code * Fix the implementation of `auth/token/revoke-orphan` to implement endpoint sudo requirements in the standard way. Currently, it has an **incorrect** path declared in the special paths metadata, and then compensates with custom code throwing an error within the request handler function itself.

maxb · 2023-07-12T07:38:33Z

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


Note to reviewer: I wonder if we might instead just delete this test entirely? It is just retrieving the value of the root paths slice, which is defined as a literal in the main code, and then comparing it to ... another identical literal definition.

It seems to me this is a pointless thing to be testing - of course two literals that are identically defined, will have equal values.

I also don't see much value in this particular test and wouldn't object if it was deleted.

Updated to remove the test.

maxb · 2023-07-12T07:40:25Z

vault/token_store.go

-				"revoke-orphan/*",
-				"accessors*",
+				"revoke-orphan",
+				"accessors/",
 			},


Note to reviewer:

The prior definition of revoke-orphan/* is just plain wrong, so it is fixed.

The prior definition of accessors* technically works, because it uses a needless wildcard - that change is just for precision.

vault/token_store.go

maxb · 2023-07-12T07:47:08Z

vault/token_store_test.go

 	req.Data = map[string]interface{}{
 		"token": "child",
 	}
 	req.ClientToken = root
-	resp, err := ts.HandleRequest(namespace.RootContext(nil), req)
+	resp, err := c.HandleRequest(namespace.RootContext(nil), req)
 	if err != nil || (resp != nil && resp.IsError()) {


Note to reviewer: Since the common sudo-enforcing logic is part of the Core, we need to fix tests which care about the sudo behaviour, and were previously slipping requests directly to the backend, bypassing the Core, to make the request via the proper layers.

This test is the case where the requesting token does have sudo rights, testing the "allowed" case, and the next test below is testing the "denied" case.

maxb · 2023-07-12T07:48:46Z

website/content/docs/concepts/policies.mdx

@@ -799,44 +799,45 @@ authenticated user.
 ## Root protected API endpoints

 ~> **Note:** Vault treats the HTTP POST and PUT verbs as equivalent, so for each mention
-   of POST in the table above, PUT may also be used. Vault uses the non-standard LIST HTTP
+   of POST in the table below, PUT may also be used. Vault uses the non-standard LIST HTTP
   verb, but also allows list requests to be made using the GET verb along with `?list=true`
   as a query parameter, so for each mention of LIST in the table above, GET with `?list=true`
   may also be used.

 The following paths requires a root token or `sudo` capability in the policy:



Note to reviewer: Only one line in the table has semantic changes, please use a whitespace-ignoring diff mode to see what is really going on: https://github.com/hashicorp/vault/pull/21772/files?diff=unified&w=1#diff-cc6f6f882b5e1952856decc6cefb2caf95af3ead2ece1d3b3cd1073d92c2625e

There are other factual inaccuracies in this table (see issue #20780), but I've refrained from expanding the scope of this PR beyond the specific endpoints I was making code changes to.

averche

This looks good to me, but probably could use another look from @hashicorp/vault-core to verify the removal of the sudo check.

maxb · 2023-07-13T06:31:10Z

This looks good to me, but probably could use another look from @hashicorp/vault-core to verify the removal of the sudo check.

Makes sense ... for the benefit of someone freshly looking at this PR, we're not removing a sudo requirement entirely - just removing an unusual manual check of sudo status in the middle of an operation handler, in favour of fixing an incorrectly declared path pattern, so that the standard declarative method of enforcing sudo requirements applies instead.

maxb · 2023-07-18T09:56:51Z

This PR will conflict with a simple cleanup-only PR that I have just filed, which would ideally be merged first: #21906

maxb · 2023-07-18T13:28:36Z

Conflict resolved.

Note: I don't think this PR needs backporting and suggest removing all backport/* labels.

If you disagree, you'll want to backport #21906 first, to avoid conflicts in the backport cherrypicks.

…onstant

vault/token_store.go

Co-authored-by: Anton Averchenkov <[email protected]>

averche

Thanks again for putting this together!

This is a follow-up to hashicorp#21772. Historically, for some reason, `auth/token/revoke-orphan` was sudo-protected by writing custom code in its handler function, instead of via the usual declarative PathsSpecial.Root mechanism. In fact, there was a declaration mentioning revoke-orphan in the token backend's PathsSpecial.Root, but it was incorrect! That was corrected in hashicorp#21772, making the custom code in the handler function redundant. However, removal of the now-redundant code was deferred to this follow-up PR, out of an abundance of caution, and wanting extra eyes on a change deleting a security check.

maxb · 2023-07-20T07:28:41Z

I have created #21968 to address the last bit of cleanup, that it was decided needed extra reviewers.

This is a follow-up to #21772. Historically, for some reason, `auth/token/revoke-orphan` was sudo-protected by writing custom code in its handler function, instead of via the usual declarative PathsSpecial.Root mechanism. In fact, there was a declaration mentioning revoke-orphan in the token backend's PathsSpecial.Root, but it was incorrect! That was corrected in #21772, making the custom code in the handler function redundant. However, removal of the now-redundant code was deferred to this follow-up PR, out of an abundance of caution, and wanting extra eyes on a change deleting a security check.

maxb requested a review from a team as a code owner July 12, 2023 07:35

maxb commented Jul 12, 2023

View reviewed changes

averche added core/openapi backport/1.12.x labels Jul 12, 2023

averche added this to the 1.15 milestone Jul 12, 2023

averche self-assigned this Jul 12, 2023

averche requested a review from a team July 12, 2023 22:48

averche approved these changes Jul 12, 2023

View reviewed changes

averche mentioned this pull request Jul 12, 2023

OpenAPI: Separate ListOperation from ReadOperation #21723

Merged

maxb added 3 commits July 13, 2023 22:11

Merge branch 'main' into fix-stealth-sudo-paths

f7ca560

changelog

64530c2

Merge branch 'main' into fix-stealth-sudo-paths

ed8edc0

Merge branch 'main' into fix-stealth-sudo-paths

b4244c4

averche removed backport/1.12.x labels Jul 18, 2023

maxb added 3 commits July 19, 2023 16:15

Merge branch 'main' into fix-stealth-sudo-paths

22b66de

As discussed in PR, delete test which is just testing equality of a c…

bbe23bd

…onstant

Restore sudo check as requested, and add comment

c1b1fb6

averche reviewed Jul 19, 2023

View reviewed changes

vault/token_store.go Outdated Show resolved Hide resolved

Update vault/token_store.go

651bbae

Co-authored-by: Anton Averchenkov <[email protected]>

averche approved these changes Jul 19, 2023

View reviewed changes

averche enabled auto-merge (squash) July 19, 2023 16:22

averche merged commit 188bdca into hashicorp:main Jul 19, 2023

maxb deleted the fix-stealth-sudo-paths branch July 19, 2023 16:36

maxb mentioned this pull request Jul 20, 2023

Remove redundant sudo check #21968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sudo paths missing from OpenAPI and docs #21772

Fix sudo paths missing from OpenAPI and docs #21772

maxb commented Jul 12, 2023

maxb Jul 12, 2023

averche Jul 12, 2023

maxb Jul 19, 2023 •

edited

Loading

maxb Jul 12, 2023

maxb Jul 12, 2023

maxb Jul 12, 2023 •

edited

Loading

averche left a comment

maxb commented Jul 13, 2023

maxb commented Jul 18, 2023

maxb commented Jul 18, 2023

averche left a comment

maxb commented Jul 20, 2023

Fix sudo paths missing from OpenAPI and docs #21772

Fix sudo paths missing from OpenAPI and docs #21772

Conversation

maxb commented Jul 12, 2023

maxb Jul 12, 2023

Choose a reason for hiding this comment

averche Jul 12, 2023

Choose a reason for hiding this comment

maxb Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

maxb Jul 12, 2023

Choose a reason for hiding this comment

maxb Jul 12, 2023

Choose a reason for hiding this comment

maxb Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

averche left a comment

Choose a reason for hiding this comment

maxb commented Jul 13, 2023

maxb commented Jul 18, 2023

maxb commented Jul 18, 2023

averche left a comment

Choose a reason for hiding this comment

maxb commented Jul 20, 2023

maxb Jul 19, 2023 •

edited

Loading

maxb Jul 12, 2023 •

edited

Loading