Make sure the token's claims are match as expected. #3846
Conversation
There was a problem hiding this comment.
Pull Request Overview
Ensures OAuth token claims match expected values during authentication migrations.
- Introduces a background task (
CheckOAuthTokenClaimsAsync) to periodically validate new OAuth v2 token claims against baseline tokens. - Adds synchronization primitives (locks, token sources) and cancellation of the claims check task alongside telemetry.
- Logs mismatches and defers migration when claim values diverge.
Comments suppressed due to low confidence (1)
src/Runner.Listener/Runner.cs:776
- Consider adding unit or integration tests to cover
CheckOAuthTokenClaimsAsync, including scenarios for matching and mismatched claims.
private async Task CheckOAuthTokenClaimsAsync(CancellationToken token)
0bb0bef to
8054699
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if (v2Claim?.Value != baselineClaim.Value) | ||
| { | ||
| Trace.Info($"Token Claim mismatch between two issuers. Expected: {baselineClaim.Type}:{baselineClaim.Value}. Actual: {v2Claim?.Type ?? "Empty"}:{v2Claim?.Value ?? "Empty"}"); | ||
| HostContext.DeferAuthMigration(TimeSpan.FromMinutes(60), $"Expected claim {baselineClaim.Type}:{baselineClaim.Value} does not match {v2Claim?.Type ?? "Empty"}:{v2Claim?.Value ?? "Empty"}"); |
There was a problem hiding this comment.
Will this trigger any telemetry? Wondering how we know whether this is happening.
There was a problem hiding this comment.
I noticed the while loop sleeps for 100m yet this for loop defers 60m. Is that discrepancy OK?
There was a problem hiding this comment.
Any DeferAuthMigration() should publish telemetry back to service.
There was a problem hiding this comment.
it's on purposely to not lines up so different things won't trigger at the same time.
| { | ||
| try | ||
| { | ||
| await HostContext.Delay(TimeSpan.FromMinutes(100), token); |
There was a problem hiding this comment.
Should this only delay after the first iteration? Or first successful iteration? Or does it matter?
There was a problem hiding this comment.
i don't want the runner to get multiple tokens during start up, try to avoid too much load on the service.
| { | ||
| try | ||
| { | ||
| await HostContext.Delay(TimeSpan.FromMinutes(100), token); |
There was a problem hiding this comment.
Does it matter than an intermittent network IO error may cause the logic further below not be skipped for more than multiple 100m delays?
There was a problem hiding this comment.
We should not see any claim diff from the token.
We will use Ring 0 to fully test this out and make sure we never see any failure.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
https://github.com/github/actions-fusion/issues/2436