Fix Certificate Reload Infinite Recursion Bug #3673
Conversation
…r to only retry for certificate errors
| , StringComparison.OrdinalIgnoreCase | ||
| return retryCount < MaxCertificateRetryAttempts && | ||
| #if !NETSTANDARD2_0 && !NET462 && !NET472 | ||
| (exMsal.Message.Contains(Constants.InvalidKeyError, StringComparison.OrdinalIgnoreCase) |
There was a problem hiding this comment.
Can we have a dictionary of these error codes to avoid the if / else and duplication?
There was a problem hiding this comment.
You should merge this one first: #3653, @bgavrilMS , @4gust
| /// This prevents infinite retry loops. | ||
| /// </summary> | ||
| [Fact] | ||
| public void IsInvalidClientCertificateError_RetryCountAtMax_ReturnsFalse() |
There was a problem hiding this comment.
Seems like a lot of these tests can consolidated into 1
There was a problem hiding this comment.
The only test test really matters would be the e2e with agentid and wrong cred, same with agent user identity
There was a problem hiding this comment.
Just to update, @4gust and I looked at the Agentic tests, and when a bad credential is used, the tests indeed have infinite recursion.
| public async Task<AcquireTokenResult> AddAccountToCacheFromAuthorizationCodeAsync( | ||
| AuthCodeRedemptionParameters authCodeRedemptionParameters) | ||
| { | ||
| return await AddAccountToCacheFromAuthorizationCodeAsync(authCodeRedemptionParameters, retryCount: 0).ConfigureAwait(false); |
There was a problem hiding this comment.
The auth code flow is not used by agent identities ...
There was a problem hiding this comment.
But the retry logic applies to all flows. GetAuthenticationResultForUserAsync is also covered and that calls ROPC / OBO or AuthCode. ROPC is used.
Fix Certificate Reload Infinite Recursion Bug
Description
Certificate reload logic was triggering for all invalid_client errors, not just certificate-related ones. This caused unnecessary retries for unrelated authentication failures like wrong passwords or missing app registrations.
Additionally, the shared _retryClientCertificate boolean flag had thread-safety issues in concurrent scenarios.
Solution
Restored specific error checking - Only reload certificates for actual cert errors:
• AADSTS700027 - Invalid key
• AADSTS700024 - Invalid time range
• AADSTS7000214 - Certificate revoked
• AADSTS1000502 - Certificate expired Replaced shared flag with per-call counter - Each call tracks its own retry count (max 1 retry),
preventing infinite loops and other conditions.
Fixes issues :
#3653 and #3654
Solution
• Removed:
private bool _retryClientCertificate(shared state)• Added:
private const int MaxCertificateRetryAttempts = 1• Added
int retryCountparameter to private overloads of:AddAccountToCacheFromAuthorizationCodeAsync(AuthCodeRedemptionParameters)andGetAuthenticationResultForUserAsync(IEnumerable<string>, string?, string?, string?, ClaimsPrincipal?, TokenAcquisitionOptions?)• GetAuthenticationResultForAppAsync(string, string?, string?, TokenAcquisitionOptions?)
• Updated
IsInvalidClientCertificateOrSignedAssertionError(MsalServiceException, int)to check retryCount < MaxCertificateRetryAttemptsOthers