-
Notifications
You must be signed in to change notification settings - Fork 406
Credential system refactor: unified context, canonical matrix, eliminate dead code #5748
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 10 commits
ea565a8
169aee8
406134b
d09d962
bd17fea
eeec027
4f83a3c
ddfeee7
b82cc3b
f44e690
73ae1db
0eb9dda
cff4860
a008344
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 |
|---|---|---|
|
|
@@ -48,149 +48,111 @@ public CertificateAndClaimsClientCredential( | |
| Certificate = certificate; | ||
| } | ||
|
|
||
| public async Task<ClientCredentialApplicationResult> AddConfidentialClientParametersAsync( | ||
| OAuth2Client oAuth2Client, | ||
| AuthenticationRequestParameters requestParameters, | ||
| ICryptographyManager cryptographyManager, | ||
| string tokenEndpoint, | ||
| public async Task<CredentialMaterial> GetCredentialMaterialAsync( | ||
| CredentialRequestContext requestContext, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| string clientId = requestParameters.AppConfig.ClientId; | ||
|
|
||
| // Log the incoming request parameters for diagnostic purposes | ||
| requestParameters.RequestContext.Logger.Verbose( | ||
| () => $"Building assertion from certificate with clientId: {clientId} at endpoint: {tokenEndpoint}"); | ||
|
|
||
| // If mTLS cert is not already set for the request, proceed with JWT bearer client assertion. | ||
| if (requestParameters.MtlsCertificate == null) | ||
| // Resolve the certificate via the provider | ||
| var opts = new AssertionRequestOptions | ||
| { | ||
| requestParameters.RequestContext.Logger.Verbose( | ||
| () => "Proceeding with JWT token creation and adding client assertion."); | ||
|
|
||
| // Resolve the certificate via the provider | ||
| X509Certificate2 certificate = | ||
| await ResolveCertificateAsync(requestParameters, tokenEndpoint, cancellationToken) | ||
| .ConfigureAwait(false); | ||
|
|
||
| // Store the resolved certificate in request parameters for later use (e.g., ExecutionResult) | ||
| requestParameters.ResolvedCertificate = certificate; | ||
|
|
||
| bool useSha2 = requestParameters.AuthorityManager.Authority.AuthorityInfo.IsSha2CredentialSupported; | ||
|
|
||
| JsonWebToken jwtToken; | ||
| if (string.IsNullOrEmpty(requestParameters.ExtraClientAssertionClaims)) | ||
| { | ||
| jwtToken = new JsonWebToken( | ||
| cryptographyManager, | ||
| clientId, | ||
| tokenEndpoint, | ||
| _claimsToSign, | ||
| _appendDefaultClaims); | ||
| } | ||
| else | ||
| { | ||
| jwtToken = new JsonWebToken( | ||
| cryptographyManager, | ||
| clientId, | ||
| tokenEndpoint, | ||
| requestParameters.ExtraClientAssertionClaims, | ||
| _appendDefaultClaims); | ||
| } | ||
|
|
||
| string assertion = jwtToken.Sign(certificate, requestParameters.SendX5C, useSha2); | ||
|
|
||
| oAuth2Client.AddBodyParameter(OAuth2Parameter.ClientAssertionType, OAuth2AssertionType.JwtBearer); | ||
| oAuth2Client.AddBodyParameter(OAuth2Parameter.ClientAssertion, assertion); | ||
|
|
||
| // No extra outputs for the common case. | ||
| return ClientCredentialApplicationResult.None; | ||
| } | ||
|
|
||
| // mTLS path: a certificate is already set on the request (e.g., mTLS/PoP transport). | ||
| requestParameters.RequestContext.Logger.Verbose( | ||
| () => "mTLS certificate is set for this request. Skipping JWT client assertion generation."); | ||
|
|
||
| requestParameters.ResolvedCertificate = requestParameters.MtlsCertificate; | ||
|
|
||
| // Return the mTLS certificate via the result object so the pipeline can use it | ||
| // (HTTP handler + policy/region checks). | ||
| return new ClientCredentialApplicationResult | ||
| { | ||
| MtlsCertificate = requestParameters.MtlsCertificate, | ||
| UseJwtPopClientAssertion = false // no client assertion set here | ||
| CancellationToken = cancellationToken, | ||
| ClientID = requestContext.ClientId, | ||
| TokenEndpoint = requestContext.TokenEndpoint, | ||
| ClientCapabilities = requestContext.ClientCapabilities, | ||
| Claims = requestContext.Claims | ||
| }; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Resolves the certificate to use for signing the client assertion. | ||
| /// Invokes the certificate provider delegate to get the certificate. | ||
| /// </summary> | ||
| /// <param name="requestParameters">The authentication request parameters containing app config</param> | ||
| /// <param name="tokenEndpoint">The token endpoint URL</param> | ||
| /// <param name="cancellationToken">Cancellation token for the async operation</param> | ||
| /// <returns>The X509Certificate2 to use for signing</returns> | ||
| /// <exception cref="MsalClientException">Thrown if the certificate provider returns null or an invalid certificate</exception> | ||
| private async Task<X509Certificate2> ResolveCertificateAsync( | ||
| AuthenticationRequestParameters requestParameters, | ||
| string tokenEndpoint, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| requestParameters.RequestContext.Logger.Verbose( | ||
| () => "[CertificateAndClaimsClientCredential] Resolving certificate from provider."); | ||
| X509Certificate2 cert = await _certificateProvider(opts).ConfigureAwait(false); | ||
|
|
||
| // Create AssertionRequestOptions for the callback | ||
| var options = new AssertionRequestOptions( | ||
| requestParameters.AppConfig, | ||
| tokenEndpoint, | ||
| requestParameters.AuthorityManager.Authority.TenantId) | ||
| if (cert == null) | ||
| { | ||
| Claims = requestParameters.Claims, | ||
| ClientCapabilities = requestParameters.AppConfig.ClientCapabilities, | ||
| CancellationToken = cancellationToken | ||
| }; | ||
|
|
||
| // Invoke the provider to get the certificate | ||
| X509Certificate2 certificate = await _certificateProvider(options).ConfigureAwait(false); | ||
|
|
||
| // Validate the certificate returned by the provider | ||
| if (certificate == null) | ||
| { | ||
| requestParameters.RequestContext.Logger.Error( | ||
| "[CertificateAndClaimsClientCredential] Certificate provider returned null."); | ||
|
|
||
| throw new MsalClientException( | ||
| MsalError.InvalidClientAssertion, | ||
| "The certificate provider callback returned null. Ensure the callback returns a valid X509Certificate2 instance."); | ||
| } | ||
|
|
||
| // Validate certificate has private key (may throw CryptographicException if cert is disposed) | ||
| try | ||
| { | ||
| if (!certificate.HasPrivateKey) | ||
| if (!cert.HasPrivateKey) | ||
| { | ||
| requestParameters.RequestContext.Logger.Error( | ||
| "[CertificateAndClaimsClientCredential] Certificate from provider does not have a private key."); | ||
|
|
||
| throw new MsalClientException( | ||
| MsalError.CertWithoutPrivateKey, | ||
| MsalErrorMessage.CertMustHavePrivateKey(certificate.FriendlyName)); | ||
| MsalErrorMessage.CertMustHavePrivateKey(cert.FriendlyName)); | ||
| } | ||
| } | ||
| catch (System.Security.Cryptography.CryptographicException ex) | ||
| { | ||
| requestParameters.RequestContext.Logger.Error( | ||
| "[CertificateAndClaimsClientCredential] A cryptographic error occurred while accessing the certificate."); | ||
| throw new MsalClientException( | ||
| MsalError.CryptographicError, | ||
| MsalErrorMessage.CryptographicError, | ||
| ex); | ||
| } | ||
|
|
||
| // If in mTLS bearer mode, skip JWT assertion - certificate will be used for TLS client auth only | ||
| if (requestContext.MtlsBearerMode) | ||
|
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. What is the meaning of MtlsBearerMode?
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. Which endpoint is used in this case? Mtls?
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. |
||
| { | ||
| return new CredentialMaterial( | ||
| tokenRequestParameters: new Dictionary<string, string>(), // Empty - no client_assertion | ||
| mtlsCertificate: cert, | ||
| metadata: new CredentialMaterialMetadata( | ||
| credentialType: CredentialType.ClientCertificate, | ||
| credentialSource: Certificate == null ? "dynamic" : "static", | ||
| mtlsCertificateIdHashPrefix: CredentialMaterialHelper.GetCertificateIdHashPrefix(cert), | ||
| mtlsCertificateRequested: requestContext.MtlsRequired, | ||
| resolutionTimeMs: 0)); | ||
| } | ||
|
|
||
| // Build JWT assertion | ||
| JsonWebToken jwtToken; | ||
| if (!string.IsNullOrEmpty(requestContext.ExtraClientAssertionClaims)) | ||
| { | ||
| // ExtraClientAssertionClaims takes precedence (e.g., for cache key binding) | ||
| jwtToken = new JsonWebToken( | ||
| requestContext.CryptographyManager, | ||
| requestContext.ClientId, | ||
| requestContext.TokenEndpoint, | ||
| requestContext.ExtraClientAssertionClaims, | ||
| _appendDefaultClaims); | ||
| } | ||
| else | ||
| { | ||
| jwtToken = new JsonWebToken( | ||
| requestContext.CryptographyManager, | ||
| requestContext.ClientId, | ||
| requestContext.TokenEndpoint, | ||
| _claimsToSign, | ||
| _appendDefaultClaims); | ||
| } | ||
|
|
||
| string assertion; | ||
| try | ||
| { | ||
| assertion = jwtToken.Sign(cert, requestContext.SendX5C, requestContext.UseSha2); | ||
| } | ||
| catch (System.Security.Cryptography.CryptographicException ex) | ||
| { | ||
| throw new MsalClientException( | ||
| MsalError.CryptographicError, | ||
| MsalErrorMessage.CryptographicError, | ||
| ex); | ||
| } | ||
|
|
||
| requestParameters.RequestContext.Logger.Info( | ||
|
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. We need this log for logging if the cert was resolved. Can you add this back when the cert is resolved?
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. addressed in #5835 |
||
| () => $"[CertificateAndClaimsClientCredential] Successfully resolved certificate from provider. " + | ||
| $"Thumbprint: {certificate.Thumbprint}"); | ||
| var tokenParameters = new Dictionary<string, string> | ||
| { | ||
| { OAuth2Parameter.ClientAssertionType, OAuth2AssertionType.JwtBearer }, | ||
| { OAuth2Parameter.ClientAssertion, assertion } | ||
| }; | ||
|
|
||
| return certificate; | ||
| return new CredentialMaterial( | ||
| tokenRequestParameters: tokenParameters, | ||
| mtlsCertificate: cert, | ||
| metadata: new CredentialMaterialMetadata( | ||
| credentialType: CredentialType.ClientCertificate, | ||
| credentialSource: Certificate == null ? "dynamic" : "static", | ||
| mtlsCertificateIdHashPrefix: CredentialMaterialHelper.GetCertificateIdHashPrefix(cert), | ||
| mtlsCertificateRequested: requestContext.MtlsRequired, | ||
| resolutionTimeMs: 0)); | ||
| } | ||
| } | ||
| } | ||
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.
Nit: its a bit confusing name
requestContextMaybe renaming it to credentialContext brings clarityThere 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.
+1, not a nit
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.
Carried forward in #5835; CredentialRequestContext is gone and CredentialContext is the active type.