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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Issue] Container Registry auth error while deploying apps through azd up #2980

Closed
1 task done
justinyoo opened this issue Nov 15, 2023 · 14 comments
Closed
1 task done
Assignees
Labels

Comments

@justinyoo
Copy link

Output from azd version

azd version 1.4.5 (commit 6cb723e44eb0697d4d8d09f7694169f5410c7d6f)

Describe the bug

While provisioning and deploying Aspire-enabled app, I encountered the permission issue like this screenshot:
image

The error says that I need to enable the "Admin User" permission. Once I enable the "Admin User" permission through Azure Portal, the error goes away.

To Reproduce

  1. Create an Aspire starter project or use my sample app repository: https://github.com/devkimchi/aspire-youtube-summariser

    If you use my repo, checkout to the polly tag, git checkout tags/polly.

  2. Run azd init
  3. Run azd up
  4. See the error screen like above.

Expected behavior

Both Web app and API app, as well as Redis Cache are properly deployed to Azure Container Apps, without having to manually enable the "Admin User" permission on ACR.

Environment

Information on your environment:
* Language name and version: C# (.NET 8)
* IDE and version : Visual Studio 2022 Preview, v17.9

Additional context

If I run azd infra synth, it generates the bicep template. Here's the Container Registry part:

resource containerRegistry 'Microsoft.ContainerRegistry/registries@2023-07-01' = {
  name: replace('acr-${resourceToken}', '-', '')
  location: location
  sku: {
    name: 'Basic'
  }
  tags: tags
}

However, it doesn't activate the admin user permission. I suspect that it should be:

resource containerRegistry 'Microsoft.ContainerRegistry/registries@2023-07-01' = {
  name: replace('acr-${resourceToken}', '-', '')
  location: location
  sku: {
    name: 'Basic'
  }
  // 馃憞馃憞馃憞 This property should be added
  properties: {
    adminUserEnabled: true
  }
  // 馃憜馃憜馃憜 This property should be added
  tags: tags
}
@weikanglim
Copy link
Contributor

Hi @justinyoo, based on the error message: azd first tried to use your logged-in user's AD token to login to the registry. That failed due to 403 Unauthorized. The fallback used (legacy) admin user auth which also failed.

  1. Does running az acr login --name <acrName> also fail for you? (Assuming you had previously ran az login
  2. Do you have at least Contributor role to the resource group? For custom roles and scopes required, see the docs here.

@weikanglim weikanglim self-assigned this Nov 15, 2023
@justinyoo
Copy link
Author

@weikanglim Thanks for the workaround! I'll take another try. I was suspecting that as well, but didn't try because azd would take care of them as well.

The second option doesn't seem to be applicable for my case because I'm the owner of the subscription. Therefore, any resource group belongs to my account.

@vhvb1989
Copy link
Member

Reading at https://azure.github.io/acr/AAD-OAuth.html#listing-a-repository-with-azure-cli, it looks like AAD refresh token is deprecated. Is that what azd is using @wbreza ? Do we need to migrate to something else? Or, can we just use the AAD refresh token directly now?

@wbreza
Copy link
Contributor

wbreza commented Nov 16, 2023

@vhvb1989 It looks like they may have deprecated parts of the flow we were using. I haven't seen this issue in any of our tests but we should migrate to the new supported path.

Current Implementation
Exchanges AAD access token for an ACR refresh token
https://github.com/Azure/azure-dev/blob/main/cli/azd/pkg/tools/azcli/container_registry.go#L213

@wbreza
Copy link
Contributor

wbreza commented Nov 17, 2023

@justinyoo can you tell us more about the infrastructure setup? The user/service principal that is logged into azd performing the azd deploy will need to have AcrPush role assigned to the container registry (ACR) to push images.

Also, the logged in user would need to have Contributor role for the resource group / resources to make any required container app revision modification after the containers are pushed.

You can take a look at our reference sample in the Azure-Samples/todo-nodejs-mongo-aca Sample for a complete sample.

We do recommend the RBAC assignments over the admin account method as it is more secure and doesn't require any additional keys.

Even though the login flow azd is using is deprecated it should still work for the time being with the correct configuration.

@justinyoo
Copy link
Author

@wbreza Thanks for the clarification.

I'm the owner of the subscription and I've got the "Service Administrator" role of the subscription. I thought it would be enough for ACR.

@justinyoo
Copy link
Author

@wbreza Here's my update:

  1. I installed the nightly version of azd.

    azd version 1.6.0-beta.1-daily.3273852 (commit f00f37d70ae38a8b53d28650b0398ea4d097ec7d)
  2. I gave myself the Owner role, in addition to the Service Administrator role.

And the error has gone away. I suspect that the classic Service Administrator role has not been respected. Would you please be double check on your end?

@vhvb1989
Copy link
Member

@justinyoo , have you tried with Contributor role instead of Service Admin ?
IIRC, Service Admin is good for assigning roles to Service Principals, for example, if you want to create a Service Principal with Contributor role, you need to be Service Admin,

@justinyoo
Copy link
Author

@vhvb1989 Yep, I've assigned myself as Owner of the subscription. Then, the error goes away.

@rajeshkamal5050
Copy link

azd depends on the logged in users RBAC. We do not assign/elevate privileges for the user. The user should make sure to have the right permissions and retry.

@weikanglim looks like there are some deprecated API usage. Can we get those fixed?

@weikanglim
Copy link
Contributor

@rajeshkamal5050 I've looked at this once before earlier, and looked at it again, and I don't see anything problematic with the token exchange. We should be good here.

From the ACR token exchange docs to receive an ACR refresh token:

The accepted form of credential exchange are:

  • AAD access token.
  • [Deprecated] AAD refresh token.
  • [Deprecated] AAD access token and refresh token.

We're using AAD access tokens:

formData := url.Values{}
formData.Set("grant_type", "access_token")
formData.Set("service", loginServer)
formData.Set("access_token", token.Token)
tokenUrl := fmt.Sprintf("https://%s/oauth2/exchange", loginServer)

I think we can make a follow-up error message improvement to make it clearer what the expectations are.

@shanselman
Copy link

I'm seeing this also and I had to

az acr update -n MYACRTHING --admin-enabled true

This is super confusing. Why does azd not know how to fix this? I have all the permissions I need

ellismg added a commit to ellismg/azure-dev that referenced this issue Dec 5, 2023
Our intention is to use Azure RBAC to secure access to the container
registry, and the exchange our AAD token for a time limited access
token that we use to log into the registry. That has worked for some
users, but others have run into issues like what we see in Azure#2980

While we're still trying to root cause the actual issue, we have
discovered that if the admin account is enabled, the end to end
seems to work.

This change enables the admin account to allow `azd` to fall back to
that when the token exchange doesn't work.

Contributes To: Azure#2980
@ellismg
Copy link
Member

ellismg commented Dec 5, 2023

We've continued to look into this. It seems that a subset of accounts have this issue where we can't do the AAD token exchange as outlined on learn.microsoft.com. This fails with a 401 Unauthorized. The issue is not specific to azd, we've confirmed that az acr login -n <name-of-container-registry> --expose-token (which should do the exchange and then print out the short lived token that could be used with docker login also fails.

It prints a subtly different error:

matell@Matts-Work-MacBook AspireStarter % az acr login -n [redacted] --expose-token
Unable to get AAD authorization tokens with message: 2023-12-05 21:12:13.982997 An error occurred: CONNECTIVITY_REFRESH_TOKEN_ERROR
Access to registry 'acrvrzplyicfkirs.azurecr.io' was denied. Response code: 401. Please try running 'az login' again to refresh permissions.

But @weikanglim believes this is a red herring and is the az incorrectly interpreting the result of the 401 and giving back a slightly misleading error.

For now, #3069 enables the admin account which allows azd to fall back to these credentials and ensures the push can happen. I've validated that this addresses the issue, but we would like to understand what we need to be doing to have this AAD token exchange actually work the way we want. It is our preference to disable this admin account if possible.

For folks hitting this, you can either pick up a build from the linked PR, wait until our next daily build comes out (12/6) and use that or use azd infra synth and then add:

properties: {
  adminUserEnabled: true
}

To the containerRegistry resource in the generated infra/resources.bicep file. It should look like this, after your edits:

resource containerRegistry 'Microsoft.ContainerRegistry/registries@2023-07-01' = {
  name: replace('acr-${resourceToken}', '-', '')
  location: location
  sku: {
    name: 'Basic'
  }
  properties: {
    adminUserEnabled: true
  }
  tags: tags
}

This matches what azd will generate in the 1.5.1 release we'll do next week which contains this fix and others.

ellismg added a commit that referenced this issue Dec 5, 2023
Our intention is to use Azure RBAC to secure access to the container
registry, and the exchange our AAD token for a time limited access
token that we use to log into the registry. That has worked for some
users, but others have run into issues like what we see in #2980

While we're still trying to root cause the actual issue, we have
discovered that if the admin account is enabled, the end to end
seems to work.

This change enables the admin account to allow `azd` to fall back to
that when the token exchange doesn't work.

Contributes To: #2980
@weikanglim
Copy link
Contributor

We're finding with Azure/acr#723 that RBAC is failing for users that are currently Classic subscription administrators that do not have the RBAC role for ACR pre-configured.

The current known workaround is to assign yourself as a Acr*/Contributor/Owner either at the subscription or at the registry level.

Example of making the current Classic service administrator an Owner of the subscription:

  1. Go to Portal
  2. Find the current Subscription
  3. Select Access control (IAM) tab
  4. Click Add in the action pane
  5. Click the menu item dropdown Add role assignment
  6. Select the Privileged administrators
  7. Select Owner/Contributor
  8. Select the correct user and walkthrough the steps to complete the assignment

Example az cli command (please verify before running):

$user=(az ad signed-in-user show --query "id" --output tsv)
$subscription=(az account show --query "id" --output tsv)
echo "Assigning $user to subscription $subscription"
az role assignment create --assignee "$user" --role "Owner" --scope "/subscriptions/$subscription"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants