From 59ae54151a86202ac4cb6eb871b3991969132912 Mon Sep 17 00:00:00 2001 From: Christopher Scott Date: Wed, 23 Oct 2024 14:36:01 -0500 Subject: [PATCH 1/2] Fix scenario where IMDS probe succeeds, but MI is unavailable --- sdk/identity/Azure.Identity/CHANGELOG.md | 4 +-- .../src/ImdsManagedIdentityProbeSource.cs | 26 ++++++++++++-- .../tests/ImdsManagedIdentitySourceTests.cs | 2 +- .../tests/ManagedIdentityCredentialTests.cs | 36 ++++++++++++++++++- 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/sdk/identity/Azure.Identity/CHANGELOG.md b/sdk/identity/Azure.Identity/CHANGELOG.md index ae0797ba603db..6e941b289c149 100644 --- a/sdk/identity/Azure.Identity/CHANGELOG.md +++ b/sdk/identity/Azure.Identity/CHANGELOG.md @@ -7,8 +7,8 @@ ### Breaking Changes ### Bugs Fixed -- Fixed an issue that prevented `ManagedIdentityCredential` from attempting to detect if Workload Identity is enabled in the current environment. [#46653](https://github.com/Azure/azure-sdk-for-net/issues/46653) -- Fixed an issue that prevented `DefaultAzureCredential` from progressing past `ManagedIdentityCredential` in some scenarios where the identity was not available. [#46709](https://github.com/Azure/azure-sdk-for-net/issues/46709) +- Fixed a regression that prevented `ManagedIdentityCredential` from attempting to detect if Workload Identity is enabled in the current environment. [#46653](https://github.com/Azure/azure-sdk-for-net/issues/46653) +- Fixed regression that prevented `DefaultAzureCredential` from progressing past `ManagedIdentityCredential` in some scenarios where the identity was not available. [#46709](https://github.com/Azure/azure-sdk-for-net/issues/46709) ### Other Changes diff --git a/sdk/identity/Azure.Identity/src/ImdsManagedIdentityProbeSource.cs b/sdk/identity/Azure.Identity/src/ImdsManagedIdentityProbeSource.cs index 7d2b8f0ed6f43..6c44cf7f9424d 100644 --- a/sdk/identity/Azure.Identity/src/ImdsManagedIdentityProbeSource.cs +++ b/sdk/identity/Azure.Identity/src/ImdsManagedIdentityProbeSource.cs @@ -6,6 +6,7 @@ using System.Threading; using System.Threading.Tasks; using Azure.Core; +using Microsoft.Identity.Client; namespace Azure.Identity { @@ -22,6 +23,7 @@ internal class ImdsManagedIdentityProbeSource : ManagedIdentitySource internal const string TimeoutError = "ManagedIdentityCredential authentication unavailable. The request to the managed identity endpoint timed out."; internal const string GatewayError = "ManagedIdentityCredential authentication unavailable. The request failed due to a gateway error."; internal const string AggregateError = "ManagedIdentityCredential authentication unavailable. Multiple attempts failed to obtain a token from the managed identity endpoint."; + internal const string UnknownError = "ManagedIdentityCredential authentication unavailable. An unexpected error has occurred."; private readonly ManagedIdentityId _managedIdentityId; private readonly Uri _imdsEndpoint; @@ -97,6 +99,7 @@ protected override HttpMessage CreateHttpMessage(Request request) public async override ValueTask AuthenticateAsync(bool async, TokenRequestContext context, CancellationToken cancellationToken) { + bool continueIMDSRequestAfterProbe; try { return await base.AuthenticateAsync(async, context, cancellationToken).ConfigureAwait(false); @@ -127,12 +130,29 @@ public async override ValueTask AuthenticateAsync(bool async, Token catch (ProbeRequestResponseException) { // This was an expected response from the IMDS endpoint without the Metadata header set. - // Re-issue the request (CreateRequest will add the appropriate header). + // Fall-through to the code below to re-issue the request through MsalManagedIdentityClient. + continueIMDSRequestAfterProbe = true; + } + + if (continueIMDSRequestAfterProbe) + { + try + { #pragma warning disable AZC0110 // DO NOT use await keyword in possibly synchronous scope. - var authResult = await _client.AcquireTokenForManagedIdentityAsync(context, cancellationToken).ConfigureAwait(false); - return authResult.ToAccessToken(); + var authResult = await _client.AcquireTokenForManagedIdentityAsync(context, cancellationToken).ConfigureAwait(false); #pragma warning restore AZC0110 // DO NOT use await keyword in possibly synchronous scope. + return authResult.ToAccessToken(); + } + catch (MsalServiceException e) + { + if (e.Message.Contains("unavailable")) + { + throw new CredentialUnavailableException(IdentityUnavailableError, e); + } + throw new CredentialUnavailableException(UnknownError, e); + } } + throw new CredentialUnavailableException(UnknownError); } protected override async ValueTask HandleResponseAsync(bool async, TokenRequestContext context, HttpMessage message, CancellationToken cancellationToken) diff --git a/sdk/identity/Azure.Identity/tests/ImdsManagedIdentitySourceTests.cs b/sdk/identity/Azure.Identity/tests/ImdsManagedIdentitySourceTests.cs index 02b62ab3e2c0a..8882324a81b46 100644 --- a/sdk/identity/Azure.Identity/tests/ImdsManagedIdentitySourceTests.cs +++ b/sdk/identity/Azure.Identity/tests/ImdsManagedIdentitySourceTests.cs @@ -142,7 +142,7 @@ public void DefaultAzureCredentialRetryBehaviorIsOverriddenWithOptions() var cred = new DefaultAzureCredential(credOptions); - Assert.ThrowsAsync(async () => await cred.GetTokenAsync(new(new[] { "test" }))); + Assert.ThrowsAsync(async () => await cred.GetTokenAsync(new(new[] { "test" }))); var expectedTimeouts = new TimeSpan?[] { TimeSpan.FromSeconds(1), null, null, null, null, null, null, null, null }; CollectionAssert.AreEqual(expectedTimeouts, networkTimeouts); diff --git a/sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialTests.cs b/sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialTests.cs index 09a1ccfd33817..f7e8167063154 100644 --- a/sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialTests.cs +++ b/sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialTests.cs @@ -447,6 +447,37 @@ public void VerifyImdsRequestFailureWithValidJsonIdentityNotFoundErrorThrowsCUE( } } + [NonParallelizable] + [Test] + [TestCase(true)] + [TestCase(false)] + public void VerifyImdsProbeRequestSuccessWithIdentityNotFoundErrorThrowsCUE(bool isChained) + { + using var environment = new TestEnvVar(new() { { "MSI_ENDPOINT", null }, { "MSI_SECRET", null }, { "IDENTITY_ENDPOINT", null }, { "IDENTITY_HEADER", null }, { "AZURE_POD_IDENTITY_AUTHORITY_HOST", null } }); + + var mockTransport = new MockTransport(req => + { + if (!req.Headers.TryGetValue(ImdsManagedIdentityProbeSource.metadataHeaderName, out _)) + { + return CreateResponse(400, """{"error":"invalid_request","error_description":"Required metadata header not specified"}"""); + } + return CreateResponse(400, """{"error":"invalid_request","error_description":"Identity not found"}"""); + }); + var options = new TokenCredentialOptions() { Transport = mockTransport, IsChainedCredential = isChained }; + var pipeline = CredentialPipeline.GetInstance(options); + + ManagedIdentityCredential credential = InstrumentClient(new ManagedIdentityCredential("mock-client-id", pipeline, options)); + if (isChained) + { + var ex = Assert.ThrowsAsync(async () => await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Alternate))); + Assert.That(ex.Message, Does.Contain(ImdsManagedIdentityProbeSource.IdentityUnavailableError)); + } + else + { + var ex = Assert.ThrowsAsync(async () => await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Alternate))); + } + } + [NonParallelizable] [Test] [TestCase(502, ImdsManagedIdentitySource.GatewayError)] @@ -476,7 +507,10 @@ public async Task VerifyIMDSRequestWithPodIdentityEnvVarMockAsync(string clientI using var environment = new TestEnvVar(new() { { "MSI_ENDPOINT", null }, { "MSI_SECRET", null }, { "IDENTITY_ENDPOINT", null }, { "IDENTITY_HEADER", null }, { "AZURE_POD_IDENTITY_AUTHORITY_HOST", "https://mock.podid.endpoint/" } }); var response = CreateMockResponse(200, ExpectedToken); - var mockTransport = new MockTransport(response); + var mockTransport = new MockTransport(req => + { + return response; + }); var options = new TokenCredentialOptions() { Transport = mockTransport }; var pipeline = CredentialPipeline.GetInstance(options); From d2c7b3cd519ec2a58f4dfcfe80b3d22f45151496 Mon Sep 17 00:00:00 2001 From: Christopher Scott Date: Wed, 23 Oct 2024 15:26:42 -0500 Subject: [PATCH 2/2] Update sdk/identity/Azure.Identity/CHANGELOG.md Co-authored-by: Scott Schaab --- sdk/identity/Azure.Identity/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/identity/Azure.Identity/CHANGELOG.md b/sdk/identity/Azure.Identity/CHANGELOG.md index 6e941b289c149..78c9fc7a3877f 100644 --- a/sdk/identity/Azure.Identity/CHANGELOG.md +++ b/sdk/identity/Azure.Identity/CHANGELOG.md @@ -8,7 +8,7 @@ ### Bugs Fixed - Fixed a regression that prevented `ManagedIdentityCredential` from attempting to detect if Workload Identity is enabled in the current environment. [#46653](https://github.com/Azure/azure-sdk-for-net/issues/46653) -- Fixed regression that prevented `DefaultAzureCredential` from progressing past `ManagedIdentityCredential` in some scenarios where the identity was not available. [#46709](https://github.com/Azure/azure-sdk-for-net/issues/46709) +- Fixed a regression that prevented `DefaultAzureCredential` from progressing past `ManagedIdentityCredential` in some scenarios where the identity was not available. [#46709](https://github.com/Azure/azure-sdk-for-net/issues/46709) ### Other Changes