Add SN/I certificate support over mTLS Proof-of-Possession (PoP) - #1040
Add SN/I certificate support over mTLS Proof-of-Possession (PoP)#1040Robbie-Microsoft wants to merge 17 commits into
Conversation
Allow a confidential-client app configured with a Subject-Name/Issuer (SN/I) certificate to obtain an mTLS-bound PoP access token from Entra ID by presenting that same certificate as the client TLS certificate in the mutual-TLS handshake to the token endpoint, instead of signing a private_key_jwt (x5c) client assertion.
Opt in via ClientCredentialParameters.builder(...).mtlsProofOfPossession(). Adds TokenType, BindingCertificate, MtlsClientCertificateHelper, and MtlsEndpointHelper; threads the mTLS socket factory through the HTTP layer; derives the mtlsauth.* endpoint (region optional); builds the mtls_pop request (direct cert -> no assertion; FIC Leg 2 -> jwt-pop with mtlsBindingCertificate); parses token_type and surfaces the public binding certificate; and isolates the cache on {token_type + cert KeyId}. Also covers the 2-leg FIC over mTLS PoP where both legs are cert-bound.
The existing SNI+Bearer (assertion) and broker SHR-PoP flows are unchanged. Includes unit tests, an E2E integration test (MtlsPopIT), a sample, and docs/changelog updates.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Per review feedback, FIC Leg 2 (binding an assertion-authenticated app to a certificate via mtlsBindingCertificate + client_assertion_type=jwt-pop) is moved to a separate stacked follow-up PR. This PR retains the core direct SN/I -> mTLS PoP path and FIC Leg 1. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds confidential-client support for acquiring mTLS Proof-of-Possession (PoP) tokens using an SN/I certificate presented on the TLS handshake (instead of signing a private_key_jwt), and surfaces token-binding metadata to callers.
Changes:
- Introduces new public result metadata (
TokenType,BindingCertificate,AuthenticationResultMetadata.tokenType()/bindingCertificate()). - Implements mTLS PoP request execution:
login.*→mtlsauth.*endpoint rewrite, mTLS socket factory transport injection, and omission ofclient_assertionfor direct certificate auth. - Adds cache-isolation dimensions and unit/integration tests, plus docs/sample updates.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AcquireTokenByClientCredentialSupplier.java | Stamps cert KeyId for cache isolation when mTLS PoP is requested. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationErrorCode.java | Adds MTLS_POP_ERROR for mTLS PoP configuration/runtime failures. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthenticationResultMetadata.java | Adds token type + binding certificate fields and builder support. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/BindingCertificate.java | New public type exposing x5c chain + x5t#S256 (public material only). |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ClientCredentialParameters.java | Adds mtlsProofOfPossession() opt-in and cache-key component support. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ClientCredentialRequest.java | Adds token_type=mtls_pop parameter when mTLS PoP is requested. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java | Exposes request-level HTTP execution overload used by mTLS path. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IHttpHelper.java | Adds overload to execute requests with an explicit HTTP client + telemetry. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/MtlsClientCertificateHelper.java | Builds mTLS socket factory and derives binding-certificate public metadata. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/MtlsEndpointHelper.java | Rewrites standard token endpoint host to mtlsauth.* and validates tenant/cloud. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/OAuthHttpRequest.java | Routes token requests through an injected mTLS HTTP client when configured. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenRequestExecutor.java | Creates mTLS transport/endpoint and populates result metadata for mTLS PoP. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenResponse.java | Captures token_type from token responses. |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/TokenType.java | New public enum for BEARER vs MTLS_POP token types. |
| msal4j-sdk/src/integrationtest/java/com/microsoft/aad/msal4j/MtlsPopIT.java | CI-only E2E validation for direct SNI→mTLS PoP and cache isolation. |
| msal4j-sdk/src/samples/confidential-client/ClientCredentialMtlsProofOfPossession.java | New sample showing direct SN/I cert → mTLS PoP token acquisition. |
| msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/MtlsClientCertificateHelperTest.java | Unit tests for socket factory, thumbprint computation, and binding cert behavior. |
| msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/MtlsEndpointHelperTest.java | Unit tests for endpoint derivation and cloud/tenant validation. |
| msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/MtlsProofOfPossessionTest.java | Unit tests verifying mTLS request shaping and cache-key isolation behavior. |
| changelog.txt | Documents the new mTLS PoP SN/I confidential-client support. |
| .github/copilot-instructions.md | Updates repo guidance to include the new client-credentials mTLS PoP flow. |
The Unit Tests build's Credential Scanner (Guardian) gate flags the checked-in PKCS12 test cert (mtls_test_cert.p12), failing the build. It is a self-signed, test-only certificate (CN=msal4j-mtls-test) with no production secret, so allowlist it in build/credscan-exclude.json. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- MtlsClientCertificateHelper: use a non-empty transient in-memory keystore password (JDK 8 PKCS12 setKeyEntry throws 'Key protection algorithm not found' NPE with an empty password) and guard against a null private key. Fixes the MtlsPopIT failures seen only in CI (JDK 8). - ClientCredentialParameters: make the cache-key fields volatile and rewrite bindingCertificateKeyId as synchronized copy-on-write so a reused params instance can't observe a partially-updated map/hash; correct the stale 'computed once / immutable' comments. - copilot-instructions.md: split the inline '- Key Classes' segment into its own nested bullet so it renders as a list item. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Pulls in the mTLS PoP socket-factory JDK 8 fix and the ClientCredentialParameters thread-safety/comment fixes from #1040. Resolved copilot-instructions.md to keep the FIC Leg 2 sentence alongside the Key Classes markdown split. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…portable keys) createMtlsSocketFactory imported the certificate's private key into an in-memory PKCS12 KeyStore via setKeyEntry. That serializes the key (PrivateKey.getEncoded()), which returns null for non-exportable OS/HSM key handles. On the Windows CI agent the lab cert is loaded from Windows-MY via SunMSCAPI, so all three MtlsPopIT tests failed with 'Key protection algorithm not found: java.lang.NullPointerException'. The earlier password change was a misdiagnosis; the password is irrelevant to this failure. Present the certificate through a custom X509ExtendedKeyManager that holds the live PrivateKey and chain, so the TLS handshake signs through the key's own provider (as JWT client-assertion signing already does). Works for both non-exportable and ordinary exportable keys. Add a regression unit test using a private key whose getEncoded()/getFormat() return null. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ESTS only issues mTLS PoP tokens for the SN/I-allow-listed app in the MSI team tenant. MtlsPopIT authenticated as LabVaultAppID against microsoft.onmicrosoft.com, which is valid for Bearer client-credentials but not allow-listed for mTLS PoP, so ESTS rejected the PoP requests with AADSTS700025 (Client is public). Mirror MSAL .NET's ClientCredentialsMtlsPopTests: use the SN/I-allow-listed app (163ffef9-...), the MSI team tenant authority (bea21ebe-...), and region westus3. App/tenant IDs are public identifiers. The lab cert and Key Vault scope are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Validate the identity provider's response token_type instead of assuming MTLS_POP from the request. When an mTLS Proof-of-Possession token is requested but ESTS returns a different token_type (e.g. a Bearer downgrade), the access token is not certificate-bound; MSAL now throws MsalClientException(token_type_mismatch) rather than mislabeling an unbound token as MTLS_POP. The binding certificate is only surfaced after the token type is validated, and the result token type is derived from the validated response so Bearer flows keep reporting Bearer. Adds AuthenticationErrorCode.TOKEN_TYPE_MISMATCH, a fail-closed unit test, a token_type-overridable test mock, and a skip-on-downgrade accommodation in MtlsPopIT (mirrors MSAL .NET's ExecuteOrInconclusiveOnTokenTypeMismatchAsync since the AAD test slice is a known intermittent downgrader). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The per-constant telemetry value was never wired into telemetry emission, so drop the field, constructor parameter, and accessor. It can be reintroduced if/when token_type telemetry is actually plumbed through. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…exact token_type Fix three divergences from the MSAL.NET reference in the direct SN/I -> mTLS Proof-of-Possession path: - MtlsEndpointHelper: enforce cloud boundaries so sovereign login.* hosts fail fast (via the authoritative TRUSTED_SOVEREIGN_HOSTS_SET, including regional forms) instead of being rewritten to the public mtlsauth.microsoft.com and having the client certificate presented cross-cloud. Other login.* hosts are rewritten domain-preserving (login -> mtlsauth) and non-login hosts are rejected. - AcquireTokenByClientCredentialSupplier: restore tokenType(MTLS_POP) and the binding certificate on cache hits, which previously defaulted to BEARER/null because only the network path stamped them. - TokenType.fromString: map only the exact (case-insensitive) mtls_pop token_type to MTLS_POP, so a non-certificate-bound "pop" response is correctly rejected by the mTLS PoP fail-closed check instead of masquerading as MTLS_POP. Adds regression tests for all three (378 msal4j-sdk unit tests green). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…rent national clouds Relax the mTLS PoP sovereign-cloud guardrail to match MSAL.NET's RegionAndMtlsDiscoveryProvider. isMtlsPoPUnsupportedCloud now denies only the two legacy sovereign hosts that have no mtlsauth.* endpoint (login.usgovcloudapi.net, login.chinacloudapi.cn). Every other login.* host -- including Azure Government and the current national clouds -- is allowed and rewritten domain-preserving (login -> mtlsauth), keeping each request within its own cloud boundary. This keeps the earlier domain-preserving rewrite (the actual cross-cloud leak fix) while removing the over-broad blanket sovereign rejection. Non-login. hosts remain rejected by deriveMtlsHost. MtlsEndpointHelperTest updated: the Azure Gov / current China / German / France cases now assert domain-preserving rewrite, and two legacy hosts assert fail-fast. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
decodeCertificateChain could throw a raw IllegalArgumentException when an x5c entry was not valid base64, bypassing the MTLS_POP_ERROR surface that callers expect. Catch it and rethrow as CertificateException so buildBindingCertificate/computeCertificateKeyId/createMtlsSocketFactory all report MsalClientException(MTLS_POP_ERROR). Adds regression tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| // SN/I-allow-listed app and MSI team tenant. mTLS PoP only works on this app/tenant pair; mirrors | ||
| // MSAL .NET's ClientCredentialsMtlsPopTests. App and tenant IDs are public identifiers, not secrets. | ||
| private static final String SNI_ALLOWLISTED_APP_ID = "163ffef9-a313-45b4-ab2f-c7e2f5e0e23e"; | ||
| private static final String SNI_ALLOWLISTED_AUTHORITY = |
There was a problem hiding this comment.
we have so much reliance on this MSI team's test tenant. Can we have our own SNI test setup in IDLABS1?
| @@ -1,5 +1,6 @@ | |||
| Version 1.25.0 | |||
| ============= | |||
| - Add SN/I certificate support over mTLS Proof-of-Possession (PoP) for confidential clients, presenting the certificate as the client TLS cert to obtain an mTLS-bound token. The response token_type is validated, so a request that is not honored as mtls_pop fails closed rather than surfacing an unbound token | |||
There was a problem hiding this comment.
The changes in this PR definitely weren't released a month ago.
| (m, ctx) -> when(m.send(any(HttpRequest.class))).thenReturn(successResponse("mtls-pop-token", "mtls_pop")))) { | ||
|
|
||
| result = app.acquireToken(ClientCredentialParameters.builder(Collections.singleton("https://graph.microsoft.com/.default")) | ||
| .mtlsProofOfPossession() |
There was a problem hiding this comment.
is there a test that covers the bearer flow using bound creds?
…name the X509 matrix Ports the rigor from the merged MSAL.NET reference (PR #6100) to the SNI mTLS PoP integration tests and docs: - Add reusable MtlsResourceCaller.callResourceWithMtlsToken(url, token, cert): presents the binding certificate on the TLS handshake (reusing createMtlsSocketFactory), sends Authorization: mtls_pop <token>, GETs the resource, returns the status; drains and never logs the external body. The headline PoP test now calls mtlstb.graph.microsoft.com and requires HTTP 200, proving the bound token is genuinely usable (not just well-formed). - Fix the @BeforeAll escape hatch: assertNotNull on the lab cert hard-failed the whole class off-CI; use Assumptions.assumeTrue so it skips instead (missing lab cert is an environment condition, not a defect). - Establish the Credential_X509_Output_<Pop|Bearer> naming: rename the PoP tests into the family and add a standalone Credential_X509_Output_Bearer (asserts BEARER + null binding cert) instead of relying only on the cache-isolation side effect. - Remove a redundant assertNotNull(thumbprint) immediately before assertEquals. - Add docs/mtls-pop.md documenting the correct public API (mtlsProofOfPossession(), result.metadata().tokenType()/bindingCertificate(), region optional/global fallback, tenanted authority, Bearer omission is mTLS-specific). Written fresh; not copied from the stale #1021 branch. - Fix the sample's missing ClientCredentialFactory import. The Fic E2E resource call reuses MtlsResourceCaller in the FIC follow-up (#1041). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Align with merged msal-dotnet #6100 (origin/main), which deleted its ExecuteOrInconclusiveOnTokenTypeMismatchAsync helper. Asserting mtls_pop directly makes a genuine token_type downgrade fail red instead of being masked as inconclusive. Keep every mtls_pop assertion on the global mtlsauth.microsoft.com endpoint (the westus3 test slice is the known intermittent downgrader): the former regional Pop cache test is now global and renamed Credential_X509_Output_Pop_CacheHit. Remove the now-unused TEST_SLICE_REGION constant and ExecutionException import. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Mirror merged msal-dotnet #6100 exactly: Credential_X509_Output_Bearer now runs against the westus3 regional endpoint (.azureRegion), keeping live regional-endpoint coverage on the one cell whose token_type is deterministic. The mtls_pop cells stay global so the intermittent westus3 mtls_pop downgrade can never mask a real regression. Re-adds the TEST_SLICE_REGION constant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Bring the parent SNI branch (rginsburg/sni-mtls-pop @ d22059a) into the FIC branch with --no-ff. Resolve the shared MtlsPopIT.java conflict by adopting the SNI hardening for the direct-SNI cells while preserving the FIC two-leg test and its skip-on-downgrade hatch untouched: - Adopt: @BeforeAll Assumptions.assumeTrue env-absence skip; the Credential_X509_Output_{Pop,Bearer,Pop_CacheHit,Pop_And_Bearer_CacheIsolated} matrix; the MtlsResourceCaller resource-call assertEquals(200) on the headline Pop cell; Bearer on the regional endpoint; direct mtls_pop assertions (no skip). - Preserve: acquireTokenFic_TwoLeg_MtlsPop_BothLegsBound and the retained acquireMtlsPopOrSkipOnDowngrade helper it depends on (de-hatching FIC is Task 2). New parent-only files (docs/mtls-pop.md, MtlsResourceCaller.java) and the sample import merged cleanly. Integration-test sources compile. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Integrate upstream dev (through 56b9c6b) into the SN/I mTLS PoP branch. The only content conflict was ClientCredentialParameters.java: dev refactored extended-cache-key storage into the shared ExtendedCacheKey helper, while this branch added the mtlsProofOfPossession option plus post-construction cert_kid stamping (bindingCertificateKeyId). Resolved by adopting dev's ExtendedCacheKey refactor and re-applying the mTLS PoP feature on top: ExtendedCacheKey gains a synchronized copy-on-write putComponent(...) so bindingCertificateKeyId can stamp the binding-cert KeyId and invalidate the memoized hash. TokenRequestExecutor auto-merged (dev's clientClaims merge block and this branch's mTLS request / endpoint / token-type validation coexist). Moved the unreleased mTLS PoP changelog entry from 1.25.0 (shipped) up to 1.25.1. All Task-1 hardening preserved (no escape hatch on the X509 cells). mvn -pl msal4j-sdk test = 402 green; integrationtest compiles. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Capture a bounded error-body snippet on non-2xx responses in MtlsResourceCaller so a failing Credential_X509_Output_Pop assertion is self-diagnosing: a Graph error JSON (code + message) distinguishes a 400 (request reached the resource, rejected as malformed / token shape / audience) from a 401/403 (binding certificate not presented on the handshake or wrong mtls_pop scheme). A successful (2xx) resource body is still drained and discarded and never surfaced. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…anch Brings in MtlsResourceCaller error-body surfacing + Credential_X509_Output_Pop assertion message. FIC test acquireTokenFic_TwoLeg_MtlsPop_BothLegsBound and its skip gate are untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The JDK's HttpURLConnection injects a default Accept header (text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2) when none is set. The bare '*; q=.2' token has no '/', so Graph's mTLS frontend rejects the request with HTTP 400 BadRequest on a strict MIME parse — a false negative with nothing to do with the mTLS binding or the mtls_pop scheme (token_type==mtls_pop asserts fine before the call). Strict vs lenient Graph frontends explain why this reproduced in CI but not locally. Set 'Accept: application/json' explicitly so the request is well-formed and the bound PoP token is accepted with HTTP 200. Test-helper-only, Java-only (other SDKs' HTTP clients do not send this malformed default). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Pulls in MtlsResourceCaller Accept: application/json fix. FIC test acquireTokenFic_TwoLeg_MtlsPop_BothLegsBound and its acquireMtlsPopOrSkipOnDowngrade hatch untouched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| * mTLS Proof-of-Possession results, and are {@code null} for Bearer results. For more details, see | ||
| * https://aka.ms/msal4j-pop | ||
| */ | ||
| public final class BindingCertificate implements Serializable { |
There was a problem hiding this comment.
[Blocker/API] This result exposes only public certificate information, so it cannot prove possession when calling the resource; the E2E consequently uses the original credential instead of the returned result. #1059 introduces IMtlsBindingContext with the exact certificate, key ID and reusable process-local SSLContext. Please converge on that as the authoritative binding capability and derive this diagnostics-only view from it.
|
|
||
| @Override | ||
| AuthenticationResult execute() throws Exception { | ||
| // For mTLS Proof-of-Possession, isolate the access token in the cache by the binding certificate's |
There was a problem hiding this comment.
[Blocker] mtlsProofOfPossession() can be combined with appTokenProvider. A provider token is returned without an mTLS request or response-token-type validation, saved under the certificate hash, and later forcibly relabelled MTLS_POP on a cache hit. Please reject this combination unless the provider contract supplies and validates an atomic binding context and token type.
| // For mTLS Proof-of-Possession, isolate the access token in the cache by the binding certificate's | ||
| // KeyId (x5t#S256) in addition to the token_type dimension, so PoP tokens bound to different | ||
| // certificates never alias. Stamped before the cache lookup so reads and writes hash identically. | ||
| IClientCertificate resolvedBindingCertificate = null; |
There was a problem hiding this comment.
[Blocker] This writes application-specific certificate identity into a public reusable parameters object. Two CCAs using certificates A and B can concurrently use the same parameters instance, causing one request to compute, read or save using the other application’s partition. Please keep public parameters immutable and carry an acquisition-local binding snapshot/cache key through lookup, execution and cache save.
| @@ -32,9 +37,30 @@ final class ExtendedCacheKey { | |||
| * when there are none. The result is memoized. | |||
| */ | |||
| String computeHash() { | |||
There was a problem hiding this comment.
[Blocker] putComponent() is synchronized but hash computation/publication is not. A thread can compute hash A, another thread can install component B and clear the cache, and the first thread can then republish stale hash A. Please eliminate post-construction mutation or synchronize/version the complete compute-and-publish operation.
| IHttpClient mtlsHttpClient = null; | ||
|
|
||
| if (isMtlsProofOfPossession()) { | ||
| ConfidentialClientApplication application = (ConfidentialClientApplication) msalRequest.application(); |
There was a problem hiding this comment.
[Blocker] The binding is independently resolved for the cache key, TLS key manager and result metadata. A rotating or mutable credential can therefore produce certificate A for caching, B for the TLS handshake and C in the result. Please resolve one immutable binding context and use its key ID, certificate and SSLContext throughout the acquisition.
| tokenEndpointUrl = MtlsEndpointHelper.deriveMtlsTokenEndpoint(tokenEndpointUrl); | ||
| SSLSocketFactory mtlsSocketFactory = | ||
| MtlsClientCertificateHelper.createMtlsSocketFactory(resolvedBindingCertificate); | ||
| mtlsHttpClient = new DefaultHttpClient( |
There was a problem hiding this comment.
[Blocker] Constructing a new DefaultHttpClient bypasses the configured IHttpClient, custom trust roots, proxy and egress controls, certificate pinning, interceptors, tracing and client-specific timeout/cancellation behavior. Please use #1059’s request-specific socket-factory plus IMtlsCapableHttpClient path and fail clearly when a custom client cannot honor mTLS.
| tokenEndpointUrl = MtlsEndpointHelper.deriveMtlsTokenEndpoint(tokenEndpointUrl); | ||
| SSLSocketFactory mtlsSocketFactory = | ||
| MtlsClientCertificateHelper.createMtlsSocketFactory(resolvedBindingCertificate); | ||
| mtlsHttpClient = new DefaultHttpClient( |
There was a problem hiding this comment.
[Blocker/security] This mTLS client retains HttpsURLConnection’s default redirect behavior. A same-protocol redirect can reconnect with the credential-bearing SSLSocketFactory, presenting the client certificate to another host; a 307 can also preserve the POST body. Disable redirects for all credential-bound token requests, matching #1059.
| ConfidentialClientApplication application = (ConfidentialClientApplication) msalRequest.application(); | ||
| this.resolvedBindingCertificate = resolveBindingCertificate(application); | ||
| tokenEndpointUrl = MtlsEndpointHelper.deriveMtlsTokenEndpoint(tokenEndpointUrl); | ||
| SSLSocketFactory mtlsSocketFactory = |
There was a problem hiding this comment.
[Blocker/availability] A new SSLContext, SSLSocketFactory and client are created for every network request. This prevents keep-alive and TLS-session reuse and temporarily strands one-use pool entries, increasing handshake CPU, latency and socket pressure. Please use a bounded per-certificate context/transport cache with explicit idle cleanup.
| } else if (credentialToUse instanceof ClientCertificate) { | ||
| if (mtlsPoP) { | ||
| // For mTLS PoP (direct SN/I cert / FIC Leg 1), the certificate is presented as the client | ||
| // TLS certificate and authenticates the client; no client_assertion is sent (ESTS resolves |
There was a problem hiding this comment.
[Blocker] extraQueryParameters is merged after grant parameters, so callers can overwrite token_type, grant_type or scope, inject req_cnf, or inject a client_assertion into the pure-certificate branch. Please reject MSAL-owned OAuth and client-authentication parameters before network execution instead of warning and overwriting.
| } | ||
|
|
||
| /** | ||
| * Maps a token_type string (as returned in a token response) to a {@link TokenType}. |
There was a problem hiding this comment.
[P1/API] Every non-mtls_pop value—including SHR pop and future token schemes—is reported publicly as BEARER. Please preserve the raw service value, as #1059 does, or add explicit POP and UNKNOWN representations. Exact mtls_pop matching can remain for fail-closed validation.
| * {@code login.*} host and therefore cannot be safely rewritten | ||
| */ | ||
| static String deriveMtlsHost(String loginHost) { | ||
| String lower = loginHost.toLowerCase(Locale.ROOT); |
There was a problem hiding this comment.
[Blocker] Regional sovereign authorities arrive as <region>.login.microsoftonline.us, <region>.login.partner.microsoftonline.cn, etc. They do not match .login.microsoft.com, are not exact trusted-host entries and do not begin directly with login., so this returns null. Parse the regional prefix separately, normalize the base authority and preserve both region and sovereign domain during rewriting.
| // mTLS PoP is unsupported ONLY for these two legacy sovereign hosts (they have no mtlsauth.* endpoint). | ||
| // Every other login.* host — including Azure Government and the current national clouds — is allowed and | ||
| // rewritten domain-preserving. Mirrors MSAL.NET's s_unsupportedMtlsHosts denylist. Hosts are lowercase; | ||
| // callers lowercase the input before lookup. |
There was a problem hiding this comment.
[Blocker/documentation] This denylist runs after preferred-network normalization, which converts these legacy aliases to their modern hosts. The helper-only tests therefore do not represent normal acquisition behavior, and the stated “legacy aliases fail fast” contract is not enforced. Please reject the original authority before normalization or document and test alias normalization as supported.
| @@ -32,9 +37,30 @@ final class ExtendedCacheKey { | |||
| * when there are none. The result is memoized. | |||
| */ | |||
| String computeHash() { | |||
There was a problem hiding this comment.
[Blocker/rebase] This head predates current dev commit 204bdd4, which replaced delimiter-free key + value concatenation with UTF-8 length-prefixed serialization. The old encoding is ambiguous and this PR adds security-sensitive cache dimensions. Please rebase before validating certificate-cache isolation and update the expected hashes and tests.
| * The type of the access token in the {@link AuthenticationResult}, see {@link TokenType} for possible | ||
| * values. Defaults to {@link TokenType#BEARER}. | ||
| */ | ||
| private TokenType tokenType = TokenType.BEARER; |
There was a problem hiding this comment.
[Blocker/compatibility] IAuthenticationResult is explicitly Serializable, but this public metadata type has no pinned UID. serialver changes from 6600853896042983641L at the PR base to 4902369891410840671L here, so previously serialized results fail after upgrade. Please pin the previous UID and add a previous-version deserialization fixture.
|
|
||
| @Test | ||
| void cacheKey_isolatesBearerFromMtlsPopAndByCertificate() { | ||
| ClientCredentialParameters bearer = ClientCredentialParameters |
There was a problem hiding this comment.
[Acceptance blocker] Comparing input hashes does not prove that cache writes, reads, coexistence and serialization preserve certificate partitions. Please exercise A-network → B-network → A-cache with real entries, serialize and restore into new clients, and add same-key/renewed-leaf coverage. Assert the token, source, token type and exact binding generation on every result.
| .build()) | ||
| .get(); | ||
|
|
||
| assertMtlsPopResult(result, expectedLabThumbprint()); |
There was a problem hiding this comment.
[Acceptance blocker] This resource call uses the original credential rather than the binding returned by MSAL, so it does not satisfy BIND-01/BIND-07. Return IMtlsBindingContext, use its exact SSLContext for the resource call, verify cnf.x5t#S256, and add rejection cells for no certificate, the wrong scheme and a different certificate.
SN/I certificate over mTLS PoP
Confidential client presents its SN/I cert as the client TLS cert in the token-endpoint handshake → certificate-bound
token_type=mtls_pop(cnf/x5t#S256) instead of a signed assertion. Opt-in, same credential; also covers FIC Leg 1. SNI+Bearer / broker SHR-PoP unchanged. Two-leg FIC stacked in #1041.ClientCredentialParameters.Builder.mtlsProofOfPossession(); new publicTokenType(BEARER/MTLS_POP) +BindingCertificate(public material only);AuthenticationResultMetadata.tokenType()/bindingCertificate().MtlsClientCertificateHelper+MtlsEndpointHelper(login.*→mtlsauth.*, region-optional, tenanted-only; legacy usgov+china fail fast); direct-cert path omitsclient_assertion; mTLS transport via injectedDefaultHttpClient; AT cache keyed bytoken_type+cert KeyId.MtlsPopITCredential_X509_Output_{Pop,Bearer}(+cache isolation) — PoP global assertsMTLS_POPand is proven usable by a strict-200mtls_pop-scheme call tomtlstb.graph.microsoft.com; Bearer regional (westus3).