Skip to content
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

Introduces a lock while loading credentials from Credential Source #2438

Merged
merged 6 commits into from
Sep 7, 2023

Conversation

sruke
Copy link
Contributor

@sruke sruke commented Sep 5, 2023

Introduces a lock while loading credentials from Credential Source

Summary of the changes
Ensure multiple requests don't attempt to load the same credentialDescription concurrently.

Description

When a customer configures the source of a token decryption certificate in the inbound policy, SAL attempts to load the certificate from the specified source or retrieves the cached certificate if it has been loaded previously. SAL then creates an X509SecurityKey from the certificate and adds it to InboundPolicy.TokenValidationParameters.TokenDecryptionKeys if the key was not already added. This logic is executed for every incoming token validation request, and SAL utilizes ID.Web's implementation of ICredentialLoader to retrieve the certificate.

Until the certificate is cached by ID.Web (i.e., when credentialDescription.CachedValue is set), multiple parallel requests (potentially exceeding 100 incoming requests, as indicated by Keegan) may attempt to load the same certificate from the source. This raises performance concerns, particularly when fetching the certificate involves making an HTTP request (e.g., fetching from KeyVault).

Open Questions:

  1. How does ID.Web detect when a certificate has been rotated?
  2. Is the ResetCredentials API invoked when a certificate rotation occurs?

Note: Unit tests will be added once the design is approved

@sruke sruke marked this pull request as draft September 5, 2023 17:05
@sruke sruke self-assigned this Sep 5, 2023
@jmprieur
Copy link
Collaborator

jmprieur commented Sep 5, 2023

@sruke:

How does ID.Web detect when a certificate has been rotated?

IdWeb detects when client certificates were rotated because AAD returns an explicit error

Is the ResetCredentials API invoked when a certificate rotation occurs?

On this error, IDWeb calls ResetCredentials, and retrys the call, which triggers reloading the certificate. This happens here:

catch (MsalServiceException exMsal) when (IsInvalidClientCertificateOrSignedAssertionError(exMsal))
{
DefaultCertificateLoader.ResetCertificates(mergedOptions.ClientCertificates);
_applicationsByAuthorityClientId[GetApplicationKey(mergedOptions)] = null;
// Retry
_retryClientCertificate = true;

IdWeb does not do anything with decrypt certificate, as the guidance (with IdWeb) is for service owners to provide several decyrpt credentials, and Wilson tries them all.

@sruke sruke marked this pull request as ready for review September 7, 2023 20:13
Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks @sruke

I proposed a small tweak.

Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
thanks @sruke

I proposed a small tweak

@sruke sruke force-pushed the sruthi/LoadingLock branch from c4545e5 to a0ec166 Compare September 7, 2023 21:05
@sruke sruke merged commit 050e3e0 into master Sep 7, 2023
4 checks passed
@jennyf19 jennyf19 deleted the sruthi/LoadingLock branch September 7, 2023 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Introduce a lock while loading credentials from Credential Source
6 participants