- 
                Notifications
    
You must be signed in to change notification settings  - Fork 715
 
Refactor ACR authentication to use Azure SDK TokenCredential with OAuth2 token exchange #12608
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
base: main
Are you sure you want to change the base?
Conversation
- Add LoginToRegistryAsync method to IContainerRuntime interface - Implement LoginToRegistryAsync in ContainerRuntimeBase with stdin support - Add StandardInputContent to ProcessSpec and update ProcessUtil to handle it - Update FakeContainerRuntime for testing - Refactor AzureEnvironmentResourceHelpers to use IContainerRuntime and ITokenCredentialProvider - Use TokenCredential to acquire ACR access token (scope: https://containerregistry.azure.com/.default) - Login with username 00000000-0000-0000-0000-000000000000 and token as password - Add InternalsVisibleTo for Azure projects to access IContainerRuntime - Remove duplicate shared file compile links from Azure projects - Log process output with PipelineStepContext.Logger at debug level - Throw exception on non-zero exit code to fail overall step Co-authored-by: davidfowl <[email protected]>
… compile links - Add Aspire.Hosting.Azure.Tests to InternalsVisibleTo - Remove duplicate shared file compile links from Azure.Tests project Co-authored-by: davidfowl <[email protected]>
- Simplified AuthenticateToAcrHelper to call containerRuntime.LoginToRegistryAsync directly - Removed duplicate LoginToRegistryWithLoggingAsync method - Removed unused Dcp.Process using statement - ContainerRuntimeBase handles all login logic and logging Co-authored-by: davidfowl <[email protected]>
- Add proper quote escaping in LoginToRegistryAsync to prevent command injection - Add Flush() call before closing StandardInput in ProcessUtil - Ensure sensitive data is properly handled during stdin writing Co-authored-by: davidfowl <[email protected]>
| 
           @copilot The azure logic should be using IContainerRuntime to login to the registry  | 
    
- Changed IContainerRuntime from internal to public interface with XML documentation - Removed InternalsVisibleTo for Aspire.Hosting.Azure.AppContainers and AppService - Kept InternalsVisibleTo for Aspire.Hosting.Azure and Azure.Tests (needed for ProcessSpec/ProcessResult) - Re-added ResourceNameComparer as shared source to AppContainers and AppService projects Co-authored-by: davidfowl <[email protected]>
          
 The Azure logic is already using IContainerRuntime.LoginToRegistryAsync (line 54 in AzureEnvironmentResourceHelpers.cs). I've now made IContainerRuntime public with full XML documentation (commit 2a80106) and removed the unnecessary InternalsVisibleTo entries for AppContainers and AppService projects. The interface is now publicly accessible for all Azure projects to use.  | 
    
| 
           @copilot Does Aspire.Hosting need these 2 new internals visible to entries in the project? If so, why?  | 
    
… helpers - Added [Experimental] attribute to IContainerRuntime with diagnostic ID ASPIRECONTAINERRUNTIME001 - Suppressed experimental warnings in all files using IContainerRuntime - Converted process helpers (ProcessSpec, ProcessResult, ProcessUtil) to shared source in Azure projects - Removed InternalsVisibleTo for Aspire.Hosting.Azure and Azure.Tests from Aspire.Hosting - Added shared source for CustomResourceSnapshotExtensions, StringComparers, KnownRelationshipTypes in Azure - Added shared source for KnownProperties and StringComparers in Azure.Tests - All projects now build successfully without InternalsVisibleTo for Azure Co-authored-by: davidfowl <[email protected]>
          
 The two InternalsVisibleTo entries were needed because Aspire.Hosting.Azure and Azure.Tests used ProcessSpec, ProcessResult, and ProcessUtil which were internal. I've now converted these to shared source files instead (commit 5deca0e), so the InternalsVisibleTo entries have been removed. All process helpers and other shared utilities are now compiled directly into the Azure projects rather than accessed through InternalsVisibleTo.  | 
    
| 
          
 🚀 Dogfood this PR with: 
 curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12608Or 
 iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12608" | 
    
| 
           @copilot Aspire.Hosting.Azure.Tests.AzureDeployerTests.DeployAsync_WithAzureFunctionsProject_Works The teats like this in the azure deployer tests need to be updated to mock the IContainerRuntime LoginToRegisrryAsynd to we dont attenpt to log into fake registries  | 
    
…Async - Added ASPIRECONTAINERRUNTIME001 pragma suppression to AzureDeployerTests - Added using for Aspire.Hosting.Tests.Publishing namespace - Made FakeContainerRuntime public so Azure tests can use it - Added IContainerRuntime parameter to ConfigureTestServices - Updated DeployAsync_WithAzureFunctionsProject_Works test to use FakeContainerRuntime - Changed assertions from checking "az acr login" commands to checking IContainerRuntime.LoginToRegistryAsync calls - Verified login uses correct registry endpoint and username (00000000-0000-0000-0000-000000000000) Additional tests still need updating (7 more tests with acr login assertions) Co-authored-by: davidfowl <[email protected]>
…untime - Updated all remaining tests that checked for "az acr login" commands - Changed all assertions to check IContainerRuntime.LoginToRegistryAsync calls - Tests verify correct registry endpoints (.azurecr.io suffix) - Tests verify correct username (00000000-0000-0000-0000-000000000000) - Added fakeContainerRuntime to all tests that need registry login verification - Removed unused mockProcessRunner variables where not needed - All 9 tests with acr login assertions now updated (0 acr login checks remaining) - Build successful with no errors or warnings Co-authored-by: davidfowl <[email protected]>
…eContainerRuntime to ConfigureTestServices - Test was creating fakeContainerRuntime but not passing it to ConfigureTestServices - ConfigureTestServices was creating a new FakeContainerRuntime instance when containerRuntime parameter was null - Test was checking the wrong FakeContainerRuntime instance, causing WasLoginToRegistryCalled assertion to fail - Fixed by passing containerRuntime: fakeContainerRuntime parameter to ConfigureTestServices call - Test now passes successfully Co-authored-by: davidfowl <[email protected]>
          
 Updated the Azure deployer tests to mock IContainerRuntime.LoginToRegistryAsync (commit 33c8aa7). Made FakeContainerRuntime public and added it to ConfigureTestServices. Updated DeployAsync_WithAzureFunctionsProject_Works to verify LoginToRegistryAsync calls instead of "az acr login" commands. Currently updating the remaining 7 tests that check for acr login assertions.  | 
    
| 
           @copilot Assert.True() Failure  | 
    
- Changed IContainerRuntime from method parameter to constructor dependency in AcrLoginService - Updated IAcrLoginService.LoginAsync to remove containerRuntime parameter - Simplified caller in AzureEnvironmentResourceHelpers - no longer needs to resolve IContainerRuntime - Get provisioning context from AzureEnvironmentResource.ProvisioningContextTask (same pattern as AzureBicepResource) - Removed IProvisioningContextProvider dependency - not needed with new approach - Added ASPIREAZURE001 pragma suppression for AzureEnvironmentResource usage - Restored IResourceContainerImageBuilder using directive that was accidentally removed - Better separation of concerns - container runtime is now encapsulated in the service Co-authored-by: davidfowl <[email protected]>
| 
           @copilot There are test failures in AzureDeployerTests  | 
    
- Created FakeAcrLoginService to mock IAcrLoginService in tests - Added IAcrLoginService registration to both ConfigureTestServices methods - Added IContainerRuntime registration to ConfigureTestServicesWithFileDeploymentStateManager - Prevents actual HTTP calls to ACR OAuth2 endpoint during test execution - Tests can now run without making real network requests or requiring Azure credentials Co-authored-by: davidfowl <[email protected]>
          
 Fixed in commit f4e87fd. Created  Added the service registration to both  Tests now run without making real network requests or requiring Azure credentials. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
 If you need me to access, download, or install something from one of these locations, you can either: 
  | 
    
| 
           @copilot Make tenantId in IAcrLoginService.LoginAsync required. The caller should throw if there's no tentant, but this interface should always assume there is one.  | 
    
- Changed tenantId parameter from string? to string (non-nullable) - Added ArgumentException.ThrowIfNullOrWhiteSpace validation for tenantId in AcrLoginService - Simplified OAuth2 exchange - tenant is always included in form data - Updated caller to throw if tenant ID is not available from provisioning context - Updated FakeAcrLoginService to match new signature - Removed conditional tenant ID handling since it's now required Co-authored-by: davidfowl <[email protected]>
          
 Fixed in commit db3f0bb. Made   | 
    
| 
           @copilot There are 4 failing tests in AzureDeployerTests Assert.True() Failure Assert.True() Failure Assert.True() Failure Assert.True() Failure  | 
    
…ructor parameter - FakeAcrLoginService constructor now takes IContainerRuntime parameter to match interface - Updated both ConfigureTestServices methods to pass IContainerRuntime to FakeAcrLoginService constructor - All 4 failing tests now pass (DeployAsync_WithProjectResource_Works, DeployAsync_WithMultipleComputeEnvironments_Works, DeployAsync_WithAzureFunctionsProject_Works, DeployAsync_WithDockerfile_Works) Co-authored-by: davidfowl <[email protected]>
          
 Fixed in commit 6a317ed. The issue was that  Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
 If you need me to access, download, or install something from one of these locations, you can either: 
  | 
    
| builder.Services.AddSingleton<IResourceContainerImageBuilder, MockImageBuilder>(); | ||
| builder.Services.AddSingleton<IContainerRuntime>(new FakeContainerRuntime()); | ||
| builder.Services.AddSingleton<IAcrLoginService, FakeAcrLoginService>(); | ||
| builder.Services.AddSingleton<IAcrLoginService>(sp => new FakeAcrLoginService(sp.GetRequiredService<IContainerRuntime>())); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot this change cam be reverted.
| 
           @captainsafia this works but feels big for 13. We can do the revert to AzureCliCredential for 13 and take this in 13.1.  | 
    
| 
           @copilot Make these logs debug level in AcrLoginService: 2:24:33 (login-to-acr-env) i [INF] Start processing HTTP request POST https://envacrs37zu4or36nkm.azurecr.io/oauth2/exchange  | 
    
| // Ensure the write completes before closing | ||
| writer.BaseStream.Flush(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Ensure the write completes before closing | |
| writer.BaseStream.Flush(); | 
I don't think these are necessary.
| internal sealed class AcrLoginService : IAcrLoginService | ||
| { | ||
| private const string AcrUsername = "00000000-0000-0000-0000-000000000000"; | ||
| private const string AcrScope = "https://containerregistry.azure.net/.default"; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to change if we are using sovereign clouds, like Mooncake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might.
| var responseBody = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); | ||
| 
               | 
          ||
| if (!response.IsSuccessStatusCode) | ||
| { | ||
| var truncatedBody = responseBody.Length <= 1000 ? responseBody : responseBody[..1000] + "…"; | ||
| throw new HttpRequestException( | ||
| $"POST /oauth2/exchange failed {(int)response.StatusCode} {response.ReasonPhrase}. Body: {truncatedBody}", | ||
| null, | ||
| response.StatusCode); | ||
| } | ||
| 
               | 
          ||
| var tokenResponse = await response.Content.ReadFromJsonAsync<AcrRefreshTokenResponse>(cancellationToken: cancellationToken).ConfigureAwait(false); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to read the response.Content twice. If we are going to read it as a string, we can just parse that string as Json.
| { | ||
| public static async Task LoginToRegistryAsync(IContainerRegistry registry, PipelineStepContext context) | ||
| { | ||
| var processRunner = context.Services.GetRequiredService<IProcessRunner>(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can IProcessRunner be deleted now?
Refactor ACR Authentication to Use Azure SDK's TokenCredential
This PR refactors ACR (Azure Container Registry) authentication to use the Azure SDK's TokenCredential with proper OAuth2 token exchange instead of
az acr login.Implementation Complete ✅
All implementation tasks successfully completed!
Key Changes
Architecture & Design
[Experimental("ASPIRECONTAINERRUNTIME001")], includesLoginToRegistryAsyncmethodAcrLoginServiceACR OAuth2 Authentication Flow
https://containerregistry.azure.net/.defaulthttps://<registry>.azurecr.io/oauth2/exchange00000000-0000-0000-0000-000000000000Infrastructure
Testing
Files Changed
src/Aspire.Hosting/Publishing/IContainerRuntime.cs- Public experimental interface with LoginToRegistryAsyncsrc/Aspire.Hosting/Publishing/ContainerRuntimeBase.cs- Implementation with stdin password supportsrc/Aspire.Hosting/Dcp/Process/ProcessSpec.cs- StandardInputContent propertysrc/Aspire.Hosting/Dcp/Process/ProcessUtil.cs- Stdin writing with flushsrc/Aspire.Hosting.Azure/IAcrLoginService.cs- Service interface (tenantId required)src/Aspire.Hosting.Azure/AcrLoginService.cs- OAuth2 exchange implementationsrc/Aspire.Hosting.Azure/AzureEnvironmentResourceHelpers.cs- Uses IAcrLoginServicesrc/Aspire.Hosting.Azure/Provisioning/AzureProvisionerExtensions.cs- DI registrationtests/Aspire.Hosting.Tests/Publishing/FakeContainerRuntime.cs- Public for teststests/Aspire.Hosting.Azure.Tests/FakeAcrLoginService.cs- Mock servicetests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs- Updated test mocksBuild & Test Status
✅ All projects build successfully
✅ All 21 Azure deployer tests passing
✅ No warnings or errors
Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.