vault: limit token delay to not exceed token TTL #504
Merged
Conversation
This file contains 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
aead
commented
Jan 29, 2025
internal/keystore/vault/client.go
Outdated
| @@ -210,6 +213,9 @@ func (c *client) RenewToken(ctx context.Context, authenticate authFunc, secret * | |||
| if err == nil { | |||
| break | |||
| } | |||
| if resp, ok := err.(*vaultapi.ResponseError); ok && http.StatusBadRequest <= resp.StatusCode && resp.StatusCode < http.StatusInternalServerError { | |||
There was a problem hiding this comment.
Reduces the time we spent trying to renew the token
aead
force-pushed
the
var-token-renew-delay
branch
3 times, most recently
from
228d734 to
7d8d150
Compare
aead
force-pushed
the
var-token-renew-delay
branch
from
11d57e4 to
5c7f6b0
Compare
This commit fixes a bug in the Vault token renewal. When KES renews its current token `T1` and receives a new token `T2`, KES waits a certain amount of time before using `T2` to account for replication lag between Vault nodes. Vault tokens are not signed but static secrets. Each Vault node in a dist. cluster needs to know the token before being able to verify it. Without the usage delay described above, KES might send a request to a Vault node that has not received the new token `T2`, yet. Now, KES must also not wait longer then the remaining TTL of the current token `T1`. Otherwise, `T1` expires BEFORE KES starts using `T2`. This results in auth errors like the following: ``` Error making API request.\n\nURL: GET http://127.0.0.1:8200/v1/kv/data/my-key3\nCode: 403. Errors:\n\n* 2 errors occurred:\n\t* permission denied\n\t* invalid token\n\n" req.method=POST req.path=/v1/key/create/my-key3 req.ip=127.0.0.1 req.identity=a49fea12c5d1c69f1eba1e4697e62ccdbe389a80f317191892711b47d83c3e85 ``` This commit limits the max. time KES waits before using the new token `T2` to either half the remaining TTL of `T1` or 30s - whatever is shorter. This ensures that `T1` is still valid once KES switches to `T2`. Co-authored-by: Ramon de Klein <mail@ramondeklein.nl> Signed-off-by: Andreas Auernhammer <github@aead.dev>
aead
force-pushed
the
var-token-renew-delay
branch
from
5c7f6b0 to
f45aa1d
Compare
harshavardhana
approved these changes
Jan 30, 2025
|
Do we know why did this start to fail with |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This commit fixes a bug in the Vault token renewal. When KES renews its current token
T1and receives a new tokenT2, KES waits a certain amount of time before usingT2to account for replication lag between Vault nodes.Vault tokens are not signed but static secrets. Each Vault node in a dist. cluster needs to know the token before being able to verify it. Without the usage delay described above, KES might send a request to a Vault node that has not received the new token
T2, yet.Now, KES must also not wait longer then the remaining TTL of the current token
T1. Otherwise,T1expires BEFORE KES starts usingT2. This results in auth errors like the following:This commit limits the max. time KES waits before using the new token
T2to either half the remaining TTL ofT1or 30s - whatever is shorter. This ensures thatT1is still valid once KES switches toT2.