mTLS PoP: add SNI / S2S-FIC test coverage + docs#6100
Open
Robbie-Microsoft wants to merge 10 commits into
Open
mTLS PoP: add SNI / S2S-FIC test coverage + docs#6100Robbie-Microsoft wants to merge 10 commits into
Robbie-Microsoft wants to merge 10 commits into
Conversation
… messages mTLS PoP does not require a region: when none is set, MSAL uses the global mtlsauth.microsoft.com endpoint (RegionAndMtlsDiscoveryProvider already falls back). Corrected the misleading RegionRequiredForMtlsPop / MtlsPopWithoutRegion / MtlsBearerWithoutRegion message text and doc comments (public constants preserved; no longer raised). Isolated the sovereign-cloud guardrail in RegionAndMtlsDiscoveryProvider as a single, clearly-marked override point (comment-only; fail-fast behavior unchanged). Added unit tests (SN/I sendX5C omits client_assertion/req_cnf; S2S FIC carry-over sends jwt-pop and binds to the carried cert; default transport is mTLS-capable) and two-leg S2S FIC E2E tests (both legs mtls_pop with binding-cert continuity, plus an FMI-audience variant). Updated design doc, skills guide, and CHANGELOG. No public API change; user_fic is not supported over mTLS. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates MSAL.NET’s mTLS Proof-of-Possession (PoP) guidance and coverage to reflect that Azure region is optional (global mtlsauth.microsoft.com fallback), and adds test + documentation coverage for SNI first-leg and S2S/app FIC leg-2 carry-over scenarios over mTLS PoP (no public API changes).
Changes:
- Corrects stale region-required mTLS PoP error-message text and updates historical error-code XML docs to reflect global endpoint fallback.
- Adds unit + integration test coverage for SNI-over-mTLS-PoP and S2S FIC carry-over (jwt-pop assertion type and binding-cert continuity).
- Updates design/docs and changelog to document region-optional behavior and S2S-only scope (no
user_ficover mTLS).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Identity.Test.Unit/PublicApiTests/MtlsPopTests.cs | Adds unit tests for SNI mTLS PoP body shape, S2S FIC carry-over behavior, and default transport being mTLS-capable. |
| tests/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/ClientCredentialsMtlsPopTests.cs | Adds E2E coverage for two-leg S2S FIC over mTLS PoP (global endpoint), including an FMI-audience variant. |
| src/client/Microsoft.Identity.Client/MsalErrorMessage.cs | Updates region-related mTLS messages to state region is optional and global fallback is used. |
| src/client/Microsoft.Identity.Client/MsalError.cs | Updates XML docs to mark region-required mTLS errors as historical / no longer raised. |
| src/client/Microsoft.Identity.Client/Instance/Discovery/RegionAndMtlsDiscoveryProvider.cs | Refactors comments to clearly document the sovereign-cloud guardrail as a single override point. |
| docs/sni_mtls_pop_token_design.md | Documents region-optional behavior and adds a two-leg S2S FIC over mTLS PoP section and examples. |
| docs/mtls_pop_skills_guide.md | Adds prompts/notes for SNI first leg, S2S-only FIC scope, and region-optional behavior. |
| CHANGELOG.md | Adds an Unreleased entry summarizing the region-optional messaging and new mTLS PoP S2S FIC coverage. |
Robbie-Microsoft
marked this pull request as draft
July 1, 2026 20:51
…ent-id Fold the both-legs-mtls_pop and binding-cert-continuity assertions into the existing Sni_AssertionFlow_GlobalEndpoint_Uses_JwtPop_And_Succeeds test (carrying Leg-1 BindingCertificate forward) and delete the redundant standalone Sni_TwoLeg_S2sFic_BothLegs_Pop_EndToEnd test. Parameterize the FMI-audience helper's Leg-2 client id so both legs run under the same identity; an ESTS rejection is then unambiguously an FMI + mTLS enablement issue rather than a client-id mismatch between legs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ce const in folded test - MsalErrorMessage.cs / MsalError.cs: the no-longer-raised region messages and XML docs said 'the global mTLS endpoint (mtlsauth.microsoft.com)', which is public-cloud only (sovereign clouds use their own mtlsauth host). Dropped the hardcoded host so the text is cloud-accurate. - ClientCredentialsMtlsPopTests.cs: the folded global-endpoint test now uses the AllowListedFinalResource constant instead of the inline literal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
bgavrilMS
reviewed
Jul 8, 2026
bgavrilMS
requested changes
Jul 8, 2026
bgavrilMS
left a comment
Member
There was a problem hiding this comment.
Unclear what this PR is about.
bgavrilMS
reviewed
Jul 8, 2026
bgavrilMS
reviewed
Jul 8, 2026
…docs - RegionAndMtlsDiscoveryProvider.cs: revert the added 'SOVEREIGN GUARDRAIL' banner comment (reverts to origin/main). The block-list is pre-existing and only redirects two legacy alias hosts; the comment mischaracterized it. - MsalError.cs: drop the confusing 'Historical error code' phrasing on the three region constants; state plainly that a region is not required (global endpoint fallback) and that the constants are retained for back-compat. They remain public shipped API, so they are not deleted. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…host; fix SNI typo Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
gladjohn
requested changes
Jul 13, 2026
gladjohn
left a comment
Contributor
There was a problem hiding this comment.
you should create separate PRs for this, "fix misleading region-required messages" is a must fix. I am not sure about FIC tests. we already have fic tests.
Revert MsalError.cs and MsalErrorMessage.cs to baseline so this PR carries only the SNI / S2S-FIC tests and docs. The misleading region-required message + doc-comment fix now ships separately in #6127. Drop the message-fix clause from the CHANGELOG (docs + tests entries retained). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…p-fic # Conflicts: # CHANGELOG.md
… other tests inline the literal Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment on lines
+534
to
+544
| try | ||
| { | ||
| await RunTwoLegS2sFicBothLegsPopAsync(FmiClientId, FmiClientId, FmiTokenExchangeUrl).ConfigureAwait(false); | ||
| } | ||
| catch (MsalServiceException ex) | ||
| { | ||
| Assert.Inconclusive( | ||
| "FMI-audience mTLS PoP exchange was rejected by ESTS for this app/lab configuration. " + | ||
| "This variant only proves the exchange audience is caller-supplied; the generic " + | ||
| $"api://AzureADTokenExchange path remains under test. Underlying error: {ex.Message}"); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SNI-cert first-leg / S2S-FIC carry-over / mTLS-PoP plumbing already shipped in
main. This PR is tests + docs only — no product code, public API, or behavior change. The related error-message / doc-comment fix was split out to #6127.Tests
MtlsPopTests.cs, +3): SNI omitsclient_assertion/client_assertion_type/req_cnf; S2S FIC carry-over sendsclient_assertion_type=jwt-popand binds to the carried cert; default transport is mTLS-capable.ClientCredentialsMtlsPopTests.cs): assert both legs aremtls_popwith binding-cert continuity; add an FMI-audience variant proving the exchange audience is caller-supplied (inconclusive if FMI+mTLS isn't lab-enabled).Docs
sni_mtls_pop_token_design.md,mtls_pop_skills_guide.md,CHANGELOG.md— region is optional / global fallback, two-leg S2S FIC, caller-supplied exchange audience.user_fic(user-delegated FIC) is not supported over mTLS — FIC over mTLS PoP is S2S/app-only.