-
Notifications
You must be signed in to change notification settings - Fork 245
Fix certificate reload logic to only trigger on certificate-specific errors #3653
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
base: master
Are you sure you want to change the base?
Conversation
…rrors Co-authored-by: jmprieur <[email protected]>
Co-authored-by: jmprieur <[email protected]>
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 have agreement from eSTS that the error codes are stable? Afaik, error codes are not part of the public contract between SDK and service. The existing logic is safer.
| internal const string SignedAssertionInvalidTimeRange = "AADSTS700024"; | ||
| internal const string CertificateHasBeenRevoked = "AADSTS7000214"; | ||
| internal const string CertificateIsOutsideValidityWindow = "AADSTS1000502"; | ||
| internal const string CertificateNotWithinValidityPeriod = "AADSTS7000274"; |
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.
I believe that this is "Client assertion contains an invalid signature", not CertificateNotWithinValidityPeriod. This makes sense as right now this is the same as CertificateIsOutsideValidityWindow which is right above it. https://stackoverflow.com/questions/62716173/jwt-token-error-aadsts700027-client-assertion-contains-an-invalid-signature
| responseBody.Contains(Constants.CertificateNotWithinValidityPeriod, StringComparison.OrdinalIgnoreCase) || | ||
| responseBody.Contains(Constants.CertificateWasRevoked, StringComparison.OrdinalIgnoreCase); | ||
| #else | ||
| return responseBody.Contains(Constants.InvalidKeyError) || |
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.
This seems error prone, if we add a new error code we need to remember to put it in multiple places. Perhaps a dictionary should be created above and referred to?
bgavrilMS
left a comment
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.
I recommend fixing the infinite loop instead. Retrying the cert once on a generic error seems ok.
|
Not instead. In addition: We got customer complains for retrying for a client secret. |
|
@tlupes - is this change is compatible with the new credential? I am ok to sign off if this is true. |
Fix certificate reload logic to only trigger on certificate-specific errors
Fix certificate reload triggering on all invalid_client errors
Description
PR #3430 broadened certificate reload logic to trigger on all
invalid_clienterrors except AADSTS7000215, causing infinite loops with Agent identities and unnecessary reloads for non-certificate authentication failures (wrong client secret, wrong client ID, etc.).Core Fix
Changed
IsInvalidClientCertificateOrSignedAssertionErrorfrom blacklist approach (exclude AADSTS7000215) to whitelist approach (only specific certificate error codes):Now triggers reload only for:
No longer triggers reload for:
invalid_clienterrorsChanges
CertificateNotWithinValidityPeriodandCertificateWasRevokedconstantsExample
This prevents infinite loops in Agent identity scenarios and eliminates unnecessary reload attempts for non-certificate authentication failures.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.