Mark expired Vault token as expired when Vault is unreachable #6235
Conversation
louisruch
left a comment
louisruch
left a comment
There was a problem hiding this comment.
This will not work as you expect, the clientStore created from the renewRevokeStore does not retrieve or set the ExpirationTime - this change will therefore just expire all tokens on any error returned from Vault. We would need to update the store .token() method and update the query to return the expiration time for this to work.
We may also want to consider an alternative path here where the query we run filters out tokens that are past their expiration time - though we would still want to update these tokens to have an expired status somewhere.
louisruch
left a comment
louisruch
left a comment
There was a problem hiding this comment.
Might wanna sync up with @laero or @bgajjala8 to chat through order or merges though they have some cleanup to do on the LLB already
.../schema/migrations/oss/postgres/102/01_credential_vault_token_renewal_revocation_view.up.sql
Show resolved
Hide resolved
.../schema/migrations/oss/postgres/101/01_credential_vault_token_renewal_revocation_view.up.sql
Outdated
Show resolved
Hide resolved
.../schema/migrations/oss/postgres/101/01_credential_vault_token_renewal_revocation_view.up.sql
Outdated
Show resolved
Hide resolved
.../schema/migrations/oss/postgres/101/01_credential_vault_token_renewal_revocation_view.up.sql
Outdated
Show resolved
Hide resolved
| ldapContainer any | ||
| postgresContainer any | ||
|
|
||
| stopped bool |
There was a problem hiding this comment.
Should we use an atomic here?
| // TestTokenRenewalJob_Run_VaultUnreachableTemporarily tests that tokens are not marked | ||
| // as expired when Vault is unreachable temporarily. | ||
| func TestTokenRenewalJob_Run_VaultUnreachableTemporarily(t *testing.T) { | ||
| // t.Parallel() - this was causing test failures, investigate before un-commenting |
There was a problem hiding this comment.
Should we just remove these at this point?
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func Test_token(t *testing.T) { |
There was a problem hiding this comment.
This isn't used anywhere- should this be removed?
3 participants
Description
Vault credential stores have an internal job that periodically renews Vault tokens. If the renewal call returns a
403error, the system assumes the token is no longer valid and marks it as expired.When Vault is unreachable, the system logs an error and continues trying to renew the token past its expiration time since Vault never returns a 403 error. This change checks if the token has already expired; if so, it will mark the token as expired even when communication with Vault returns an error.
PCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.