Adding Credential Probe logic #5116
Conversation
| @@ -152,8 +152,9 @@ private async Task<AuthenticationResult> SendTokenRequestForManagedIdentityAsync | |||
|
|
|||
| await ResolveAuthorityAsync().ConfigureAwait(false); | |||
|
|
|||
| ManagedIdentityClient managedIdentityClient = | |||
| new ManagedIdentityClient(AuthenticationRequestParameters.RequestContext); | |||
| ManagedIdentityClient managedIdentityClient = | |||
There was a problem hiding this comment.
@gladjohn - we discussed that you'd start a feature branch? The base branch for this PR is main.
|
|
||
| namespace Microsoft.Identity.Client.ManagedIdentity | ||
| { | ||
| internal class CredentialManagedIdentitySource : AbstractManagedIdentity |
There was a problem hiding this comment.
nit: maybe we just call it SLC or MSIv2 ? Credential is such an overloaded term.
There was a problem hiding this comment.
In fact, should it be ImdsV2Source? I would imagine that Arc, SF etc. will be sligthly different.
| HttpContent httpContent = request.CreateHttpContent(); | ||
|
|
||
| _logger.Info($"[Credential Probe] Sending request to {CredentialEndpoint}"); | ||
| _logger.Verbose(() => $"[Credential Probe] Request Headers: {string.Join(", ", request.Headers)}"); |
There was a problem hiding this comment.
It's a little dangerous to log headers and body. Maybe don't do it unless we really need it.
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.Error($"[Credential Probe] Exception during probe: {ex.Message}"); |
There was a problem hiding this comment.
It's not really an error is it? It is part of the normal flow. It will confuse ppl. We've did this before in regional and ppl complained that the logs are too noisy. Pls use info level.
I also recommend you do not print the full exception here, just the message should be suficient.
| { | ||
| if (response == null) | ||
| { | ||
| _logger.Error("[Credential Probe] No response received from the server."); |
|
|
||
| if (response.Body != null) | ||
| { | ||
| _logger.Verbose(() => $"[Credential Probe] Response Body: {response.Body}"); |
There was a problem hiding this comment.
avoid printing entire reponse body. it could have PII or worse, some secrets.
|
|
||
| LogResponseDetails(response); | ||
|
|
||
| return EvaluateProbeResponse(response); |
There was a problem hiding this comment.
Is it worth recording in telemetry about this? I guess we will know MSAL version and MSI source already - will this be enough?
|
|
||
| LogResponseDetails(response); | ||
|
|
||
| return EvaluateProbeResponse(response); |
There was a problem hiding this comment.
Your spec states that a retry mechanism needs to be used. This doesn't seem implemented.
| private readonly AbstractManagedIdentity _identitySource; | ||
|
|
||
| public ManagedIdentityClient(RequestContext requestContext) | ||
| internal static async Task<ManagedIdentityClient> CreateAsync(RequestContext requestContext, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
I believe the requestContext has the cancellation token, but it's ok to move it as last param. But I recommend you do not use a default.
| { | ||
| _identitySource = SelectManagedIdentitySource(requestContext); | ||
| throw new ArgumentNullException(nameof(requestContext), "RequestContext cannot be null."); |
| { | ||
| ManagedIdentitySource.ServiceFabric => ServiceFabricManagedIdentitySource.Create(requestContext), | ||
| ManagedIdentitySource.AppService => AppServiceManagedIdentitySource.Create(requestContext), | ||
| ManagedIdentitySource.MachineLearning => MachineLearningManagedIdentitySource.Create(requestContext), | ||
| ManagedIdentitySource.CloudShell => CloudShellManagedIdentitySource.Create(requestContext), | ||
| ManagedIdentitySource.AzureArc => AzureArcManagedIdentitySource.Create(requestContext), | ||
| ManagedIdentitySource.Credential => CredentialManagedIdentitySource.Create(requestContext), |
There was a problem hiding this comment.
naming: avoid using "credential". Maybe "ImdsV2" or smth
| /// <param name="cancellationToken"></param> | ||
| /// <returns></returns> | ||
| /// <exception cref="ArgumentNullException"></exception> | ||
| public static async Task<ManagedIdentitySource> GetManagedIdentitySourceAsync( |
There was a problem hiding this comment.
Logic is too complex. Why do we have 2 public methods - one with the probe and one without the probe?
| /// <param name="logger"></param> | ||
| /// <param name="cancellationToken"></param> | ||
| /// <returns></returns> | ||
| private static async Task<ManagedIdentitySource> ComputeManagedIdentitySourceAsync( |
There was a problem hiding this comment.
Please have a look at all these method names and consolidate. They are so many "select source", "compute source", etc.
|
|
||
| if (s_cachedManagedIdentitySource.HasValue) | ||
| { | ||
| return s_cachedManagedIdentitySource.Value; |
There was a problem hiding this comment.
Won't Azure SDK need a way to reset this? For their tests?
Maybe it's time we expose a "ResetCachesForTest()" method in the extensibility namespace.
Fixes #5115
Changes proposed in this request
This pull request introduces significant changes to the managed identity authentication process in the Microsoft Identity Client. The changes include adding new classes for handling credential-based managed identity, updating existing methods to support asynchronous operations, and improving logging and error handling.
Key Changes:
New Classes and Methods:
CredentialManagedIdentitySource: A new class to handle credential-based managed identity, including a factory method for creating instances and a method to create minimal valid requests.ImdsCredentialProbeManager: A new class to probe the IMDS credential endpoint and evaluate the response to determine if the endpoint is supported.Asynchronous Operations:
ManagedIdentityClient: Updated to include an asynchronous factory methodCreateAsyncand to use the newCredentialManagedIdentitySourceas a potential identity source.SendTokenRequestForManagedIdentityAsync: Updated to use the asynchronous creation method forManagedIdentityClient.Logging and Error Handling:
ManagedIdentityClientandImdsCredentialProbeManagerto provide detailed information about the managed identity source selection and probe execution. [1] [2]Public API Changes:
CredentialtoManagedIdentitySourceto indicate the use of credential-based managed identity.GetManagedIdentitySourceAsyncinManagedIdentityApplicationto detect the managed identity source in the environment.Retry logic
These changes aim to improve the flexibility and reliability of the managed identity authentication process by supporting new identity sources and enhancing the existing mechanisms.
Testing
unit tests been added
Performance impact
Before we fallback to IMDS we check for /credential endpoint so sources like App Service, Azure ARC that are determined by environment variables should not have any issues. However, for Resource providers that do not plan to use /credential endpoint and rely on the IMDS fallback logic will have one more source detection before the fallback.
Documentation