-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
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
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep a check doesn't trigger lifetime watcher if the secret.Renewable == false?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") |
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.
Cool strategy!
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.
Looks good to me! Small comments.
Co-authored-by: divyaac <[email protected]>
The issue was twofold:
Both issues should be resolved by this PR.
Closes #22575