Refactor to include mtls with managed cert#5692
Conversation
…AzureAD/microsoft-authentication-library-for-dotnet into nebharg/managedCertMtlsRefactor
| /// <summary> | ||
| /// Shared validation utilities for client certificate scenarios. | ||
| /// </summary> | ||
| internal static class ClientCertificateValidation |
There was a problem hiding this comment.
It seems a bit overkill to have a full class for a helper (static) method. Consider adding to ClientCertificateOrchestrator
| /// </exception> | ||
| public static async Task<ClientCertificateContext> ResolveCertificateAsync( | ||
| IClientCredential credential, | ||
| IServiceBundle serviceBundle, |
There was a problem hiding this comment.
IServiceBundle is a "god" object. Ideally we should not pass it around. Maybe the Config is enough?
| /// Indicates whether MTLS Proof-of-Possession is requested for this authentication operation. | ||
| /// When true, the certificate (if provided) will be used for TLS binding rather than just JWT signing. | ||
| /// </summary> | ||
| public bool IsMtlsPopRequested { get; set; } |
There was a problem hiding this comment.
I don't think consumers of AssertionRequestOptions (e.g. OneCert) need to know this?
| /// <summary> | ||
| /// Describes how a client certificate is used in the authentication flow. | ||
| /// </summary> | ||
| internal enum ClientCertificateUsage |
There was a problem hiding this comment.
I wonder if we can simplify a bit more here, to decouple the Credential from the usage?
Credentials in OAuth are of 2 types: JWT (FIC, client_assertion) and X509Certificate2 (for mTLS). MSAL decides if to request the token over mTLS (in which case it MUST have access to X509Certificate2 obj) or over regular HTTPS.
There are 4 credentials:
- secrets (extra POST param)
- certificates (extra POST param or mTLS)
- FIC - regular and bound ... in case of regular FIC, this is extra POST param only. But in case of bound FICs, the credential is a combination of a { jwt, x509certificate}. So ... bearer FIC - extra POST param. Bound FIC - mTLS with custom POST params
So maybe the IClientCredential interface is really
internal interface IClientCredential
{
Task<CredentialMaterial> GetCredentialMaterialAsync(bool mTLSRequired)
}
internal class CredentialMaterial
{
public X509Certificate2 MtlsCertificate { get; }
public IDictionary<string, string> TokenRequestParameters { get; }
}There was a problem hiding this comment.
We can also have a dedicated WithBoundClientAssertion(lambda) @gladjohn if it makes the API cleaner.
There was a problem hiding this comment.
agreed,
-
Single Responsibility: Concrete credential classes only know "what credentials I can provide" not "how or when they're used"
-
Inspection Without Implementation Knowledge: The caller inspects
CredentialMaterial(via the getter) to decide if it has mTLS cert or what parameters to use, but the credential class itself has zero knowledge of mTLS context or usage intent.
| certContext = await ClientCertificateOrchestrator.ResolveCertificateAsync( | ||
| ServiceBundle.Config.ClientCredential, |
There was a problem hiding this comment.
This is where the orchestrator is called CC: @gladjohn
|
Closing as the work was duplicated with some tech debt in PR #5670 |
3 participants
Fixes #5608
Changes proposed in this request
Centralize and simplify certificate resolution for MTLS Proof-of-Possession while maintaining backward compatibility and existing validation behavior.
Refactor the mtls flow and include mtls with cert provider as well
Testing
Performance impact
Documentation