Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure Options are passed down to ManagedIdentityClient #34078

Closed
wants to merge 3 commits into from
Closed

Ensure Options are passed down to ManagedIdentityClient #34078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1389a84
Ensure Options are passed down to ManagedIdentityClient
pharring Feb 10, 2023
a85cdcd
Improve on the fix
pharring Feb 10, 2023
3c38288
Add tenantId to test case to demonstrate the bug
pharring Feb 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions sdk/identity/Azure.Identity/src/Credentials/ManagedIdentityCredential.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,8 @@ protected ManagedIdentityCredential()
/// </param>
/// <param name="options">Options to configure the management of the requests sent to the Azure Active Directory service.</param>
public ManagedIdentityCredential(string clientId = null, TokenCredentialOptions options = null)
: this(clientId, CredentialPipeline.GetInstance(options))
: this(clientId, CredentialPipeline.GetInstance(options), options)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the related changes in constructors below) is really the key fix. Ensuring that options gets passed down to the inner ManagedIdentityClient and eventually into the MsalConfidentialClient.
This is the big difference between using the ManagedIdentityCredential stand-alone versus via DefaultAzureCredential. DefaultAzureCredential's factory calls one of the internal constructors, correctly passing in Options. So what I'm doing here is making sure the two approaches have the same behavior.

This fix alone would be enough to fix the bug.

{
_logAccountDetails = options?.Diagnostics?.IsAccountIdentifierLoggingEnabled ?? false;
}

/// <summary>
Expand All @@ -59,23 +58,22 @@ public ManagedIdentityCredential(string clientId = null, TokenCredentialOptions
/// </param>
/// <param name="options">Options to configure the management of the requests sent to the Azure Active Directory service.</param>
public ManagedIdentityCredential(ResourceIdentifier resourceId, TokenCredentialOptions options = null)
: this(
new ManagedIdentityClient(new ManagedIdentityClientOptions { ResourceIdentifier = resourceId, Pipeline = CredentialPipeline.GetInstance(options) }))
: this(resourceId, CredentialPipeline.GetInstance(options), options)
{
_logAccountDetails = options?.Diagnostics?.IsAccountIdentifierLoggingEnabled ?? false;
_clientId = resourceId.ToString();
}

internal ManagedIdentityCredential(string clientId, CredentialPipeline pipeline, bool preserveTransport = false)
: this(new ManagedIdentityClient(new ManagedIdentityClientOptions { Pipeline = pipeline, ClientId = clientId, PreserveTransport = preserveTransport }))
internal ManagedIdentityCredential(string clientId, CredentialPipeline pipeline, TokenCredentialOptions options = null, bool preserveTransport = false)
: this(new ManagedIdentityClient(new ManagedIdentityClientOptions { Pipeline = pipeline, ClientId = clientId, Options = options, PreserveTransport = preserveTransport }))
{
_clientId = clientId;
_logAccountDetails = options?.Diagnostics?.IsAccountIdentifierLoggingEnabled ?? false;
}

internal ManagedIdentityCredential(ResourceIdentifier resourceId, CredentialPipeline pipeline, bool preserveTransport = false)
: this(new ManagedIdentityClient(new ManagedIdentityClientOptions{Pipeline = pipeline, ResourceIdentifier = resourceId, PreserveTransport = preserveTransport}))
internal ManagedIdentityCredential(ResourceIdentifier resourceId, CredentialPipeline pipeline, TokenCredentialOptions options = null, bool preserveTransport = false)
: this(new ManagedIdentityClient(new ManagedIdentityClientOptions { Pipeline = pipeline, ResourceIdentifier = resourceId, Options = options, PreserveTransport = preserveTransport }))
{
_clientId = resourceId.ToString();
_logAccountDetails = options?.Diagnostics?.IsAccountIdentifierLoggingEnabled ?? false;
}

internal ManagedIdentityCredential(ManagedIdentityClient client)
Expand Down
4 changes: 2 additions & 2 deletions sdk/identity/Azure.Identity/src/MsalConfidentialClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public MsalConfidentialClient(CredentialPipeline pipeline, string tenantId, stri
: base(pipeline, tenantId, clientId, options)
{
_appTokenProviderCallback = appTokenProviderCallback;
_authority = options?.AuthorityHost ?? AzureAuthorityHosts.AzurePublicCloud;
_authority = options?.AuthorityHost ?? pipeline.AuthorityHost ?? AzureAuthorityHosts.AzurePublicCloud;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't strictly necessary, but it seems like a reasonable fallback. What do you think?

}

internal string RegionalAuthority { get; } = EnvironmentVariables.AzureRegionalAuthorityName;
Expand Down Expand Up @@ -146,7 +146,7 @@ public virtual async ValueTask<AuthenticationResult> AcquireTokenForClientCoreAs

if (!string.IsNullOrEmpty(tenantId))
{
builder.WithAuthority(Pipeline.AuthorityHost.AbsoluteUri, tenantId);
builder.WithTenantId(tenantId);
Copy link
Contributor Author

@pharring pharring Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code path is hit by the ChallengeBasedAuthenticationPolicy in KeyVault. The tenantId comes from the challenge response. If Pipeline.AuthorityHost doesn't agree with the authority passed to the ConfidentialClientBuilder, then we hit the MSAL exception about mismatched authorities (https://aka.ms/msal-net-authority-override).

Switching to .WithTenantId sidesteps the issue of knowing what authority was passed to the ConfidentialClientBuilder.

I think this alone would be sufficient to fix the bug, but it's really subtle.

}
return await builder
.ExecuteAsync(async, cancellationToken)
Expand Down
4 changes: 1 addition & 3 deletions sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialImdsLiveTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,7 @@ private ManagedIdentityCredential CreateManagedIdentityCredential(string clientI
{
options = InstrumentClientOptions(options ?? new TokenCredentialOptions());

var pipeline = CredentialPipeline.GetInstance(options);

var cred = new ManagedIdentityCredential(new ManagedIdentityClient(pipeline, clientId));
var cred = new ManagedIdentityCredential(clientId, options);
Copy link
Contributor Author

@pharring pharring Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the public constructor here instead of the internal one, now that the public one passes through options correctly.


return cred;
}
Expand Down
16 changes: 7 additions & 9 deletions sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,9 @@ public async Task VerifyImdsRequestWithClientIdAndNonPubCloudMockAsync(Uri autho
var pipeline = new CredentialPipeline(authority, _pipeline, new ClientDiagnostics(options));

ManagedIdentityCredential credential = InstrumentClient(
new ManagedIdentityCredential(
new ManagedIdentityClient(
new ManagedIdentityClientOptions { Pipeline = pipeline, ClientId = "mock-client-id", Options = options })));
new ManagedIdentityCredential("mock-client-id", pipeline, options));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, making use of the public constructor instead of the internal one.


AccessToken actualToken = await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default));
AccessToken actualToken = await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default, tenantId: "mock-tenant-id"));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the tenantId here exercises the code-path that hits the original bug.


Assert.AreEqual(ExpectedToken, actualToken.Token);

Expand Down Expand Up @@ -216,9 +214,9 @@ public async Task VerifyServiceFabricRequestWithResourceIdMockAsync(string clien

ManagedIdentityCredential credential = (clientId, includeResourceIdentifier) switch
{
(Item1: null, Item2: true) => InstrumentClient(new ManagedIdentityCredential(new ResourceIdentifier(_expectedResourceId), pipeline, true)),
(Item1: not null, Item2: false) => InstrumentClient(new ManagedIdentityCredential(clientId, pipeline, true)),
_ => InstrumentClient(new ManagedIdentityCredential(clientId: null, pipeline, true))
(Item1: null, Item2: true) => InstrumentClient(new ManagedIdentityCredential(new ResourceIdentifier(_expectedResourceId), pipeline, preserveTransport: true)),
(Item1: not null, Item2: false) => InstrumentClient(new ManagedIdentityCredential(clientId, pipeline, preserveTransport: true)),
_ => InstrumentClient(new ManagedIdentityCredential(clientId: null, pipeline, preserveTransport: true))
};

AccessToken actualToken = await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default));
Expand Down Expand Up @@ -605,7 +603,7 @@ public async Task VerifyCloudShellMsiRequestWithClientIdMockAsync(string clientI

[NonParallelizable]
[Test]
public async Task VerifyMsiUnavailableOnIMDSAggregateExcpetion()
public async Task VerifyMsiUnavailableOnIMDSAggregateException()
{
using var environment = new TestEnvVar(new() { { "MSI_ENDPOINT", null }, { "MSI_SECRET", null }, { "IDENTITY_ENDPOINT", null }, { "IDENTITY_HEADER", null }, { "AZURE_POD_IDENTITY_AUTHORITY_HOST", "http://169.254.169.001/" } });

Expand All @@ -623,7 +621,7 @@ public async Task VerifyMsiUnavailableOnIMDSAggregateExcpetion()

[NonParallelizable]
[Test]
public async Task VerifyMsiUnavailableOnIMDSRequestFailedExcpetion()
public async Task VerifyMsiUnavailableOnIMDSRequestFailedException()
{
using var environment = new TestEnvVar(new() { { "MSI_ENDPOINT", null }, { "MSI_SECRET", null }, { "IDENTITY_ENDPOINT", null }, { "IDENTITY_HEADER", null }, { "AZURE_POD_IDENTITY_AUTHORITY_HOST", "http://169.254.169.001/" } });

Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/Azure.Identity/tests/Mock/TestDefaultAzureCredentialFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public override TokenCredential CreateEnvironmentCredential()
=> new EnvironmentCredential(Pipeline, Options);

public override TokenCredential CreateManagedIdentityCredential()
=> new ManagedIdentityCredential(new ManagedIdentityClient(Pipeline, Options.ManagedIdentityClientId));
=> new ManagedIdentityCredential(Options.ManagedIdentityClientId, Pipeline, Options);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, using the public constructor instead of the internal one.


public override TokenCredential CreateSharedTokenCacheCredential()
=> new SharedTokenCacheCredential(Options.SharedTokenCacheTenantId, Options.SharedTokenCacheUsername, Options, Pipeline);
Expand Down