[MSI V2 Feature] Adding Credential as a Managed Identity Source #5148

gladjohn · 2025-02-18T00:59:45Z

Changes proposed in this request
This adds a new credential source to Managed Identity sources. We call it IMDSV2 source.

This pull request introduces significant changes to the Managed Identity authentication flow in the Microsoft Identity Client library. The most important changes include refactoring the ManagedIdentityAuthRequest class into an abstract class, adding new classes for handling different Managed Identity sources, and modifying the visibility of the IMsalMtlsHttpClientFactory interface.

Refactoring and new classes:

src/client/Microsoft.Identity.Client/Internal/Requests/ManagedIdentityAuthRequest.cs: Refactored ManagedIdentityAuthRequest into an abstract class to support different Managed Identity sources.
src/client/Microsoft.Identity.Client/Internal/Requests/CredentialBasedMsiAuthRequest.cs: Added CredentialManagedIdentityAuthRequest class to handle Managed Identity authentication using certificates.
src/client/Microsoft.Identity.Client/Internal/Requests/LegacyManagedIdentityAuthRequest.cs: Added LegacyManagedIdentityAuthRequest class to handle legacy Managed Identity authentication.

Modifications in existing methods:

src/client/Microsoft.Identity.Client/ApiConfig/Executors/ManagedIdentityExecutor.cs: Updated ExecuteAsync method to determine the Managed Identity source and use the appropriate authentication request class.

Visibility changes:

src/client/Microsoft.Identity.Client/AppConfig/IMsalMtlsHttpClientFactory.cs: Changed the visibility of IMsalMtlsHttpClientFactory from internal to public.

Testing
tests will be added in a new PR

Performance impact
none

Documentation
Have a separate task for docs

bgavrilMS · 2025-02-18T12:40:28Z

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

+
+            //allow only one call to the provider 
+            logger.Verbose(() => "[CredentialManagedIdentityAuthRequest] Entering acquire token for managed identity credential request semaphore.");
+            await s_semaphoreSlim.WaitAsync(cancellationToken).ConfigureAwait(false);


This semaphore should not block MSAL from getting tokens from the cache. Only from eSTS, and even them you should consider if it's worth using.

bgavrilMS · 2025-02-18T12:41:20Z

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

+                    }
+                    else
+                    {
+                        logger.Verbose(() => "[CredentialManagedIdentityAuthRequest] Getting Access token from cache ...");


Suggested change

logger.Verbose(() => "[CredentialManagedIdentityAuthRequest] Getting Access token from cache ...");

logger.Verbose(() => "[CredentialManagedIdentityAuthRequest] Returning Access token from cache ...");

bgavrilMS · 2025-02-18T12:41:45Z

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

+                    !string.IsNullOrEmpty(AuthenticationRequestParameters.Claims) ||
+                    AuthenticationRequestParameters.RequestContext.ApiEvent.CacheInfo == CacheRefreshReason.ProactivelyRefreshed)
+                {
+                    authResult = await GetAccessTokenFromTokenEndpointAsync(cancellationToken, logger).ConfigureAwait(false);


log statement would be good here to show which condition was true.

bgavrilMS · 2025-02-18T12:43:05Z

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

+                MsalTokenResponse msalTokenResponse = await client
+                        .GetTokenAsync(tokenUrl,
+                        AuthenticationRequestParameters.RequestContext,
+                        true,


nit: add the param name for booleans, to make the code easier to read

bgavrilMS · 2025-02-18T12:43:44Z

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

+            }
+            catch (MsalClientException ex)
+            {
+                logger.Verbose(() => $"[CredentialManagedIdentityAuthRequest] Caught an exception. {ex.Message}");


nit: Doesn't it get logged higher up?

bgavrilMS · 2025-02-18T12:44:19Z

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

+            }
+            catch (MsalServiceException ex)
+            {
+                logger.Verbose(() => $"[CredentialManagedIdentityAuthRequest] Caught an exception. {ex.Message}. Error Code : {ex.ErrorCode} Status Code : {ex.StatusCode}");


Suggested change

logger.Verbose(() => $"[CredentialManagedIdentityAuthRequest] Caught an exception. {ex.Message}. Error Code : {ex.ErrorCode} Status Code : {ex.StatusCode}");

logger.Verbose(() => $"[CredentialManagedIdentityAuthRequest] A service exception occured. {ex.Message}. Error Code : {ex.ErrorCode} Status Code : {ex.StatusCode}");

bgavrilMS · 2025-02-18T12:45:05Z

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

+            }
+            catch (Exception e) when (e is not MsalServiceException)
+            {
+                logger.Error($"[CredentialManagedIdentityAuthRequest] Exception: {e.Message}");


Perfer to log the entire exception. The logger is responsible for outputting it.

bgavrilMS · 2025-02-18T12:46:17Z

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


        protected override KeyValuePair<string, string>? GetCcsHeader(IDictionary<string, string> additionalBodyParameters)
        {
            return null;
        }
+
+        // Override method to return a sorted set of scopes based on the input set.
+        protected override SortedSet<string> GetOverriddenScopes(ISet<string> inputScopes)


Why? There's only 1 scope possible? Why do we need to bring this whole "collection of scope" concept from user tokens to S2S tokens?

bgavrilMS · 2025-02-18T12:46:30Z

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

+            )
+        {
+            // Create a Managed Identity request dynamically using the provided resource.
+            string resource = GetOverriddenScopes(AuthenticationRequestParameters.Scope).AsSingleString();


There is no "overriden" scopes?

bgavrilMS · 2025-02-18T12:47:44Z

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

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


Do you actually use anything from this base class? Would it be more appropriate to inherit from RequestBase instead?

bgavrilMS · 2025-02-18T12:48:22Z

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

@@ -30,51 +26,52 @@ public ManagedIdentityAuthRequest(

        protected override async Task<AuthenticationResult> ExecuteAsync(CancellationToken cancellationToken)
        {
-            AuthenticationResult authResult = null;
+            if (AuthenticationRequestParameters.Scope == null || AuthenticationRequestParameters.Scope.Count == 0)


This should be thrown earlier, when the request object is "built".

bgavrilMS · 2025-02-18T12:49:53Z

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

+        private readonly CancellationToken _cancellationToken;
+        internal const string TimeoutError = "[Managed Identity] Authentication unavailable. The request to the managed identity endpoint timed out.";
+
+        // New constructor that accepts a ManagedIdentityRequest


How can it be new if it's the first version of this class?

bgavrilMS · 2025-02-18T12:51:27Z

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

+        /// <param name="credentialResponse">The CredentialResponse to be validated.</param>
+        private void ValidateCredentialResponse(ManagedIdentityCredentialResponse credentialResponse)
+        {
+            var errorMessages = new List<string>();


Use a StringBuilder instead.

bgavrilMS · 2025-02-18T12:55:21Z

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

+            string resource = GetOverriddenScopes(AuthenticationRequestParameters.Scope).AsSingleString();
+            ManagedIdentityRequest miRequest = CreateManagedIdentityRequest(resource);
+
+            var msiCredentialService = new ManagedIdentityCredentialService(


Generally it's a advised to not construct new objects in the main code path, but only in ctors. It will make unit testing easier.

Conceptually, there should be only 1 ManagedIdentityCredentialService which can serve all the requests? There is no need to pass around all this data through constructors.

bgavrilMS · 2025-02-18T12:59:05Z

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

+    {
+        private readonly ManagedIdentityRequest _request;
+        private readonly X509Certificate2 _bindingCertificate;
+        private readonly RequestContext _requestContext;


Try to avoid passing in "RequesetContext" around. It's starting to become a https://en.wikipedia.org/wiki/God_object

A better pattern is to pass in only what is needed - the logger and the httpManager in this case.

bgavrilMS · 2025-02-18T13:00:56Z

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

@@ -0,0 +1,169 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.


Any opportunitiers to log client telemetry?

bgavrilMS · 2025-02-18T13:05:02Z

src/client/Microsoft.Identity.Client/Utils/ManagedIdentityCertificateManager.cs

+        /// Not for final release. For platform certificates, this is a no-op.
+        /// </summary>
+        /// <returns>The renewed certificate.</returns>
+        public static X509Certificate2 ForceUpdateInMemoryCertificate()


How is Azure SDK going to call an intenal API?

bgavrilMS · 2025-02-18T13:07:46Z

src/client/Microsoft.Identity.Client/Utils/ManagedIdentityCertificateManager.cs

+        /// <summary>
+        /// Event triggered when a new certificate is created.
+        /// </summary>
+        internal static event Action<X509Certificate2> CertificateUpdated;


This is again internal, so Azure SDK can't use. YOu need to use a public class instead.

bgavrilMS · 2025-02-18T13:10:42Z

src/client/Microsoft.Identity.Client/Utils/ManagedIdentityCertificateManager.cs

+        /// <returns>An X509Certificate2 valid for 90 days.</returns>
+        public static X509Certificate2 GetOrCreateCertificate(bool forceUpdate = false)
+        {
+            lock (s_lock)


Do we really need to block on reading the cert? Can you use a https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-threading-readerwriterlockslim instead?

bgavrilMS · 2025-02-18T13:11:40Z

src/client/Microsoft.Identity.Client/Utils/ManagedIdentityCertificateManager.cs

+
+                bool isWindows = DesktopOsHelper.IsWindows();
+
+                X509KeyStorageFlags flags = isWindows


Pls add some comments on WHY you used these flags.

bgavrilMS

PRs should have tests. This will help reviewers go through your code. And the final version of the code will be different due to test hooks.

inital

59a876e

gladjohn requested a review from a team as a code owner February 18, 2025 00:59

bgavrilMS reviewed Feb 18, 2025

View reviewed changes

bgavrilMS requested changes Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MSI V2 Feature] Adding Credential as a Managed Identity Source #5148

[MSI V2 Feature] Adding Credential as a Managed Identity Source #5148

gladjohn commented Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025 •

edited

Loading

bgavrilMS Feb 18, 2025

bgavrilMS Feb 18, 2025

bgavrilMS left a comment

	logger.Verbose(() => "[CredentialManagedIdentityAuthRequest] Getting Access token from cache ...");
	logger.Verbose(() => "[CredentialManagedIdentityAuthRequest] Returning Access token from cache ...");

	logger.Verbose(() => $"[CredentialManagedIdentityAuthRequest] Caught an exception. {ex.Message}. Error Code : {ex.ErrorCode} Status Code : {ex.StatusCode}");
	logger.Verbose(() => $"[CredentialManagedIdentityAuthRequest] A service exception occured. {ex.Message}. Error Code : {ex.ErrorCode} Status Code : {ex.StatusCode}");

		@@ -0,0 +1,169 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.


		bool isWindows = DesktopOsHelper.IsWindows();

		X509KeyStorageFlags flags = isWindows

[MSI V2 Feature] Adding Credential as a Managed Identity Source #5148

Are you sure you want to change the base?

[MSI V2 Feature] Adding Credential as a Managed Identity Source #5148

Conversation

gladjohn commented Feb 18, 2025

Refactoring and new classes:

Modifications in existing methods:

Visibility changes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS left a comment

Choose a reason for hiding this comment

bgavrilMS Feb 18, 2025 •

edited

Loading