VAULT-25341 Address issue where having no permissions to renew caused auto-auth to attempt to renew with no backoff #26844
Conversation
… Agent and Proxy auth to attempt to renew with no backoff
github-actions
bot
added
the
hashicorp-contributed-pr
| @@ -478,10 +479,9 @@ func (ah *AuthHandler) Run(ctx context.Context, am AuthMethod) error { | |||
| } | |||
|
|
|||
| metrics.IncrCounter([]string{ah.metricsSignifier, "auth", "success"}, 1) | |||
| // We don't want to trigger the renewal process for tokens with | |||
| // unlimited TTL, such as the root token. | |||
| if leaseDuration == 0 && isTokenFileMethod { | |||
There was a problem hiding this comment.
Previously, this was a bug, and could occur for non-renewable tokens that happened to check it as their lease ticked down, causing re-authentication to not happen.
There was a problem hiding this comment.
Do we want to keep a check doesn't trigger lifetime watcher if the secret.Renewable == false?
There was a problem hiding this comment.
If the secret's not renewable, then the LifetimeWatcher still needs to get triggered given the current architecture, so that auth gets re-triggered at the correct time. If we need to authenticate again for any reason, we should run the lifetimewatcher -- the root token is the only token where we don't need to reauthenticate (since it never expires)
e.g. for a 10s TTL non-renewable token, it might look like this
- LifetimeWatcher triggered
- It waits for ~8.5 seconds, does not try to renew
- Triggers the done channel with no error
- Auth gets re-triggered
We're essentially making renewable tokens without the permission to renew follow the same path as the above ^
|
CI Results: |
| // so that we don't go into a loop, as the LifetimeWatcher will immediately | ||
| // return for tokens like this. | ||
| if leaseDuration == 0 { | ||
| time.Sleep(1 * time.Second) |
There was a problem hiding this comment.
Without this sleep, we'd reauthenticate then receive something on the DoneCh immediately a few times. It's a bit of an edge case but it seemed important. I wanted to avoid using backoffSleep since that's intended to be for errors only.
|
Build Results: |
| // Prior to the fix made in the same PR this test was added, it would trigger many, many | ||
| // retries (hundreds to thousands in less than a minute). | ||
| // We really want to make sure that doesn't happen. | ||
| numberOfRenewSelves := strings.Count(stringAudit, "auth/token/renew-self") |
Co-authored-by: divyaac <divya.chandrasekaran@hashicorp.com>
The issue was twofold:
Both issues should be resolved by this PR.
Closes #22575