Ensure Options are passed down to ManagedIdentityClient #34078
Conversation
Specifically, this is to pass the AuthorityHost to the MSAL client
ghost
added
the
|
Probably needs unit tests to show the problem. Note that DefaultAzureCredentialFactory.CreateManagedIdentityCredential already gets it right. |
|
I believe this is the fix for #34077 |
|
API change check API changes are not detected in this pull request. |
|
@schaabs ping me when you get a chance because I want to discuss this with you. There are a 3 different fixes in here. I think any one of them would fix the bug. Do we want to include all three? |
| @@ -146,7 +146,7 @@ protected override async ValueTask<IConfidentialClientApplication> CreateClientA | |||
|
|
|||
| if (!string.IsNullOrEmpty(tenantId)) | |||
| { | |||
| builder.WithAuthority(Pipeline.AuthorityHost.AbsoluteUri, tenantId); | |||
| builder.WithTenantId(tenantId); | |||
There was a problem hiding this comment.
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.
| var pipeline = CredentialPipeline.GetInstance(options); | ||
|
|
||
| var cred = new ManagedIdentityCredential(new ManagedIdentityClient(pipeline, clientId)); | ||
| var cred = new ManagedIdentityCredential(clientId, options); |
There was a problem hiding this comment.
Using the public constructor here instead of the internal one, now that the public one passes through options correctly.
| @@ -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); | |||
There was a problem hiding this comment.
Again, using the public constructor instead of the internal one.
| @@ -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; | |||
There was a problem hiding this comment.
This isn't strictly necessary, but it seems like a reasonable fallback. What do you think?
| @@ -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) | |||
There was a problem hiding this comment.
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.
| new ManagedIdentityCredential( | ||
| new ManagedIdentityClient( | ||
| new ManagedIdentityClientOptions { Pipeline = pipeline, ClientId = "mock-client-id", Options = options }))); | ||
| new ManagedIdentityCredential("mock-client-id", pipeline, options)); |
There was a problem hiding this comment.
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")); |
There was a problem hiding this comment.
Adding the tenantId here exercises the code-path that hits the original bug.
|
@christothes Great. Put me on the final PR when you have it. |
Specifically, this is to pass the AuthorityHost to the MSAL client