Skip to content

Revise mTLS PoP acceptance-test matrix structure - #6161

Merged
Gladwin Johnson VR (gladjohn) merged 5 commits into
mainfrom
gladjohn-patch-25
Sep 2, 2026
Merged

Gladwin Johnson VR (gladjohn) merged 5 commits into
mainfrom
gladjohn-patch-25

Conversation

@gladjohn

Copy link
Copy Markdown
Contributor

Updated the mTLS PoP acceptance-test matrix to clarify approval criteria and restructured test cases for better organization.

Updated the mTLS PoP acceptance-test matrix to clarify approval criteria and restructured test cases for better organization.
Copilot AI lite review requested due to automatic review settings August 17, 2026 20:41
@gladjohn
Gladwin Johnson VR (gladjohn) requested a review from a team as a code owner August 17, 2026 20:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a new documentation artifact defining an mTLS PoP acceptance-test matrix for MSAL.NET, intended to clarify PR approval criteria and provide a structured checklist of end-to-end behaviors to validate.

Changes:

  • Introduces an “Approval criteria” section (P0/P1) to define required evidence and expectations for end-to-end tests.
  • Adds a structured acceptance-test matrix organized by functional areas (configuration, request semantics, region/authority routing, binding, cache, transport, platform).
  • Defines explicit expected endpoints/behaviors for regional/global and sovereign cloud routing scenarios.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Copilot AI review requested due to automatic review settings August 17, 2026 20:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (6)

docs/mtls-pop-acceptance-test-matrix.md:18

  • CFG-05 currently describes an impossible/undefined setup (“empty TLS certificate”) and has a non-verifiable assertion (“See if we handle services errors correctly”). This should be a concrete, testable MSAL.NET behavior (e.g., missing certificate fails fast before any network request with the established MtlsCertificateNotProvided error).
| CFG-05 | Empty certificate chain | Provide an empty TLS certificate. | See if we handle services errors correctly. | P0 |

docs/mtls-pop-acceptance-test-matrix.md:19

  • This matrix is for MSAL.NET, but CFG-06 references Go’s crypto.Signer and Go-specific casting/export concerns. Please express the non-exportable/private-key scenario using .NET concepts (e.g., X509Certificate2 with a non-exportable KeyGuard/TPM/HSM-backed key).
| CFG-06 | Opaque signer | Use a non-exportable `crypto.Signer` implementation. | The TLS handshake succeeds without exporting the key or casting it to a concrete key type. | P1 |

docs/mtls-pop-acceptance-test-matrix.md:115

  • HTTP-06 references “the race detector” which is specific to Go tooling. For MSAL.NET, describe the concurrency expectation in platform-neutral/.NET terms (parallel acquisitions should be thread-safe and reuse the same transport).
| HTTP-06 | Concurrent same certificate | Run many parallel acquisitions under the race detector. | No races, one transport, and consistent results. | P0 |

docs/mtls-pop-acceptance-test-matrix.md:127

  • PLAT-04 again references Go’s crypto.Signer. For MSAL.NET platform acceptance, describe this as a non-exportable X509Certificate2 private key backed by the platform/HSM/KeyGuard rather than a Go interface type.
| PLAT-04 | Non-exportable signer | A supported hardware or platform key provider | Token and resource handshakes use `crypto.Signer` without exporting the key. | P1 |

docs/mtls-pop-acceptance-test-matrix.md:112

  • HTTP-03 uses Go terms (nil, panicking). In MSAL.NET this should be null/exceptions and describe the expected managed failure mode (explicit exception/error result without crashing).
| HTTP-03 | Nil factory result | Make the factory return `nil`. | Return an explicit error without panicking. | P0 |

docs/mtls-pop-acceptance-test-matrix.md:110

  • HTTP-01 uses the term “dialing”, which is Go-specific; in MSAL.NET docs this should refer to HTTP connection behavior (e.g., sockets/handlers/proxy behavior).
| HTTP-01 | Default transport | Use the built-in mTLS transport. | Enforce TLS 1.2 minimum while preserving normal proxy and dialing behavior. | P1 |

Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
|---|---|---|---|---|---|
| REQ-01 | Global PoP request | Use a tenanted AAD authority without a region. | POST to `https://mtlsauth.microsoft.com/{tenant}/oauth2/v2.0/token`. | An `mtls_pop` token is returned. | P0 |
| REQ-02 | Regional PoP request | Configure `westus3`. | POST to `https://westus3.mtlsauth.microsoft.com/{tenant}/oauth2/v2.0/token`. | An `mtls_pop` token is returned. | P0 |
| REQ-03 | TLS certificate presented | Configure the token server to require a client certificate. | Capture the TLS peer certificate. Its leaf DER must equal the configured certificate. | The handshake and request succeed. | P0 |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't understand this one. Isn't it just vanilla SN/I credential -> POP token ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes - updated

Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
| REQ-01 | Global PoP request | Use a tenanted AAD authority without a region. | POST to `https://mtlsauth.microsoft.com/{tenant}/oauth2/v2.0/token`. | An `mtls_pop` token is returned. | P0 |
| REQ-02 | Regional PoP request | Configure `westus3`. | POST to `https://westus3.mtlsauth.microsoft.com/{tenant}/oauth2/v2.0/token`. | An `mtls_pop` token is returned. | P0 |
| REQ-03 | TLS certificate presented | Configure the token server to require a client certificate. | Capture the TLS peer certificate. Its leaf DER must equal the configured certificate. | The handshake and request succeed. | P0 |
| REQ-04 | Required body parameters | Capture the PoP request body. | It contains `client_id`, `scope`, `grant_type=client_credentials`, and `token_type=mtls_pop`. | The request is accepted. | P0 |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

REQ-04 and REQ-05 are not tests on their own. These are just normal assertions that should be included in all tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated

| ID | Test | Setup and action | Required assertions | Priority |
|---|---|---|---|---|
| SNI-01 | Existing SNI assertion | Use the same certificate without the PoP option and enable x5c. | Use the regular token endpoint. Send `client_assertion` and x5c. Do not present a TLS client certificate. | P0 |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is more to ensure we have an E2E test with SN/I cert + x5c right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes - existing feature has an e2e and we do not break it

Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
| AUTH-01 | Specific tenant | Use a tenant GUID or verified tenant domain. | Token acquisition succeeds. | P0 |
| AUTH-02 | `/common` | Request PoP using `/common`. | Fail before the token request or credential transmission. | P0 |
| AUTH-03 | `/organizations` | Request PoP using `/organizations`. | Fail before the token request or credential transmission. | P0 |
| AUTH-04 | `/consumers` | Request PoP using `/consumers`. | Fail before the token request or credential transmission. | P0 |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should not fail on /consumers. Consumers is just a tenant.

Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
Comment thread docs/mtls-pop-acceptance-test-matrix.md Outdated
| BIND-07 | Wrong resource certificate | Call the resource using another certificate. | The resource rejects the request. | P0 |
| BIND-08 | Exact returned certificate | Use the result certificate directly without reconstructing it. | The TLS handshake and resource call succeed. | P0 |

## Cache and certificate lifecycle

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a lot of syncronization logic in MSAL .NET around certs, keys etc. Is the goal for all MSALs to access those caches? Or should each MSAL deal with its own?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for the cache key (keyid) we should have common goals but for the cert cache each MSAL should define its own logic

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b9ee472f-6e81-416f-90b2-46c9f74292e1
Copilot AI review requested due to automatic review settings August 18, 2026 15:51
@gladjohn

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review Bogdan Gavril (@bgavrilMS) . I pushed a revised matrix that:

  • removes priorities;
  • uses MSAL.NET terminology;
  • separates X509, FIC + X509, and Managed Identity scenarios;
  • consolidates request and SNI assertions;
  • corrects authority/cloud routing;
  • clarifies binding-certificate validation;
  • keeps cache and transport requirements behavioral

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (2)

docs/mtls-pop-acceptance-test-matrix.md:16

  • The matrix lists client_assertion_type=jwt-pop, but the actual wire value used elsewhere in this doc (and in tests) is the full URN (urn:ietf:params:oauth:client-assertion-type:jwt-pop). Using jwt-pop as a literal value would be incorrect and could mislead acceptance criteria.
| Federated assertion | `ClientSignedAssertion` containing an assertion and `TokenBindingCertificate` | Present the binding certificate over mTLS and send the assertion with `client_assertion_type=jwt-pop`. |

docs/mtls-pop-acceptance-test-matrix.md:126

  • The platform acceptance section marks Linux as required for mTLS PoP, but the current integration tests explicitly skip Linux for PoP/mTLS (e.g., ClientCredentialsMtlsPopTests uses [RunOn(SkipConditions.Linux)] // PoP is not supported on Linux). This makes the acceptance contract inconsistent with the repo’s current supported-test surface.
| PLAT-02 | Linux certificate | Linux | Token acquisition and the resource call succeed. |
| PLAT-03 | Supported TLS versions | Windows and Linux | Token and resource TLS handshakes meet MSAL.NET's supported TLS requirements. |

Copilot AI review requested due to automatic review settings August 19, 2026 12:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (3)

docs/mtls-pop-acceptance-test-matrix.md:27

  • CFG-05 groups multiple misconfigurations that currently produce different error codes: static assertions and string callbacks (which cannot supply a binding certificate) fail with InvalidCredentialMaterial, while a ClientSignedAssertion callback that returns no TokenBindingCertificate fails with MtlsCertificateNotProvided. The acceptance criteria should reflect this so tests can assert the correct public contract.
| CFG-05 | Assertion without binding certificate rejected | Configure a static assertion, string callback, or `ClientSignedAssertion` without a binding certificate and request mTLS PoP. | Fail before network with a clear missing-binding-certificate error. |

docs/mtls-pop-acceptance-test-matrix.md:16

  • The document uses client_assertion_type=jwt-pop, but MSAL emits the full URN value for JWT-PoP assertions (urn:ietf:params:oauth:client-assertion-type:jwt-pop). This is also inconsistent with REQ-03 in the same matrix, and can lead to incorrect acceptance criteria for request-body assertions.

This issue also appears on line 27 of the same file.

| Federated assertion | `ClientSignedAssertion` containing an assertion and `TokenBindingCertificate` | Present the binding certificate over mTLS and send the assertion with `client_assertion_type=jwt-pop`. |

docs/mtls-pop-acceptance-test-matrix.md:26

  • CFG-04’s expected error code doesn’t match the current implementation: using a client secret in mTLS mode throws InvalidCredentialMaterial (see ClientCredentialGuards.ThrowIfMtlsNotSupported), not MtlsCertificateNotProvided. As written, this acceptance criterion would fail against the product behavior.
| CFG-04 | Client secret rejected | Configure a client secret and request mTLS PoP. | Fail before any token request with `MtlsCertificateNotProvided` or the established unsupported-credential error. |

@gladjohn
Gladwin Johnson VR (gladjohn) merged commit 551ea46 into main Sep 2, 2026
17 checks passed
@gladjohn
Gladwin Johnson VR (gladjohn) deleted the gladjohn-patch-25 branch September 2, 2026 12:20
@Avery-Dunn Avery-Dunn mentioned this pull request Sep 8, 2026
This was referenced Sep 16, 2026
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.

4 participants