secrets/hashivault: reject keyIDs that escape the engine/operation path - #3763
Merged
vangent merged 2 commits intoJul 31, 2026
Merged
Conversation
Decrypt and Encrypt build the Vault API request path with path.Join(opts.Engine+"/decrypt"|"/encrypt", keyID). path.Join cleans ".." segments, so a keyID such as "../../sys/health" silently produces a request outside the intended engine and operation entirely, unlike every other secrets driver in this module (awskms/gcpkms hand key identification to the cloud provider's structured API; azurekeyvault validates keyID with a strict regex). Since Vault's own authorization model is commonly path-prefix ACLs (e.g. a policy scoped to "transit/decrypt/tenant-a-*"), a caller that only controls the key name -- the documented use of OpenKeeper's keyID parameter -- could reach a path the deploying application never intended to expose. Add vaultPath, which joins and then verifies the result is still inside the given prefix, rejecting the request client-side otherwise.
…r keyID rejection Decrypt already had a regression test; add the symmetric case for Encrypt (which goes through the same vaultPath check) and an end-to-end test through OpenKeeper to confirm a keyID supplied this way is rejected before any request reaches the server, not just when calling the driver.Keeper directly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3763 +/- ##
==========================================
+ Coverage 75.38% 75.41% +0.02%
==========================================
Files 104 104
Lines 14241 14246 +5
==========================================
+ Hits 10736 10743 +7
+ Misses 2768 2766 -2
Partials 737 737 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
keeper.Decrypt/keeper.Encrypt(secrets/hashivault/vault.go) build the literal Vault API request path withpath.Join(opts.Engine+"/decrypt", keyID)/.../encrypt.path.Joincleans..segments, so akeyIDsuch as"../../sys/health"makes the actual HTTP request escape the intended engine and operation entirely — confirmed against a fake Vault server, wherekeyID = "a/../../../auth/token/create"produced a request to/v1/auth/token/createinstead of/v1/transit/decrypt/....This is the only secrets driver in this module that builds a raw path this way:
awskms/gcpkmspass their key identifier as a structured field on the cloud SDK's request type (server-side authorization decides what's valid), andazurekeyvaultvalidateskeyIDwith a strict regex before use.hashivault'skeyIDis exactly the parameterOpenKeeper(client, keyID, opts)is documented to take directly, and Vault's own authorization model is commonly path-prefix ACLs (e.g. a policy scoped totransit/decrypt/tenant-a-*for a specific tenant/key namespace) — so a caller that only controls the key name, not the Vault client's credentials, can reach paths the deploying application never intended to expose.Adds
vaultPath, which joins the prefix andkeyIDand then verifies the result is still inside that prefix, returning an error instead of sending the request otherwise.tenant-a/../tenant-b(relative navigation that stays within<engine>/<op>/) still works; only escapes past that boundary are rejected.