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.

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

[Az.Accounts]: ambiguity in clientfactory.CreateArmClient #18634

Open
VeryEarly opened this issue Jun 20, 2022 · 0 comments
Open

[Az.Accounts]: ambiguity in clientfactory.CreateArmClient #18634

VeryEarly opened this issue Jun 20, 2022 · 0 comments
Labels
Accounts Issues in Az.Accounts except authentication related Authentication feature-request This issue requires a new behavior in the product in order be resolved.
Milestone

Comments

@VeryEarly
Copy link
Contributor

VeryEarly commented Jun 20, 2022

Description

To create track1 SDK client, we use method

public virtual TClient CreateArmClient<TClient>(IAzureContext context, string endpoint) where TClient : Microsoft.Rest.ServiceClient<TClient>

the endpoint passed in will be used to

  • as endpoint: construct base uri ( var baseUri = context.Environment.GetEndpointAsUri(endpoint);)
  • as resourceId: retrieve access token (var creds = AzureSession.Instance.AuthenticationFactory.GetServiceClientCredentials(context, endpoint);)

there are two part of authenticate flow:

  1. when login as access token:
    RenewingTokenCredential(new ExternalAccessToken(GetEndpointToken(context.Account, targetEndpoint), () => GetEndpointToken(context.Account, targetEndpoint)));
private string GetEndpointToken(IAzureAccount account, string targetEndpoint)
        {
            string tokenKey = AzureAccount.Property.AccessToken;
            if (string.Equals(targetEndpoint, AzureEnvironment.Endpoint.AzureKeyVaultServiceEndpointResourceId, StringComparison.OrdinalIgnoreCase))
            {
                tokenKey = AzureAccount.Property.KeyVaultAccessToken;
            }
            if (string.Equals(targetEndpoint, AzureEnvironment.ExtendedEndpoint.MicrosoftGraphEndpointResourceId, StringComparison.OrdinalIgnoreCase))
            {
                tokenKey = Constants.MicrosoftGraphAccessToken;
            }
            if (string.Equals(targetEndpoint, AzureEnvironment.Endpoint.Graph, StringComparison.OrdinalIgnoreCase))
            {
                tokenKey = AzureAccount.Property.GraphAccessToken;
            }
            return account.GetProperty(tokenKey);
        }
  1. otherwise
public IAccessToken Authenticate(
            IAzureAccount account,
            IAzureEnvironment environment,
            string tenant,
            SecureString password,
            string promptBehavior,
            Action<string> promptAction,
            IAzureTokenCache tokenCache,
            string resourceId = AzureEnvironment.Endpoint.ActiveDirectoryServiceEndpointResourceId)

for flow 1, the targetEndpoint is actually irrelevant to the actual access token (in context.account.extendedproperties, key: Microsoft.Azure.Commands.Common.Authentication.Constants.MicrosoftGraphAccessToken, value: msgraph access token)

When create MSGraph client , as the "CreateArmClient" method suggested, we passed in the endpoint "MicrosoftGraphUrl", however this cannot be matched in flow 1 :

if (string.Equals(targetEndpoint, AzureEnvironment.ExtendedEndpoint.MicrosoftGraphEndpointResourceId, StringComparison.OrdinalIgnoreCase))

which returned the ARM accesstoken instead.

I made a work around #18414 to let it match both MSGraph ResourceId and MSGraph endpoint. A better fix would be use ResourceId for both flow 1 and 2, separate endpoint and resourceId in:

var creds = AzureSession.Instance.AuthenticationFactory.GetServiceClientCredentials(context, context.Environment.GetTokenAudience(endpoint));
var baseUri = context.Environment.GetEndpointAsUri(endpoint);

and in flow 1:

RenewingTokenCredential(new ExternalAccessToken(GetEndpointToken(context.Account, ResourceId), () => GetEndpointToken(context.Account, ResourceId)));
@dingmeng-xue dingmeng-xue added Authentication Accounts Issues in Az.Accounts except authentication related and removed Engineering labels Jun 23, 2022
@dingmeng-xue dingmeng-xue added this to the Backlog milestone Jun 23, 2022
@dingmeng-xue dingmeng-xue added the feature-request This issue requires a new behavior in the product in order be resolved. label Jun 23, 2022
@VeryEarly VeryEarly changed the title [Eng]: ambiguity in clientfactory.CreateArmClient [Az.Accounts]: ambiguity in clientfactory.CreateArmClient Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accounts Issues in Az.Accounts except authentication related Authentication feature-request This issue requires a new behavior in the product in order be resolved.
Projects
None yet
Development

No branches or pull requests

2 participants