-
Notifications
You must be signed in to change notification settings - Fork 2k
MWI: Fall back to registering without an existing auth client #56927
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
Changes from all commits
0398efc
3c9c40b
1bedfa5
4c1f268
714f54e
2b4bee4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -439,6 +439,24 @@ func (s *identityService) unblockWaiters() { | |
| s.initializedOnce.Do(func() { close(s.initialized) }) | ||
| } | ||
|
|
||
| // renewIdentity attempts to renew an existing bot identity. "Renewal" in this | ||
| // case means one of two things: | ||
| // | ||
| // 1. If using an explicitly renewable identity (i.e. `token` joining), | ||
| // certificates will be renewed directly via Auth using the formal renewal | ||
| // process. | ||
| // | ||
| // If the existing identity is expired, this will fail and cannot be | ||
| // recovered. | ||
| // | ||
| // 2. For all other join methods, a "lightweight renewal" is performed. The | ||
| // existing client is used to authenticate the request and prove ownership | ||
| // of the existing bot instance ID, but otherwise the delegated joining | ||
| // ceremony is performed as usual. | ||
| // | ||
| // If the existing identity appears to be expired (`time.Now()` > | ||
| // `NotAfter`), the existing auth client will be discarded and the bot will | ||
| // try to join without it. This will result in a new bot instance ID. | ||
| func renewIdentity( | ||
| ctx context.Context, | ||
| log *slog.Logger, | ||
|
|
@@ -470,9 +488,38 @@ func renewIdentity( | |
| return newIdentity, nil | ||
| } | ||
|
|
||
| // Note: This simple expiration check is probably not the best possible | ||
| // solution to determine when to discard an existing identity: the client | ||
| // could have severe clock drift, or there could be non-expiry related | ||
| // reasons that an identity should be thrown out. We may improve this | ||
| // discard logic in the future if we determine we're still creating excess | ||
| // bot instances. | ||
| now := time.Now() | ||
| if expiry, ok := facade.Expiry(); !ok || now.After(expiry) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My only thought with client-side checks of a certificates validity is that the certificate /could/ be valid but the client could be heavily drifted, but, I think in that case we'd probably want to encourage people to fix the drift rather than adjusting the product to accomodate it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From our 1x1 today - This client-side check is definitely not the best possible solution but is relatively simple and shouldn't cause a meaningful jump in bot instance cycling - in practice it only restores previous behavior, since bots would crash, restart, and discard their identity with this same check at startup. I've added some more detail to the warning message to call out potential clock drift along with a note that we might iterate on this more in the future. (And also tweaked wording to emphasize that its only a Problem if it happens repeatedly) |
||
| slog.WarnContext( | ||
| ctx, | ||
| "The bot identity appears to be expired and will not be used to "+ | ||
| "authenticate the identity renewal. If it is possible to "+ | ||
| "rejoin, a new bot instance will be created. If this occurs "+ | ||
| "repeatedly, ensure the local machine's clock is properly "+ | ||
| "synchronized, the certificate TTL is adjusted to your "+ | ||
| "environment, and that no external issues will prevent "+ | ||
| "the bot from renewing its identity on schedule.", | ||
| "now", now, | ||
| "expiry", expiry, | ||
| "credential_lifetime", botCfg.CredentialLifetime, | ||
| ) | ||
|
|
||
| newIdentity, err := botIdentityFromToken(ctx, log, botCfg, nil) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err, "renewing identity using Register without existing auth client") | ||
| } | ||
| return newIdentity, nil | ||
| } | ||
|
|
||
| newIdentity, err := botIdentityFromToken(ctx, log, botCfg, authClient) | ||
| if err != nil { | ||
| return nil, trace.Wrap(err, "renewing identity using Register") | ||
| return nil, trace.Wrap(err, "renewing identity using Register with existing auth client") | ||
| } | ||
| return newIdentity, nil | ||
| } | ||
|
|
||
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.
Is this comment about the new bot instance ID still accurate?
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.
Yes, a new ID is issued whenever a bot joins without an existing valid cert bundle.