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

Make AAD resource scope match cloud #20872

Closed
1 task
annelo-msft opened this issue May 5, 2021 · 6 comments · Fixed by #21493
Closed
1 task

Make AAD resource scope match cloud #20872

annelo-msft opened this issue May 5, 2021 · 6 comments · Fixed by #21493
Labels
Client This issue points to a problem in the data-plane of the library. Container Registry
Milestone

Comments

@annelo-msft
Copy link
Member

annelo-msft commented May 5, 2021

We are currently hard-coding resource scope in the client:

private readonly string AcrAadScope = "https://management.core.windows.net/.default";

We'll need to use a different resource id per cloud:
https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/services-support-managed-identities#azure-resource-manager

The registry domain will be different for every cloud, we have myregistry.azurecr.io for public, myregistry.azurecr.cn for China etc. We will obtain the cloud identity from the registryURI and set the resource id based on that value.

See comments for discussion of what libraries in the SDK do -- it varies based on service implementation.

Updated implementation plan

We will add an AuthenticationScope field to ContainerRegistryClientOptions. For now, if this value is set, it will override the authentication scope we're using to obtain the AAD token, which otherwise will default to the scope we're currently using -- which gives access to the public cloud.

From a customer perspective, to use the SDK library with a non-public cloud, the AuthenticationScope value will need to be set to the correct value.

  • Make sure to include a sample showing this in the README.
@annelo-msft annelo-msft added Client This issue points to a problem in the data-plane of the library. Container Registry labels May 5, 2021
@annelo-msft annelo-msft added this to the [2021] June milestone May 5, 2021
@annelo-msft
Copy link
Member Author

FYI @mnltejaswini @AlexGhiondea

@annelo-msft
Copy link
Member Author

KeyVault gets the resource for authenticating a request from a challenge response sent back from the KV service. ACR doesn't have the same paradigm, so we won't follow this precedent.

@annelo-msft
Copy link
Member Author

annelo-msft commented May 10, 2021

Discussion in prior issue: #18520 (comment)

Per this comment, storage hardcodes the resource scope.

Per offline discussion with @schaabs and @christothes, we'd like to avoid exposing this in ClientOptions if possible, to isolate the concept of token scope to the Identity library in the SDK.

Given this discussion and the confirmation from service team that registry URI contains the necessary information to distinguish between clouds, we will move forward with the approach where we obtain the cloud identity from the registry URI and map this to the appropriate token scope.

@annelo-msft
Copy link
Member Author

List of cloud suffixes can be obtained from the acrLoginServerValue returned by az cloud list. These are:

  • "AzureCloud": ".azurecr.io"
  • "AzureChinaCloud": ".azurecr.cn"
  • "AzureUSGovernment": ".azurecr.us"
  • "AzureGermanCloud" : ".azurecr.de"

@annelo-msft
Copy link
Member Author

Cloud endpoint URIs are available in Azure.Identity here, if relevant: https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/identity/Azure.Identity/src/AzureAuthorityHosts.cs

@annelo-msft
Copy link
Member Author

@pallavit @jeremymeng @seankane-msft FYI re: updated implementation plan above.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Container Registry
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant