-
Notifications
You must be signed in to change notification settings - Fork 113
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
use the latest key version #1428
base: main
Are you sure you want to change the base?
use the latest key version #1428
Conversation
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com> Co-authored-by: Furkan Türkal <furkan.turkal@trendyol.com>
I've programmed a similar integration in Vault before, and wonder whether it's a good idea to hit the Vault API just to verify a signature 🙂 Because you really only need (assuming keys are not exportable) the Vault Transit Secrets Engine to make a signature but not verify it. This would yield in better performance and reliability, but you do need knowledge about the public key from somewhere else. WDYT? |
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'll let Bob review this PR since he has more context.
I'm mostly familiar with GCP KMS, which I believe allows either latest or a specific key version. Should we do the same here? Some may prefer a specific key version to avoid silently switching out the key.
There is certainly this issue also, yes. Perhaps default to latest is version is unspecified, but fetch the exact version when it is specified. |
@@ -313,7 +313,27 @@ func (h hashivaultClient) verify(sig, digest []byte, alg crypto.Hash, opts ...si | |||
if h.keyVersion > 0 { | |||
vaultDataPrefix = fmt.Sprintf("vault:v%d:", h.keyVersion) | |||
} else { | |||
vaultDataPrefix = vaultV1DataPrefix | |||
// use hashivault client to grab the latest version of the key and use that as the prefix |
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 have one concern with this implementation - it ignores the keyCache
completely; I'd rather have it invalidate the cache, call public()
(which would hydrate the cache with the latest_version
.
It still results in another round trip to Vault to do the verification, but we at least have the key material for re-use in memory.
Hi all, any updates on this issue? stuck with the same problem. |
Also interested in updates, has anyone looked at this recently? |
Summary
based on the discussions here, we added a code to use the latest version of the rotated key.
P.S. If you think this is wrong please feel free to close the PR
Release Note
Documentation