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

Add AuthenticationScope to ACR ClientOptions #21493

Merged
merged 16 commits into from
Jun 4, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions sdk/containerregistry/Azure.Containers.ContainerRegistry/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,37 @@ az acr create --name MyContainerRegistry --resource-group MyResourceGroup --loca

### Authenticate the client

The [Azure Identity library][identity] provides easy Azure Active Directory support for authentication.
The [Azure Identity library][identity] provides easy Azure Active Directory support for authentication. You can set the environment variables `AZURE_CLIENT_ID`, `AZURE_TENANT_ID`, and `AZURE_CLIENT_SECRET` to configure `DefaultAzureCredential` to authenticate with AAD. For more information refer to [Azure Identity environment variables](https://github.com/Azure/azure-sdk-for-net/tree/master/sdk/identity/Azure.Identity#environment-variables) or [DefaultAzureCredential](https://github.com/Azure/azure-sdk-for-net/tree/master/sdk/identity/Azure.Identity#defaultazurecredential).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe only EnvironmentCredential will use those environment variables, so depending on the current execution environment or the options passed to DAC, these may be ignored.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't EnvironmentCredential the first the the DAC chain? Are there cases where this could be bypassed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to push our readme's away from documenting using environment variables with the DAC as this requires setting up a service principal. Instead I think service client readme's should document that users can log in through the CLII and link to the Identity readme for more ways to authenticate your dev environment when using the DAC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't EnvironmentCredential the first the the DAC chain? Are there cases where this could be bypassed?

Yes -

public bool ExcludeEnvironmentCredential { get; set; }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schaabs can you point me to an example of a service client README that does this the way you'd like to see it, so I can copy it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this to reflect Scott's feedback


```C#
// Create a ContainerRegistryClient that will authenticate through Active Directory
// Create a ContainerRegistryClient that will authenticate through Azure Active Directory
Uri endpoint = new Uri(Environment.GetEnvironmentVariable("REGISTRY_ENDPOINT"));
ContainerRegistryClient client = new ContainerRegistryClient(endpoint, new DefaultAzureCredential());
```

Note that these samples assume you have a `REGISTRY_ENDPOINT` environment variable set, which is the URL including the name of the login server and the `https://` prefix.
Note that this samples assume you have a `REGISTRY_ENDPOINT` environment variable set. This would be set to a string containing the `https://` prefix and the name of the login server, for example "https://myregistry.azurecr.io".

#### National Clouds

To authenticate with a registry in a [National Cloud](https://docs.microsoft.com/azure/active-directory/develop/authentication-national-cloud), you will need to make the following additions to your client configuration:

- Set the `AuthorityHost` in the credential options or via the `AZURE_AUTHORITY_HOST` environment variable
- Set the `AuthenticationScope` in `ContainerRegistryClientOptions`

```C#
// Create a ContainerRegistryClient that will authenticate through AAD in the China national cloud
Uri endpoint = new Uri(Environment.GetEnvironmentVariable("REGISTRY_ENDPOINT"));
ContainerRegistryClient client = new ContainerRegistryClient(endpoint,
new DefaultAzureCredential(
new DefaultAzureCredentialOptions()
{
AuthorityHost = AzureAuthorityHosts.AzureChina
}),
new ContainerRegistryClientOptions()
{
AuthenticationScope = "https://management.chinacloudapi.cn/.default"
});
```

For more information on using AAD with Azure Container Registry, please see the service's [Authentication Overview](https://docs.microsoft.com/azure/container-registry/container-registry-authentication).

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ public partial class ContainerRegistryClient
private readonly ClientDiagnostics _clientDiagnostics;
private readonly ContainerRegistryRestClient _restClient;
private readonly AuthenticationRestClient _acrAuthClient;
private readonly string AcrAadScope = "https://management.core.windows.net/.default";

/// <summary>
/// Initializes a new instance of the <see cref="ContainerRegistryClient"/> for managing container images and artifacts,
Expand Down Expand Up @@ -80,7 +79,7 @@ public ContainerRegistryClient(Uri endpoint, TokenCredential credential, Contain
_acrAuthPipeline = HttpPipelineBuilder.Build(options);
_acrAuthClient = new AuthenticationRestClient(_clientDiagnostics, _acrAuthPipeline, endpoint.AbsoluteUri);

_pipeline = HttpPipelineBuilder.Build(options, new ContainerRegistryChallengeAuthenticationPolicy(credential, AcrAadScope, _acrAuthClient));
_pipeline = HttpPipelineBuilder.Build(options, new ContainerRegistryChallengeAuthenticationPolicy(credential, options.AuthenticationScope, _acrAuthClient));
_restClient = new ContainerRegistryRestClient(_clientDiagnostics, _pipeline, _endpoint.AbsoluteUri);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,17 @@ public class ContainerRegistryClientOptions : ClientOptions
{
internal string Version { get; }

/// <summary>
/// Gets or sets the authentication scope to use for authentication with AAD.
/// This defaults to the Azure Resource Manager "Azure Global" scope. To
/// connect to a different cloud, set this value to "&lt;resource-id&gt;/.default",
/// where &lt;resource-id&gt; is one of the Resource IDs listed at
/// https://docs.microsoft.com/azure/active-directory/managed-identities-azure-resources/services-support-managed-identities#azure-resource-manager.
/// For example, to connect to the Azure Germany cloud, create a client with
/// <see cref="AuthenticationScope"/> set to "https://management.microsoftazure.de/.default".
/// </summary>
public string AuthenticationScope { get; set; } = "https://management.azure.com/.default";
annelo-msft marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// </summary>
/// <param name="version"></param>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@

using System.Collections.Generic;
using Azure.Core.TestFramework;
using Azure.Identity;
using NUnit.Framework;
using Task = System.Threading.Tasks.Task;

namespace Azure.Containers.ContainerRegistry.Tests
{
public class ContainerRegistryClientLiveTests : ContainerRegistryRecordedTestBase
{
public ContainerRegistryClientLiveTests(bool isAsync) : base(isAsync)
public ContainerRegistryClientLiveTests(bool isAsync) : base(isAsync, RecordedTestMode.Record)
{
}

Expand Down Expand Up @@ -152,5 +153,15 @@ public void CanDeleteRepostitory_Anonymous()

Assert.ThrowsAsync<RequestFailedException>(() => client.DeleteRepositoryAsync(repository));
}

[RecordedTest]
public void InvalidAuthenticationScope_FailsToAuthenticate()
{
// Arrange
string repository = $"library/hello-world";
var client = CreateClient(false, "https://management.azure.other-cloud/.default");

Assert.ThrowsAsync<AuthenticationFailedException>(() => client.DeleteRepositoryAsync(repository));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,14 @@ public ContainerRegistryRecordedTestBase(bool isAsync, RecordedTestMode mode) :
Sanitizer = new ContainerRegistryRecordedTestSanitizer();
}

public ContainerRegistryClient CreateClient(bool anonymousAccess = false)
public ContainerRegistryClient CreateClient(bool anonymousAccess = false, string authenticationScope = null)
{
ContainerRegistryClientOptions options = new ContainerRegistryClientOptions();
if (authenticationScope != null)
{
options.AuthenticationScope = authenticationScope;
}

return anonymousAccess ?

InstrumentClient(new ContainerRegistryClient(
Expand All @@ -38,7 +44,7 @@ public ContainerRegistryClient CreateClient(bool anonymousAccess = false)
InstrumentClient(new ContainerRegistryClient(
new Uri(TestEnvironment.Endpoint),
TestEnvironment.Credential,
InstrumentClientOptions(new ContainerRegistryClientOptions())
InstrumentClientOptions(options)
));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,5 +82,26 @@ public async Task SetImagePropertiesAsync()
CanDelete = true
});
}

//public async Task NationalCloudAuth()
//{
// Identity.DefaultAzureCredential credential = new Identity.DefaultAzureCredential(
// new DefaultAzureCredentialOptions()
// {
// AuthorityHost = AzureAuthorityHosts.AzureChina
// });

// ContainerRegistryClient client = new ContainerRegistryClient(new Uri("https://myreg.azurecr.us"),
// new Identity.DefaultAzureCredential(
// new DefaultAzureCredentialOptions()
// {
// AuthorityHost = AzureAuthorityHosts.AzureChina
// }),
// new ContainerRegistryClientOptions()
// {
// AuthenticationScope = "https://management.chinacloudapi.cn/.default"
// });

//}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.