IMDS Source Detection Logic Improvement #5602
Conversation
… back to imdsv1 when determining source.
… tests in MangedIdentityTests.cs
src/client/Microsoft.Identity.Client/ManagedIdentity/ImdsManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentityClient.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/ManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
|
|
||
| var queryParams = ImdsQueryParamsHelper(requestContext, apiVersionQueryParam, imdsApiVersion); | ||
|
|
||
| var headers = new Dictionary<string, string> |
There was a problem hiding this comment.
would be good to have some comments here like,
probe deliberately omits the Metadata: true header and then treats 400 as IMDS present
| return false; | ||
| } | ||
|
|
||
| if (response.StatusCode == HttpStatusCode.BadRequest) |
There was a problem hiding this comment.
I’d add a short comment near the StatusCode == BadRequest check explaining why 400 is treated as success, just for future reference.
| namespace Microsoft.Identity.Client.Http.Retry | ||
| { | ||
| internal class CsrMetadataProbeRetryPolicy : ImdsRetryPolicy | ||
| internal class ImdsProbeRetryPolicy : ImdsRetryPolicy |
There was a problem hiding this comment.
consider renaming the file to ImdsProbeRetryPolicy.cs
| RequestContext requestContext, | ||
| bool isMtlsPopRequested) | ||
| bool isMtlsPopRequested, | ||
| bool noImdsV2 = false) |
There was a problem hiding this comment.
is this used anywhere? noImdsV2
| ManagedIdentitySource.ImdsV2 => ImdsV2ManagedIdentitySource.Create(requestContext), | ||
| _ => new ImdsManagedIdentitySource(requestContext) | ||
| ManagedIdentitySource.Imds => ImdsManagedIdentitySource.Create(requestContext), | ||
| _ => throw new MsalServiceException(MsalError.ManagedIdentityAllSourcesUnavailable, MsalErrorMessage.ManagedIdentityAllSourcesUnavailable) |
There was a problem hiding this comment.
is this really a service exception?
There was a problem hiding this comment.
MsalClientException is more appropriate here.
| Assert.ThrowsException<MsalServiceException>(() => | ||
| CsrValidator.ParseRawCsr(badCert)); | ||
| } | ||
| #endregion CSR Generation Tests |
There was a problem hiding this comment.
Are there tests to validate GetManagedIdentitySourceAsync does not probe again, when the application already returned it as None?
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| namespace Microsoft.Identity.Client.ManagedIdentity.V2 |
There was a problem hiding this comment.
can this be in Microsoft.Identity.Client.ManagedIdentity namespace?
| } | ||
| else | ||
| { | ||
| requestContext.Logger.Info("[Managed Identity] Mtls Pop was not requested; skipping ImdsV2 probe."); |
There was a problem hiding this comment.
We are really not skipping probe? can we actually do that?
There was a problem hiding this comment.
what I mean is, "[Managed Identity] Mtls Pop was not requested; skipping ImdsV2 probe." is based on the probe result. Can we actually skip the probe itself?
| ManagedIdentityApplication mi = miBuilder.Build() as ManagedIdentityApplication; | ||
|
|
||
| Assert.AreEqual(expectedManagedIdentitySource, await mi.GetManagedIdentitySourceAsync().ConfigureAwait(false)); | ||
| if (managedIdentitySource == ManagedIdentitySource.ImdsV2) |
There was a problem hiding this comment.
Do we have a test where we first get a bearer and then request a POP token?
|
|
||
| try | ||
| { | ||
| using (var timeoutCts = new CancellationTokenSource(TimeSpan.FromSeconds(1))) |
There was a problem hiding this comment.
A better pattern for timeout is to let the caller (Azure SDK) provide it, via CancellationToken. So your async method GetSourceAsync() needs to take a parameetr GetSourceAsync(CancellationToken token) and you need to propagate this to the HTTP calls.
There was a problem hiding this comment.
If we also want MSAL to provide a default timeout, we can combine our timeout with the one from the caller. But I think we are ok without a timeout of our own? We can always add one later.
Implemented ImdsV2 and ImdsV1 probing
Implements #5594