Add CAE capability for Managed Identity flows #4719

gladjohn · 2024-04-16T19:11:17Z

Adds CAE capability to managed identity flows on Windows VMs
Unit tests have been added

…reAD/microsoft-authentication-library-for-dotnet into gladjohn/slc_cae_pub_preview

build/platform_and_feature_flags.props

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

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

src/client/Microsoft.Identity.Client/Platforms/Features/SLC/KeyMaterialManager.cs

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

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

src/client/Microsoft.Identity.Client/Credential/ManagedIdentityCredentialResponse.cs

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

src/client/Microsoft.Identity.Client/Credential/ManagedIdentityCredentialResponse.cs

src/client/Microsoft.Identity.Client/Credential/IManagedIdentityCredentialResponse.cs

bgavrilMS · 2024-04-22T22:07:23Z

src/client/Microsoft.Identity.Client/Credential/ManagedIdentityCredentialResponse.cs

+        /// <returns></returns>
+        private static string GetCredentialPayload(X509Certificate2 x509Certificate2)
+        {
+            string certificateBase64 = Convert.ToBase64String(x509Certificate2.Export(X509ContentType.Cert));


consider caching this (as a member just like _bindingCertificate)

Credential is short lived, 10 mins, and the token itself is longer than the credential, so not worth caching it

src/client/Microsoft.Identity.Client/Credential/ManagedIdentityCredentialResponse.cs

localden · 2024-05-07T05:30:24Z

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

+            RequestBase handler = null;
+
+            // May or may not be initialized, depending on the state of the Azure resource
+            handler = SlcManagedIdentityAuthRequest.TryCreate(


Do we want to use a more generic name here, since SLC is an internal acronym?

localden · 2024-05-07T05:40:06Z

src/client/Microsoft.Identity.Client/Extensibility/ManagedIdentityApplicationExtensions.cs

+        /// Used to determine if managed identity is able to handle claims.
+        /// </summary>
+        /// <returns>Boolean indicating if Claims is supported</returns>
+        public static bool IsClaimsSupportedByClient(this IManagedIdentityApplication app)


How about consistently using Is prefix? IsClientSupportingClaims?

cc: @gladjohn @bgavrilMS @pmaytak

localden · 2024-05-07T05:42:25Z

src/client/Microsoft.Identity.Client/Internal/Constants.cs

@@ -43,6 +43,7 @@ internal static class Constants
        public const string ManagedIdentityResourceId = "mi_res_id";
        public const string ManagedIdentityDefaultClientId = "system_assigned_managed_identity";
        public const string ManagedIdentityDefaultTenant = "managed_identity";
+        public const string CredentialEndpoint = "http://169.254.169.254/metadata/identity/credential?cred-api-version=1.0";


Can we make the name more explicit here, like ImdsCredentialEndpoint?

localden · 2024-05-07T05:45:43Z

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

+
+namespace Microsoft.Identity.Client.Internal.Requests
+{
+    internal class SlcManagedIdentityAuthRequest : ManagedIdentityAuthRequest


Same comment as earlier - we probably should not expose the SLC name in the API surface. How about ManagedIdentityAuthRequestion (if the other one is legacy)?

localden · 2024-05-07T05:47:15Z

src/client/Microsoft.Identity.Client/ManagedIdentity/IManagedIdentityCredentialServices.cs

+{
+    internal interface IManagedIdentityCredentialService
+    {
+        Task<SlcCredentialResponse> GetCredentialAsync();


Another instance of SLC term usage 😀

localden · 2024-05-07T05:48:12Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityCredentialServices.cs

+        private readonly X509Certificate2 _bindingCertificate;
+        private readonly RequestContext _requestContext;
+        private readonly CancellationToken _cancellationToken;
+        internal const string TimeoutError = "[Managed Identity] Authentication unavailable. The request to the managed identity endpoint timed out.";


Suggested change

internal const string TimeoutError = "[Managed Identity] Authentication unavailable. The request to the managed identity endpoint timed out.";

internal const string TimeoutError = "[Managed Identity] Authentication endpoint unavailable. The request to the Managed Identity endpoint timed out.";

localden · 2024-05-07T05:49:52Z

src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentitySource.cs

+        /// <summary>
+        /// The source to acquire token for managed identity is Slc Credential Endpoint.
+        /// </summary>
+        SlcCredential


SLC already contains the word "credential" - maybe ShortLivedCredential? I still think we shouldn't expose the name in the API, but this would make it more readable.

localden · 2024-05-07T05:50:36Z

src/client/Microsoft.Identity.Client/ManagedIdentity/SlcCredentialResponse.cs

+{
+    [JsonObject]
+    [Preserve(AllMembers = true)]
+    internal class SlcCredentialResponse


localden · 2024-05-07T05:51:07Z

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

+        /// Used to determine if managed identity is able to handle claims.
+        /// </summary>
+        /// <returns>Boolean indicating if Claims is supported</returns>
+        public static bool IsClaimsSupportedByClient()


Suggest IsClientSupportingClaims

localden · 2024-05-07T05:52:43Z

src/client/Microsoft.Identity.Client/Platforms/Features/SLC/KeyGuardProxy.cs

+using Microsoft.Identity.Client.ManagedIdentity;
+using Microsoft.Identity.Client.PlatformsCommon.Interfaces;
+
+namespace Microsoft.Identity.Client.Platforms.Features.SLC


SLC in the namespace 😀

pmaytak · 2024-05-07T06:58:31Z

@gladjohn Please associate an appropriate issue with this PR.

trwalke · 2024-05-08T06:05:25Z

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

-            string instanceDiscoveryEndpoint =  UriBuilderExtensions.GetHttpsUriWithOptionalPort(
-                $"https://{discoveryHost}/common/discovery/instance",
+            string instanceDiscoveryEndpoint = UriBuilderExtensions.GetHttpsUriWithOptionalPort(
+                string.Format(


I reccomend keeping the string interpolation because it is generally faster and more efficient.
Invariant culture shouldnt be needed for uri's right?

trwalke · 2024-05-08T06:09:05Z

src/client/Microsoft.Identity.Client/Instance/Region/RegionManager.cs

-                            HttpResponse response = await _httpManager.SendGetAsync(imdsUri, headers, logger, retry: false, cancellationToken: GetCancellationToken(requestCancellationToken))
-                                .ConfigureAwait(false);
+                            HttpResponse response = await _httpManager.SendRequestAsync(
+                                 imdsUri,


Why only some are using the 'parameterName: value' format. When passing in many variables like this, it can be helpful in figuring out what is what.

trwalke · 2024-05-08T06:18:22Z

src/client/Microsoft.Identity.Client/Instance/Region/RegionManager.cs

-                                response = await _httpManager.SendGetAsync(BuildImdsUri(apiVersion), headers, logger, retry: false, cancellationToken: GetCancellationToken(requestCancellationToken))
-                                    .ConfigureAwait(false); // Call again with updated version
+                                response = await _httpManager.SendRequestAsync(
+                                   imdsUri,


Same as the above comment

trwalke · 2024-05-08T07:01:25Z

.../Microsoft.Identity.Client/Internal/ClientCredential/CertificateAndClaimsClientCredential.cs

-            X509Certificate2 certificate,
-            IDictionary<string, string> claimsToSign, 
-            bool appendDefaultClaims)
+        public CertificateAndClaimsClientCredential(X509Certificate2 certificate, IDictionary<string, string> claimsToSign, bool appendDefaultClaims)


Why remove the stacking of these parameters? Do we have any general guidance on this?
Generally, if the parameters line has a bunch of characters, I always stack them.

trwalke · 2024-05-08T07:29:12Z

src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs

+
+        private static HttpClient CreateMtlsHttpClient(X509Certificate2 bindingCertificate)
+        {
+            if (s_httpClient.Count > 1000)


Seems like you already know the the cache is above 1000 here. Is there a reason to check again inside the CheckAndClearCache() method?

bgavrilMS · 2024-05-08T10:09:55Z

src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs

+            //Create an HttpClientHandler and configure it to use the client certificate
+            HttpClientHandler handler = new();
+
+#if SUPPORTS_MTLS


I assume this code path should never be executed on TFM that don't support MTLS? In that case, there should be an else throw exception.

bgavrilMS · 2024-05-08T10:10:29Z

src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs

    {
        //Please see (https://aka.ms/msal-httpclient-info) for important information regarding the HttpClient.
-        private static readonly Lazy<HttpClient> s_httpClient = new Lazy<HttpClient>(InitializeClient);
+        private static readonly ConcurrentDictionary<string, HttpClient> s_httpClient = new ConcurrentDictionary<string, HttpClient>();


this is not a good name for a dictionary

bgavrilMS · 2024-05-08T10:11:36Z

src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs

+
+        private static HttpClient CreateMtlsHttpClient(X509Certificate2 bindingCertificate)
+        {
+            if (s_httpClient.Count > 1000)


it's better to check size of cache on write, not on read. There are many many more read operations than write operations.

bgavrilMS · 2024-05-08T10:19:39Z

src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs

+            {
+                if (s_httpClient.Count > 1000)
+                {
+                    s_httpClient.Clear();


I don't know about this. If there are pending requests on other threads they will be cancelled. The better solution would be to use an LRU cache, but that's probably a bit overkill. I'd leave it and rely on internal retries.

Can you log the size?

This reverts commit 0fa96f7.

GladwinJohnson and others added 7 commits April 16, 2024 12:09

initial commit

d145191

Merge branch 'main' into gladjohn/slc_cae_pub_preview

ab33420

test fix

d78eb83

Merge branch 'gladjohn/slc_cae_pub_preview' of https://github.com/Azu…

3f8c9ca

…reAD/microsoft-authentication-library-for-dotnet into gladjohn/slc_cae_pub_preview

unit tests

88596ce

Merge branch 'main' into gladjohn/slc_cae_pub_preview

cf4fe68

fix

d72353e

neha-bhargava reviewed Apr 18, 2024

View reviewed changes

build/platform_and_feature_flags.props Show resolved Hide resolved