Detect system-assigned managed identities in AzureCredentialHelper#15885
Detect system-assigned managed identities in AzureCredentialHelper#15885DavidZidar wants to merge 1 commit intomicrosoft:mainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15885Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15885" |
There was a problem hiding this comment.
Pull request overview
Restores managed-identity support in AzureCredentialHelper for Azure App Service deployments by detecting system-assigned managed identity signals and selecting a managed-identity credential path instead of falling back to development credentials.
Changes:
- Detects
IDENTITY_ENDPOINT(in addition toAZURE_CLIENT_ID) to decide when to useManagedIdentityCredential. - Adds an inline comment clarifying why
IDENTITY_ENDPOINTis checked.
When system-assigned managed identities are enabled the IDENTITY_ENDPOINT and IDENTITY_HEADER environment variables are defined. It should be enough to detect the endpoint variable for this purpose. AZURE_CLIENT_ID is still used by user-managed identities. Reference: https://learn.microsoft.com/en-us/azure/app-service/overview-managed-identity?tabs=portal%2Chttp#rest-endpoint-reference
60f26ad to
05cce1c
Compare
|
Copilot pointed out that the code doesn't seem to be working for user-assigned managed identities either so I made the suggested adjustments (with some minor tweaks) to account for this too. |
|
|
||
| if (Environment.GetEnvironmentVariable("AZURE_CLIENT_ID") is not null) | ||
| var azureClientId = Environment.GetEnvironmentVariable("AZURE_CLIENT_ID"); | ||
| var identityEndpoint = Environment.GetEnvironmentVariable("IDENTITY_ENDPOINT"); |
There was a problem hiding this comment.
I have a few concerns with this approach.
- Aspire by default configures a user-assigned identity. In order to use a system-assigned identity, you need to customize the app. It isn't unreasonable to also need to configure the
settings.Credentialproperty inside the app as well. See the comment that said "just use ManagedIdentityCredential because that's the only credential type that Aspire Hosting enables by default." (which is now wrong because it also says system-assigned) - Looking at the Azure SDK code, I see an unused property (
EnvironmentVariables.IdentityEndpoint) reading theIDENTITY_ENDPOINTenvironment variable. But no one reads that property. So I'm not sure if this is the right env var to read.
There was a problem hiding this comment.
- That would be fine if this was code intended to be run only in Aspire. But currently the AzureCredentialHelper class is incompatible with Azure without extra modification and the documentation do not say that
Credentialmust be replaced to be able to deploy to Azure. - That is the correct environment variable, see https://learn.microsoft.com/en-us/azure/app-service/overview-managed-identity?tabs=portal%2Chttp#rest-endpoint-reference
There was a problem hiding this comment.
IDENTITY_ENDPOINT is only expected to be present in App Service or Azure Functions - it's not a general indicator of the presence of a system assigned identity, if that was your intent here.
There was a problem hiding this comment.
That's good to know, is there a general way to detect this? My intent is to restore functionality that's currently broken because DefaultAzureCredential is no longer in use like before. I understand it's not recommended in production but the current solution is worse.
There was a problem hiding this comment.
Perhaps the logic can be reversed so that it detects Aspire instead and customizes credentials for that use case? When not detected use DefaultAzureCredential like before.
There was a problem hiding this comment.
I'm using the Aspire.Azure.Security.KeyVault package to add KeyVault secrets to my configuration using AddAzureKeyVaultSecrets. This has been working well for a while now but suddenly broke when I tried to deploy my solution to Azure after I updated packages recently and I had to spend 2 hours figuring out what had happened.
Before the change in #14951 the package was using DefaultAzureCredential which correctly figured out to use system-assigned managed identity, but it no longer works out of the box in Azure and Credential has to be set manually if one wants to use the Aspire packages.
There was a problem hiding this comment.
Plus, the documentation makes no mention that Credential must be set manually to be able to use system-managed identities which didn't help.
There was a problem hiding this comment.
Might I suggest a different approach.
If Aspire must use custom heuristics to figure out what credentials to use then at least fall back to the DefaultAzureCredential with all options enabled if no certain option could be determined and perhaps log a warning in non development environments that it's recommended to set Credential explicitly. If DefaultAzureCredential needs to be avoided in production completely then at least throw a more informative exception referring to best practices and what to do.
The current failure mode is not good, it throws a pretty obtuse message that lists a lot of providers and it's up to the user to figure out what might be wrong. This is why it took me so long to get unblocked.
There was a problem hiding this comment.
We've been trying to encourage, via our APIs in Azure.Identity, that developers be explicit with which managed identity they expect, since the defaults can cause unexpected behavior. For example, if no preference is expressed in how the credential is configured, the following will occur:
- No system assigned identity is configured
- no user-assigned MI is configured
- error
- exactly 1 UAMI is configured
- the UAMI is used
- More than 1 UAMI is configured
- error
- no user-assigned MI is configured
- SAMI is configured
- no user-assigned MI is configured
- SAMI is used
- exactly 1 UAMI is configured
- the UAMI is used
- More than 1 UAMI is configured
- error
- no user-assigned MI is configured
There was a problem hiding this comment.
I'm not opposed to encouraging, it's good to steer people into the pit of success. That's why I suggested logging a warning. But the current solution breaks people apps, that's more than some encouraging. I already spent the time and figured out why my app suddenly broke but there will be more people that will be hit by this change when they try to deploy their things to Azure, hopefully not directly to production.
| var managedIdentityId = !string.IsNullOrWhiteSpace(azureClientId) | ||
| ? ManagedIdentityId.FromUserAssignedClientId(azureClientId) | ||
| : ManagedIdentityId.SystemAssigned; |
There was a problem hiding this comment.
If we are going to take this approach, I think I'd prefer less branches. Something like:
if (Environment.GetEnvironmentVariable("AZURE_CLIENT_ID") is not null)
{
// When we don't see DefaultEnvironmentVariableName, but we do see AZURE_CLIENT_ID,
// we just use ManagedIdentityCredential because that's the only credential type that
// Aspire Hosting enables by default.
// If this doesn't work for applications, they can override the TokenCredential in their settings.
return new ManagedIdentityCredential(new ManagedIdentityCredentialOptions());
}
else if (Environment.GetEnvironmentVariable("IDENTITY_ENDPOINT") is not null)
{
// <insert reasoning>
return new ManagedIdentityCredential(new ManagedIdentityCredentialOptions(ManagedIdentityId.SystemAssigned));
}
// when we can't detect a known Azure environment, fall back to the development credential
return CreateDevelopmentAzureCredential();
Description
This should restore support for system-assigned managed identities.
When system-assigned managed identities are enabled the
IDENTITY_ENDPOINTandIDENTITY_HEADERenvironment variables are defined. It should be enough to detect the endpoint variable for this purpose.AZURE_CLIENT_IDis still used by user-managed identities.Reference:
https://learn.microsoft.com/en-us/azure/app-service/overview-managed-identity?tabs=portal%2Chttp#rest-endpoint-reference
Fixes #15879
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: