-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix regexes for sys/raw/
and sys/leases/lookup/
to match prevailing conventions
#21760
Conversation
…ng conventions There are several endpoints in Vault which take an arbitrary path as the last parameter. Many of these are defined in terms of the `framework.MatchAllRegex` helper. Some were not, and were defined using custom regexes which gave rise to multiple OpenAPI endpoints - one with the path parameter, and one without. We need to fix these definitions, because they give rise to a very unnatural result when used to generate a client API - for example, you end up with `LeasesLookUp()` which is only capable of being used to list the very top level of the hierarchical collection of leases, and `LeasesLookUpWithPrefix(prefix)` which must be used for all deeper levels. This PR changes the regexes used for `sys/raw/` and `sys/leases/lookup/` to be consistent with the approach used for other well-known similar endpoints, such as `cubbyhole/`, `kv-v1/` and `kv-v2/metadata/`. This PR does have a very small compatibility issue, which I think is tolerable: prior to this change, `sys/raw` with no trailing slash was considered a valid endpoint, and now it will no longer be. One way to observe this is to try `vault path-help sys/raw` - before this change, it would work, after, it will not. You would have to instead use `vault path-help sys/raw/foobar` to see the help. I also considered whether losing the ability to read/write/delete `sys/raw` would be an issue. In each case, the precise HTTP result code will change, but each of these were meaningless operations that make no sense - you cannot read/write/delete a "file" at the "root directory" of the underlying Vault storage. In fact, during testing, I discovered that currently, `vault write sys/raw x=y` when using Raft storage, will permanently break the Vault instance - it causes a panic within the Raft FSM, which re-occurs immediately on restarting the server! This PR also closes off that footgun / DoS vector. None of these issues apply to `sys/leases/lookup/`, as the existing regex in that case was already not matching the path without the trailing slash.
Please label |
This PR is great for the OpenAPI output and the generated API libraries!
I also agree that it's a reasonable compromise but I'm not 100% sure if there are any other side effects, perhaps someone in @hashicorp/vault-core has a better understanding of |
Just noticed a test failure, possibly due to the merge with main:
|
My other recent PR made the test cover more cases ... now the extended test has been merged into this branch, and demanded a further update to the changes here. Done. |
This PR will conflict with a simple cleanup-only PR that I have just filed, which would ideally be merged first: #21906 |
Conflict resolved. Note: I don't think this PR needs backporting and suggest removing all If you disagree, you'll want to backport #21906 first, to avoid conflicts in the backport cherrypicks. |
Please let me know if there's anything more I should be doing with this PR - for example, if you want any more evidence/test results/explanation regarding #21760 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken another look at related code and tried out sys/raw
with these changes and I think I have a better understanding now and enough confidence to merge this.
Thanks for fixing this issue, @maxb!
…#203) From hashicorp/vault#21760 (This PR actually uses an openapi.json carefully prepared with Git, so that I can prepare and file this PR whilst the previous "Sync OpenAPI" PR, #202, is still unmerged. They should merge together to produce the desired end result.)
There are several endpoints in Vault which take an arbitrary path as the
last parameter. Many of these are defined in terms of the
framework.MatchAllRegex
helper. Some were not, and were defined usingcustom regexes which gave rise to multiple OpenAPI endpoints - one with
the path parameter, and one without.
We need to fix these definitions, because they give rise to a very
unnatural result when used to generate a client API - for example, you
end up with
LeasesLookUp()
which is only capable of being used to listthe very top level of the hierarchical collection of leases, and
LeasesLookUpWithPrefix(prefix)
which must be used for all deeperlevels.
This PR changes the regexes used for
sys/raw/
andsys/leases/lookup/
to be consistent with the approach used for other well-known similar
endpoints, such as
cubbyhole/
,kv-v1/
andkv-v2/metadata/
.This PR does have a very small compatibility issue, which I think is
tolerable: prior to this change,
sys/raw
with no trailing slash wasconsidered a valid endpoint, and now it will no longer be.
One way to observe this is to try
vault path-help sys/raw
- beforethis change, it would work, after, it will not. You would have to
instead use
vault path-help sys/raw/foobar
to see the help.I also considered whether losing the ability to read/write/delete
sys/raw
would be an issue. In each case, the precise HTTP result codewill change, but each of these were meaningless operations that make no
sense - you cannot read/write/delete a "file" at the "root directory" of
the underlying Vault storage.
In fact, during testing, I discovered that currently,
vault write sys/raw x=y
when using Raft storage, will permanentlybreak the Vault instance - it causes a panic within the Raft FSM,
which re-occurs immediately on restarting the server! This PR also
closes off that footgun / DoS vector.
None of these issues apply to
sys/leases/lookup/
, as the existingregex in that case was already not matching the path without the
trailing slash.