Implementation for SNI + MTLS Flow in MSAL #4965

gladjohn · 2024-10-22T00:32:27Z

Fixes : #4986

Changes proposed in this request

When .WithCertificate API is used and if the new .WithMtlsPop... API is used at request level, we now use that cert over mTLS
ESTS on the other hand, will use the mTLS cert as the auth cert in this client credentials flow
MSAL will simply pass the client id and the MTLS cert and will also specify that the token type = mtls_pop and ESTS will issue a token that will contain the cnf claim in it.
Todo : need to test passing X5C and skipping it when ESTS is ready to observe errors they send

Testing

unit tests
waiting for ests implementation and then add integ tests

Performance impact

Documentation

Todo : need to update docs for MTLS POP

src/client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForClientParameterBuilder.cs

src/client/Microsoft.Identity.Client/ApiConfig/Executors/ConfidentialClientExecutor.cs

...oft.Identity.Client/ApiConfig/Parameters/AbstractAcquireTokenConfidentialClientParameters.cs

src/client/Microsoft.Identity.Client/AppConfig/ApplicationConfiguration.cs

src/client/Microsoft.Identity.Client/AuthScheme/PoP/MtlsPopAuthenticationOperation.cs

src/client/Microsoft.Identity.Client/Instance/Discovery/RegionDiscoveryProvider.cs

src/client/Microsoft.Identity.Client/Internal/Requests/ClientCredentialRequest.cs

bgavrilMS · 2024-10-22T10:59:57Z

src/client/Microsoft.Identity.Client/OAuth2/OAuthConstants.cs

+    internal static class RequestTokenType
+    {
+        public const string Bearer = "bearer";
+        public const string MTLSPop = "mtls_pop";


Pls consolidate with SHR POP.

...s/Microsoft.Identity.Test.Integration.netcore/HeadlessTests/ClientCredentialsTests.NetFwk.cs

gladjohn · 2024-10-26T15:38:43Z

/azp run

azure-pipelines · 2024-10-26T15:38:53Z

Azure Pipelines successfully started running 1 pipeline(s).

…oft-authentication-library-for-dotnet into gladjohn/sni_mtls

gladjohn · 2024-10-31T17:45:15Z

/azp run

azure-pipelines · 2024-10-31T17:45:28Z

Azure Pipelines successfully started running 1 pipeline(s).

gladjohn · 2024-10-31T19:36:38Z

/azp run

azure-pipelines · 2024-10-31T19:36:49Z

Azure Pipelines successfully started running 1 pipeline(s).

gladjohn · 2024-11-02T16:44:02Z

/azp run

azure-pipelines · 2024-11-02T16:44:11Z

Azure Pipelines successfully started running 1 pipeline(s).

bgavrilMS · 2024-11-04T13:12:58Z

When can we review this @gladjohn ? Are there unit tests ?

...icrosoft.Identity.Client/ApiConfig/AbstractConfidentialClientAcquireTokenParameterBuilder.cs

gladjohn · 2024-11-04T23:04:23Z

When can we review this @gladjohn ? Are there unit tests ?

@bgavrilMS working on adding unit tests., I should have PR updated by EOD tomorrow

src/client/Microsoft.Identity.Client/ApiConfig/AcquireTokenForClientParameterBuilder.cs

src/client/Microsoft.Identity.Client/AuthScheme/PoP/MtlsPopAuthenticationOperation.cs

src/client/Microsoft.Identity.Client/AuthScheme/TokenType.cs

gladjohn · 2024-11-05T19:32:27Z

/azp run

azure-pipelines · 2024-11-05T19:32:37Z

Azure Pipelines successfully started running 1 pipeline(s).

gladjohn · 2024-11-05T19:53:15Z

/azp run

azure-pipelines · 2024-11-05T19:53:25Z

Azure Pipelines successfully started running 1 pipeline(s).

bgavrilMS · 2024-11-06T11:02:00Z

...icrosoft.Identity.Client/ApiConfig/AbstractConfidentialClientAcquireTokenParameterBuilder.cs

        /// <item><description>The PoP token is bound to the HTTP request, more specifically to the HTTP method (GET, POST, etc.) and to the Uri (path and query, but not query parameters).</description></item>
        /// <item><description>MSAL creates, reads and stores a key in memory that will be cycled every 8 hours.</description></item>
        /// <item><description>This is an experimental API. The method signature may change in the future without involving a major version upgrade.</description></item>
        /// </list>
        /// </remarks>
+        [EditorBrowsable(EditorBrowsableState.Never)] // Soft deprecate
+        [Obsolete]


Pls add an error message explaining the rename.

bgavrilMS · 2024-11-06T11:03:14Z

src/client/Microsoft.Identity.Client/AuthScheme/TokenType.cs

+        /// MTLS Pop token type.
+        /// MTLS_Pop = 5
+        /// </summary>
+        Mtls_Pop = 5


Extension is 5, this needs to be at least 6.

bgavrilMS · 2024-11-06T11:22:04Z

src/client/Microsoft.Identity.Client/Instance/Discovery/RegionAndMtlsDiscoveryProvider.cs

-            return $"{region}.{host}";
+
+            // Decide whether to use MTLS or standard environment
+            if (!requestContext.UseMtlsPop)


In region is configured by app developer, and public cloud is used, then the regional env will be returned at line 65. So it will ignore mTLS endpoint.

This shows there is insufficient unit testing in this area.

bgavrilMS · 2024-11-06T11:24:20Z

src/client/Microsoft.Identity.Client/AuthScheme/TokenType.cs

@@ -32,6 +32,12 @@ internal enum TokenType
        /// Extension token type.
        /// Extension = 5
        /// </summary>
-        Extension = 5
+        Extension = 5,


unrelated - @trwalke, what will happen if we create another extension ?

bgavrilMS · 2024-11-06T11:37:01Z

tests/Microsoft.Identity.Test.Unit/ExceptionTests/ExperimentalFeatureTests.cs

@@ -17,18 +17,21 @@ namespace Microsoft.Identity.Test.Unit.ExceptionTests
    [TestClass]
    public class ExperimentalFeatureTests
    {
+        private static readonly string[] s_scopes = ["scope"];


nit: I am generally a fan of refactoring, but it's making life harder for reviewers. Next time, pls try to separate out refactoring either in a separate PR or in a separate commit.

bgavrilMS · 2024-11-06T11:39:38Z

tests/Microsoft.Identity.Test.Unit/PublicApiTests/MtlsPopTests.cs

+        [TestCleanup]
+        public override void TestCleanup()
+        {
+            Environment.SetEnvironmentVariable("REGION_NAME", null);


Not a good idea. This is set to some value when running on ADO agent.

See how all the MSI unit tests are doing it instead.

bgavrilMS · 2024-11-06T11:41:48Z

tests/Microsoft.Identity.Test.Unit/PublicApiTests/MtlsPopTests.cs

+
+                Assert.AreEqual("header.payload.signature", result.AccessToken);
+                Assert.AreEqual(region, result.ApiEvent.RegionUsed);
+                Assert.AreEqual(RegionOutcome.UserProvidedValid, result.ApiEvent.RegionOutcome);


No need to use ApiEvent to check this. Use result.Metadata

bgavrilMS · 2024-11-06T11:42:05Z

tests/Microsoft.Identity.Test.Unit/PublicApiTests/MtlsPopTests.cs

+                    .ConfigureAwait(false);
+
+                Assert.AreEqual("header.payload.signature", result.AccessToken);
+                Assert.AreEqual(region, result.ApiEvent.RegionUsed);


Use result.AuthenticationResultMetadata.TokenEndpoint to check the mTLS endpoint. This is a comment for all tests

localden · 2024-11-06T18:49:07Z

...icrosoft.Identity.Client/ApiConfig/AbstractConfidentialClientAcquireTokenParameterBuilder.cs

@@ -89,5 +91,29 @@ public T WithProofOfPossession(PoPAuthenticationConfiguration popAuthenticationC

            return this as T;
        }
+
+        /// <summary>
+        ///  Modifies the token acquisition request so that the acquired token is a Signed HTTP Request (SHR) Proof-of-Possession (PoP) token, rather than a Bearer token.


Suggested change

/// Modifies the token acquisition request so that the acquired token is a Signed HTTP Request (SHR) Proof-of-Possession (PoP) token, rather than a Bearer token.

/// Modifies the request to acquire a Signed HTTP Request (SHR) Proof-of-Possession (PoP) token, rather than a Bearer.

localden · 2024-11-06T18:49:56Z

...icrosoft.Identity.Client/ApiConfig/AbstractConfidentialClientAcquireTokenParameterBuilder.cs

+
+        /// <summary>
+        ///  Modifies the token acquisition request so that the acquired token is a Signed HTTP Request (SHR) Proof-of-Possession (PoP) token, rather than a Bearer token.
+        ///  SHR PoP tokens are bound to the HTTP request and to a cryptographic key, which MSAL can manage on Windows.


Suggested change

/// SHR PoP tokens are bound to the HTTP request and to a cryptographic key, which MSAL can manage on Windows.

/// SHR PoP tokens are bound to the HTTP request and to a cryptographic key, which MSAL manages on Windows.

localden · 2024-11-06T18:51:18Z

...icrosoft.Identity.Client/ApiConfig/AbstractConfidentialClientAcquireTokenParameterBuilder.cs

+        /// <summary>
+        ///  Modifies the token acquisition request so that the acquired token is a Signed HTTP Request (SHR) Proof-of-Possession (PoP) token, rather than a Bearer token.
+        ///  SHR PoP tokens are bound to the HTTP request and to a cryptographic key, which MSAL can manage on Windows.
+        ///  This differs from MTLS PoP tokens, which are used for Mutual TLS (mTLS) authentication. See <see href="https://aka.ms/mtls-pop"/> for mTLS PoP details.


Suggested change

/// This differs from MTLS PoP tokens, which are used for Mutual TLS (mTLS) authentication. See <see href="https://aka.ms/mtls-pop"/> for mTLS PoP details.

/// SHR PoP tokens are different from mTLS PoP tokens, which are used for Mutual TLS (mTLS) authentication. See <see href="https://aka.ms/mtls-pop"/> for details.

localden · 2024-11-06T18:53:53Z

...icrosoft.Identity.Client/ApiConfig/AbstractConfidentialClientAcquireTokenParameterBuilder.cs

+        ///  Modifies the token acquisition request so that the acquired token is a Signed HTTP Request (SHR) Proof-of-Possession (PoP) token, rather than a Bearer token.
+        ///  SHR PoP tokens are bound to the HTTP request and to a cryptographic key, which MSAL can manage on Windows.
+        ///  This differs from MTLS PoP tokens, which are used for Mutual TLS (mTLS) authentication. See <see href="https://aka.ms/mtls-pop"/> for mTLS PoP details.
+        ///  See <see href="https://aka.ms/msal-net-pop"/> for general PoP token information.


Suggested change

/// See <see href="https://aka.ms/msal-net-pop"/> for general PoP token information.

This is not needed because the previous link points to the exact same page.

localden · 2024-11-06T18:55:30Z

...icrosoft.Identity.Client/ApiConfig/AbstractConfidentialClientAcquireTokenParameterBuilder.cs

+        /// <returns>The builder.</returns>
+        /// <remarks>
+        /// <list type="bullet">
+        /// <item><description>The SHR PoP token is bound to the HTTP request, more specifically to the HTTP method (GET, POST, etc.) and to the URI (path and query, but not query parameters).</description></item>


Suggested change

/// <item><description>The SHR PoP token is bound to the HTTP request, more specifically to the HTTP method (GET, POST, etc.) and to the URI (path and query, but not query parameters).</description></item>

/// <item><description>The SHR PoP token is bound to the HTTP request, specifically to the HTTP method (for example, `GET` or `POST`) and to the URI path and query, excluding query parameters.</description></item>

localden · 2024-11-06T19:01:34Z

src/client/Microsoft.Identity.Client/MsalErrorMessage.cs

@@ -428,5 +428,18 @@ public static string InvalidTokenProviderResponseValue(string invalidValueName)
        public const string SetCiamAuthorityAtRequestLevelNotSupported = "Setting the CIAM authority (ex. \"{tenantName}.ciamlogin.com\") at the request level is not supported. The CIAM authority must be set during application creation";
        public const string ClaimsChallenge = "The returned error contains a claims challenge. For additional info on how to handle claims related to multifactor authentication, Conditional Access, and incremental consent, see https://aka.ms/msal-conditional-access-claims. If you are using the On-Behalf-Of flow, see https://aka.ms/msal-conditional-access-claims-obo for details.";
        public const string CryptographicError = "A cryptographic exception occurred. Possible cause: the certificate has been disposed. See inner exception for full details.";
+
+        /// <summary>
+        /// MTLS Proof of Possession (PoP) requires a region to be specified in the configuration.


Suggested change

/// MTLS Proof of Possession (PoP) requires a region to be specified in the configuration.

/// mTLS Proof-of-Possession (PoP) requires a region to be specified in the configuration.

localden · 2024-11-06T19:02:02Z

src/client/Microsoft.Identity.Client/MsalErrorMessage.cs

+
+        /// <summary>
+        /// MTLS Proof of Possession (PoP) requires a region to be specified in the configuration.
+        /// <para>What happens?</para> You have enabled MTLS PoP, but no region is specified.


Suggested change

/// <para>What happens?</para> You have enabled MTLS PoP, but no region is specified.

/// <para>What happened?</para> You enabled mTLS PoP but no region was specified.

localden · 2024-11-06T19:02:27Z

src/client/Microsoft.Identity.Client/MsalErrorMessage.cs

+        /// <summary>
+        /// MTLS Proof of Possession (PoP) requires a region to be specified in the configuration.
+        /// <para>What happens?</para> You have enabled MTLS PoP, but no region is specified.
+        /// <para>Mitigation</para> Ensure that a region is set when using MTLS PoP by configuring AzureRegion in the application.


Suggested change

/// <para>Mitigation</para> Ensure that a region is set when using MTLS PoP by configuring AzureRegion in the application.

/// <para>Mitigation</para> Ensure that a region is set when using mTLS PoP by configuring `AzureRegion` in the application.

localden · 2024-11-06T19:02:53Z

src/client/Microsoft.Identity.Client/MsalErrorMessage.cs

+        public const string MtlsPopWithoutRegion = "MTLS Proof of Possession requires a region to be specified. Please set AzureRegion in the configuration at the application level.";
+
+        /// <summary>
+        /// <para>What happens?</para> MTLS Proof of Possession requires a certificate to be configured.


Suggested change

/// <para>What happens?</para> MTLS Proof of Possession requires a certificate to be configured.

/// <para>What happened?</para> mTLS Proof-of-Possession requires a certificate to be configured.

localden · 2024-11-06T19:03:53Z

src/client/Microsoft.Identity.Client/MsalErrorMessage.cs

+
+        /// <summary>
+        /// <para>What happens?</para> MTLS Proof of Possession requires a certificate to be configured.
+        /// <para>Mitigation</para> Provide a certificate in the configuration to enable MTLS PoP. This is required to perform mutual TLS.


Suggested change

/// <para>Mitigation</para> Provide a certificate in the configuration to enable MTLS PoP. This is required to perform mutual TLS.

/// <para>Mitigation</para> Provide a certificate in the configuration to enable mTLS PoP.

very very draft

97c5d95