Fix instance metadata validation error (#32520)

christothes · web-flow · commit edfcf84218fc · 2022-12-02T11:52:37.000-06:00
* Fix instance metadata validation error
diff --git a/sdk/identity/Azure.Identity/CHANGELOG.md b/sdk/identity/Azure.Identity/CHANGELOG.md
@@ -9,6 +9,7 @@
 ### Bugs Fixed
 - Fixed error message parsing in `AzureCliCredential` which would misinterpret AAD errors with the need to login with `az login`.
 - `ManagedIdentityCredential` will no longer fail when a response received from the endpoint is invalid JSON. It now treats this scenario as if the credential is unavailable.
+- Fixed an issue when using `ManagedIdentityCredential` in combination with authorities other than Azure public cloud that resulted in a bogus instance metadata validation error. [#32498](https://github.com/Azure/azure-sdk-for-net/issues/32498)
 
 ### Other Changes
 
diff --git a/sdk/identity/Azure.Identity/src/MsalConfidentialClient.cs b/sdk/identity/Azure.Identity/src/MsalConfidentialClient.cs
@@ -2,7 +2,6 @@
 // Licensed under the MIT License.
 
 using System;
-using System.Runtime.CompilerServices;
 using System.Security.Cryptography.X509Certificates;
 using System.Threading;
 using System.Threading.Tasks;
@@ -20,6 +19,7 @@ internal class MsalConfidentialClient : MsalClientBase<IConfidentialClientApplic
         private readonly Func<string> _assertionCallback;
         private readonly Func<CancellationToken, Task<string>> _asyncAssertionCallback;
         private readonly Func<AppTokenProviderParameters, Task<AppTokenProviderResult>> _appTokenProviderCallback;
+        private readonly Uri _authority;
 
         internal string RedirectUrl { get; }
 
@@ -59,6 +59,7 @@ public MsalConfidentialClient(CredentialPipeline pipeline, string tenantId, stri
             : base(pipeline, tenantId, clientId, options)
         {
             _appTokenProviderCallback = appTokenProviderCallback;
+            _authority = options?.AuthorityHost ?? AzureAuthorityHosts.AzurePublicCloud;
         }
 
         internal string RegionalAuthority { get; } = EnvironmentVariables.AzureRegionalAuthorityName;
@@ -69,11 +70,12 @@ protected override async ValueTask<IConfidentialClientApplication> CreateClientA
                 .WithHttpClientFactory(new HttpPipelineClientFactory(Pipeline.HttpPipeline))
                 .WithLogging(LogMsal, enablePiiLogging: IsPiiLoggingEnabled);
 
-            //special case for using appTokenProviderCallback, authority validation and instance metadata discovery should be disabled since we're not calling the STS
+            // Special case for using appTokenProviderCallback, authority validation and instance metadata discovery should be disabled since we're not calling the STS
+            // The authority matches the one configured in the CredentialOptions.
             if (_appTokenProviderCallback != null)
             {
                 confClientBuilder.WithAppTokenProvider(_appTokenProviderCallback)
-                    .WithAuthority(Pipeline.AuthorityHost.AbsoluteUri, TenantId, false)
+                    .WithAuthority(_authority.AbsoluteUri, TenantId, false)
                     .WithInstanceDiscoveryMetadata(s_instanceMetadata);
             }
             else
@@ -102,7 +104,7 @@ protected override async ValueTask<IConfidentialClientApplication> CreateClientA
                 confClientBuilder.WithCertificate(clientCertificate);
             }
 
-            if (!string.IsNullOrEmpty(RegionalAuthority))
+            if (_appTokenProviderCallback == null && !string.IsNullOrEmpty(RegionalAuthority))
             {
                 confClientBuilder.WithAzureRegion(RegionalAuthority);
             }
diff --git a/sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialImdsLiveTests.cs b/sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialImdsLiveTests.cs
@@ -3,11 +3,7 @@
 
 using System;
 using System.Collections.Generic;
-using System.Reflection;
-using System.Text;
 using System.Threading.Tasks;
-using Azure.Core.TestFramework;
-using Azure.Identity.Tests.Mock;
 using Azure.Security.KeyVault.Secrets;
 using NUnit.Framework;
 
diff --git a/sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialTests.cs b/sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialTests.cs
@@ -11,6 +11,7 @@
 using System.Web;
 using Azure.Core;
 using Azure.Core.Diagnostics;
+using Azure.Core.Pipeline;
 using Azure.Core.TestFramework;
 using Azure.Identity.Tests.Mock;
 using Microsoft.AspNetCore.Http;
@@ -100,6 +101,73 @@ public async Task VerifyImdsRequestWithClientIdMockAsync()
             Assert.AreEqual("true", metadataValue);
         }
 
+        [NonParallelizable]
+        [Test]
+        [TestCase(null)]
+        [TestCase("Auto-Detect")]
+        [TestCase("eastus")]
+        [TestCase("westus")]
+        public async Task VerifyImdsRequestWithClientIdAndRegionalAuthorityNameMockAsync(string regionName)
+        {
+            using var environment = new TestEnvVar(new() { {"AZURE_REGIONAL_AUTHORITY_NAME", regionName}, {"MSI_ENDPOINT", null }, { "MSI_SECRET", null }, { "IDENTITY_ENDPOINT", null }, { "IDENTITY_HEADER", null }, { "AZURE_POD_IDENTITY_AUTHORITY_HOST", null } });
+
+            var response = CreateMockResponse(200, ExpectedToken);
+            var mockTransport = new MockTransport(response);
+            var options = new TokenCredentialOptions() { Transport = mockTransport };
+            var pipeline = CredentialPipeline.GetInstance(options);
+
+            ManagedIdentityCredential credential = InstrumentClient(new ManagedIdentityCredential("mock-client-id", pipeline));
+
+            AccessToken actualToken = await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default));
+
+            Assert.AreEqual(ExpectedToken, actualToken.Token);
+
+            MockRequest request = mockTransport.Requests[0];
+
+            string query = request.Uri.Query;
+
+            Assert.AreEqual(request.Uri.Host, "169.254.169.254");
+            Assert.AreEqual(request.Uri.Path, "/metadata/identity/oauth2/token");
+            Assert.IsTrue(query.Contains("api-version=2018-02-01"));
+            Assert.IsTrue(query.Contains($"resource={Uri.EscapeDataString(ScopeUtilities.ScopesToResource(MockScopes.Default))}"));
+            Assert.IsTrue(request.Headers.TryGetValue("Metadata", out string metadataValue));
+            Assert.IsTrue(query.Contains($"{Constants.ManagedIdentityClientId}=mock-client-id"));
+            Assert.AreEqual("true", metadataValue);
+        }
+
+        [NonParallelizable]
+        [Test]
+        [TestCaseSource(nameof(AuthorityHostValues))]
+        public async Task VerifyImdsRequestWithClientIdAndNonPubCloudMockAsync(Uri authority)
+        {
+            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 response = CreateMockResponse(200, ExpectedToken);
+            var mockTransport = new MockTransport(response);
+            var options = new TokenCredentialOptions() { Transport = mockTransport, AuthorityHost = authority };
+            //var pipeline = CredentialPipeline.GetInstance(options);
+            var _pipeline = new HttpPipeline(mockTransport);
+            var pipeline =  new CredentialPipeline(authority, _pipeline, new ClientDiagnostics(options));
+
+            ManagedIdentityCredential credential = InstrumentClient(new ManagedIdentityCredential(new ManagedIdentityClient( pipeline, "mock-client-id")));
+
+            AccessToken actualToken = await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default));
+
+            Assert.AreEqual(ExpectedToken, actualToken.Token);
+
+            MockRequest request = mockTransport.Requests[0];
+
+            string query = request.Uri.Query;
+
+            Assert.AreEqual(request.Uri.Host, "169.254.169.254");
+            Assert.AreEqual(request.Uri.Path, "/metadata/identity/oauth2/token");
+            Assert.IsTrue(query.Contains("api-version=2018-02-01"));
+            Assert.IsTrue(query.Contains($"resource={Uri.EscapeDataString(ScopeUtilities.ScopesToResource(MockScopes.Default))}"));
+            Assert.IsTrue(request.Headers.TryGetValue("Metadata", out string metadataValue));
+            Assert.IsTrue(query.Contains($"{Constants.ManagedIdentityClientId}=mock-client-id"));
+            Assert.AreEqual("true", metadataValue);
+        }
+
         [NonParallelizable]
         [Test]
         public async Task VerifyImdsRequestWithResourceIdMockAsync()
@@ -781,6 +849,17 @@ private static IEnumerable<TestCaseData> ExceptionalEnvironmentConfigs()
             yield return new TestCaseData(new Dictionary<string, string>() { { "MSI_ENDPOINT", null }, { "MSI_SECRET", null }, { "IDENTITY_ENDPOINT", null }, { "IDENTITY_HEADER", null }, { "IDENTITY_SERVER_THUMBPRINT", "null" }, { "AZURE_POD_IDENTITY_AUTHORITY_HOST", "http::@/bogusuri" } });
         }
 
+        public static IEnumerable<object[]> AuthorityHostValues()
+        {
+            // params
+            // az thrown Exception message, expected message, expected  exception
+            yield return new object[] { AzureAuthorityHosts.AzureChina };
+            yield return new object[] { AzureAuthorityHosts.AzureGermany };
+            yield return new object[] { AzureAuthorityHosts.AzureGovernment };
+            yield return new object[] { AzureAuthorityHosts.AzurePublicCloud };
+            yield return new object[] { new Uri("https://foo.bar") };
+        }
+
         private MockResponse CreateMockResponse(int responseCode, string token)
         {
             var response = new MockResponse(responseCode);

Original file line number	Diff line number	Diff line change
`@@ -2,7 +2,6 @@`
`2`	`2`	`// Licensed under the MIT License.`
`3`	`3`
`4`	`4`	`using System;`
`5`		`-using System.Runtime.CompilerServices;`
`6`	`5`	`using System.Security.Cryptography.X509Certificates;`
`7`	`6`	`using System.Threading;`
`8`	`7`	`using System.Threading.Tasks;`
`@@ -20,6 +19,7 @@ internal class MsalConfidentialClient : MsalClientBase<IConfidentialClientApplic`
`20`	`19`	`private readonly Func<string> _assertionCallback;`
`21`	`20`	`private readonly Func<CancellationToken, Task<string>> _asyncAssertionCallback;`
`22`	`21`	`private readonly Func<AppTokenProviderParameters, Task<AppTokenProviderResult>> _appTokenProviderCallback;`
	`22`	`+ private readonly Uri _authority;`
`23`	`23`
`24`	`24`	`internal string RedirectUrl { get; }`
`25`	`25`
`@@ -59,6 +59,7 @@ public MsalConfidentialClient(CredentialPipeline pipeline, string tenantId, stri`
`59`	`59`	`: base(pipeline, tenantId, clientId, options)`
`60`	`60`	`{`
`61`	`61`	`_appTokenProviderCallback = appTokenProviderCallback;`
	`62`	`+ _authority = options?.AuthorityHost ?? AzureAuthorityHosts.AzurePublicCloud;`
`62`	`63`	`}`
`63`	`64`
`64`	`65`	`internal string RegionalAuthority { get; } = EnvironmentVariables.AzureRegionalAuthorityName;`
`@@ -69,11 +70,12 @@ protected override async ValueTask<IConfidentialClientApplication> CreateClientA`
`69`	`70`	`.WithHttpClientFactory(new HttpPipelineClientFactory(Pipeline.HttpPipeline))`
`70`	`71`	`.WithLogging(LogMsal, enablePiiLogging: IsPiiLoggingEnabled);`
`71`	`72`
`72`		`- //special case for using appTokenProviderCallback, authority validation and instance metadata discovery should be disabled since we're not calling the STS`
	`73`	`+ // Special case for using appTokenProviderCallback, authority validation and instance metadata discovery should be disabled since we're not calling the STS`
	`74`	`+ // The authority matches the one configured in the CredentialOptions.`
`73`	`75`	`if (_appTokenProviderCallback != null)`
`74`	`76`	`{`
`75`	`77`	`confClientBuilder.WithAppTokenProvider(_appTokenProviderCallback)`
`76`		`- .WithAuthority(Pipeline.AuthorityHost.AbsoluteUri, TenantId, false)`
	`78`	`+ .WithAuthority(_authority.AbsoluteUri, TenantId, false)`
`77`	`79`	`.WithInstanceDiscoveryMetadata(s_instanceMetadata);`
`78`	`80`	`}`
`79`	`81`	`else`
`@@ -102,7 +104,7 @@ protected override async ValueTask<IConfidentialClientApplication> CreateClientA`
`102`	`104`	`confClientBuilder.WithCertificate(clientCertificate);`
`103`	`105`	`}`
`104`	`106`
`105`		`- if (!string.IsNullOrEmpty(RegionalAuthority))`
	`107`	`+ if (_appTokenProviderCallback == null && !string.IsNullOrEmpty(RegionalAuthority))`
`106`	`108`	`{`
`107`	`109`	`confClientBuilder.WithAzureRegion(RegionalAuthority);`
`108`	`110`	`}`