Add authentication support for ACR #19926

pallavit · 2021-03-17T22:02:17Z

ACR follows a challenge based authorization and functions as follows

For example
GET {url} /api/v1/acr/repositories translates into the following calls.

Step1: GET /api/v1/acr/repositories
Return Header: 401: www-authenticate header - Bearer realm="{url}",service="{serviceName}",scope="{scope}",error="invalid_token".

Step2: Parse the serviceName, scope from the service.

Step3: POST /api/oauth2/exchange
Request Body : {service, scope, grant-type, aadToken with ARM scope}
Response Body: {acrRefreshToken}

Step4: POST /api/oauth2/token
Request Body: {acrRefreshToken, scope, grant-type}
Response Body: {acrAccessToken}

Step5: GET /api/v1/acr/repositories
Request Header: {Bearer acrTokenAccess}

In order to prevent user from having to do this tedious process we have decided to build a convenience layer on ACR - that does this under the hood by exposing an ACR specific policy.

pallavit · 2021-03-17T22:05:11Z

...ry/src/main/java/com/azure/containers/containerregistry/authentication/AccessTokenCache.java

+/**
+ * A token cache that supports caching a token and refreshing it.
+ */
+class AccessTokenCache {


Ideally this class can be exposed via the experimental package and we can consume it.
@g2vinay - I will sync up with you on this. #Resolved

As per the offline discussion:
Given that ACR has a separate sts service and issues its own independent tokens with custom expiry.
It makes sense to have independent caching logic for it, as it has the potential to evolve in the future if we choose to cache in the ACR tokens per scope. (That'd be a great optimization from a performance perspective if we pull it off).

The AAD token caching logic in azure-core can evolve too in the future and will give priority to AAD tokens specifically. So, the two implementations are not directly tied together, and having independent caches, allows to optimize and augment the caching logic best for ACR and AAD respectively.

Seems both the caches are implementation details, once ACR caching logic has stabilized and evolved we can always look into merging them together without breaking public API, if possible given their mature state at that time.

#Resolved

I created an issue to track it.

In reply to: 597131284 [](ancestors = 597131284)

pallavit · 2021-03-17T22:07:39Z

...a/com/azure/containers/containerregistry/implementation/authentication/AccessTokensImpl.java

+import reactor.core.publisher.Mono;
+
+/** An instance of this class provides access to all the operations defined in AccessTokensService. */
+public final class AccessTokensImpl {


public [](start = 0, length = 6)

Why do we generate these classes as public? #Resolved

I had to hand author the generated files due to the discrepancy between www-encoded form not correctly supported. Also, I could not use the builder pattern since this class is a hierarchial pattern and has only one builder. And it needed the policy that had to be added to the builder to consume this class. Unfortunately ACR's STS goes through the same rest endpoints. Let me know if there is a better way to do it.

In reply to: 596415910 [](ancestors = 596415910)

These classes are generated as public since Java classes cannot be accessible across package boundaries without either being protected or public. #Resolved

I missed that they were in a different package.

In reply to: 597119525 [](ancestors = 597119525)

@pallavit could you please file a bug in autorest.java repo with steps to reproduce and the expected code that should be generated? Ideally, we should not modify any generated code manually as it would be overwritten when we regenerate this code. #Resolved

There already is a bug and @jianghaolu is aware of it.

In reply to: 597954987 [](ancestors = 597954987)

pallavit · 2021-03-17T22:08:48Z

sdk/containerregistry/azure-containers-containerregistry/swagger/readme.md

+autorest --java --use=C:/work/autorest.java
+```
+
+### Code generation settings


This file and the swagger will change. The swagger is still being worked upon. #Resolved

alzimmermsft · 2021-03-18T17:57:50Z

...om/azure/containers/containerregistry/authentication/ContainerRegistryCredentialsPolicy.java

+ *        Request Header: {Bearer acrTokenAccess}
+ *
+ */
+public class ContainerRegistryCredentialsPolicy implements HttpPipelinePolicy {


Generally, we want to eagerly mark classes as final, equivalent to sealed in .NET. It's a non-breaking change to remove final in the future if required but marking as final is breaking #Resolved

Done. Also, this class was not meant to be public.

In reply to: 597116660 [](ancestors = 597116660)

...om/azure/containers/containerregistry/authentication/ContainerRegistryCredentialsPolicy.java

alzimmermsft · 2021-03-18T18:01:38Z

...a/com/azure/containers/containerregistry/implementation/authentication/AccessTokensImpl.java

+import reactor.core.publisher.Mono;
+
+/** An instance of this class provides access to all the operations defined in AccessTokensService. */
+public final class AccessTokensImpl {


These classes are generated as public since Java classes cannot be accessible across package boundaries without either being protected or public. #Resolved

alzimmermsft · 2021-03-18T18:03:05Z

...com/azure/containers/containerregistry/authentication/ContainerRegistryClientPolicyTest.java

+package com.azure.containers.containerregistry.authentication;
+
+import com.azure.core.credential.AccessToken;
+import com.azure.core.http.*;


nit: Our guidelines don't allow * imports #Resolved

srnagar · 2021-03-19T20:12:10Z

...gistry/src/main/java/com/azure/containers/containerregistry/authentication/package-info.java

+/**
+ * This package contains classes for the container registry project.
+ */
+package com.azure.containers.containerregistry.authentication;


I don't see any classes in this package. Is this package-info required? #Resolved

Missed this. I moved all the files from here to the implementation package.

In reply to: 597948739 [](ancestors = 597948739)

srnagar · 2021-03-19T20:13:47Z

...m/azure/containers/containerregistry/implementation/authentication/AccessTokenCacheImpl.java

+                    Mono<AccessToken> fallback;
+
+                    Supplier<Mono<AccessToken>> tokenSupplier = () -> {
+                        Objects.requireNonNull(this.tokenRequestContext);


Don't throw an exception when returning a reactive publisher (Mono/Flux). Instead return Mono.error() or Flux.error() #Resolved

srnagar · 2021-03-19T20:15:18Z

...m/azure/containers/containerregistry/implementation/authentication/AccessTokenCacheImpl.java

+
+    private Supplier<Mono<? extends AccessToken>> retrieveToken(ContainerRegistryTokenRequestContextImpl tokenRequestContext) {
+        return () -> {
+            try {


This whole supplier should be wrapped with Mono.defer() to evaluate the need to refresh at subscription-time instead of at assembly-time. #Resolved

The caller actually does that - retrieveToken call just above this method. Since this is a private method chose not to do it.

In reply to: 597950212 [](ancestors = 597950212)

srnagar · 2021-03-19T20:25:03Z

...a/com/azure/containers/containerregistry/implementation/authentication/AccessTokensImpl.java

+import reactor.core.publisher.Mono;
+
+/** An instance of this class provides access to all the operations defined in AccessTokensService. */
+public final class AccessTokensImpl {


@pallavit could you please file a bug in autorest.java repo with steps to reproduce and the expected code that should be generated? Ideally, we should not modify any generated code manually as it would be overwritten when we regenerate this code. #Resolved

srnagar · 2021-03-19T20:29:19Z

...ners/containerregistry/implementation/authentication/ContainerRegistryCredentialsPolicy.java

+ * <p>Step5: GET /api/v1/acr/repositories
+ * Request Header: {Bearer acrTokenAccess}</p>
+ */
+public class ContainerRegistryCredentialsPolicy implements HttpPipelinePolicy {


Generally, we make all pipeline policies public so that users add the policy to their own HTTP pipeline. #Resolved

The policy is not public - just because we are still ironing out what is the right handshake with experimental package. Having said that, this policy may not be public even in future - since its a convenience layer added for the customer.

In reply to: 597957262 [](ancestors = 597957262)

srnagar · 2021-03-19T20:29:57Z

...rs/containerregistry/implementation/authentication/ContainerRegistryTokenCredentialImpl.java

+ * Token credentials representing the container registry refresh token.
+ * This token is unique per registry operation.
+ */
+class ContainerRegistryTokenCredentialImpl {


As discussed offline, will this type be implementing TokenCredential? #Resolved

it may. As of now since the surface is non-public, we have the flexibility of changing it in future.

In reply to: 597957584 [](ancestors = 597957584)

srnagar · 2021-03-19T20:30:31Z

...ontainerregistry/implementation/authentication/ContainerRegistryTokenRequestContextImpl.java

+/**
+ * A token request context associated with a given container registry token.
+ */
+class ContainerRegistryTokenRequestContextImpl {


Don't need to append Impl to class names if they are already in implementation package. #Resolved

srnagar · 2021-03-19T20:36:15Z

.../com/azure/containers/containerregistry/implementation/authentication/RefreshTokensImpl.java

@@ -0,0 +1,123 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.


Is this class handwritten? The license header doesn't have this line // Code generated by Microsoft (R) AutoRest Code Generator. which is generally included for all generated files. #Resolved

Yes, as mentioned in another file, a bug tracking this already exists.

In reply to: 597960498 [](ancestors = 597960498)

alzimmermsft · 2021-03-19T21:01:14Z

sdk/containerregistry/azure-containers-containerregistry/README.md


 ## Contributing
-<Add contributing steps>
+<Instructions on how to contribute>.


Should be able to copy the template project's section for this #Resolved

alzimmermsft · 2021-03-19T21:02:50Z

...gistry/src/main/java/com/azure/containers/containerregistry/authentication/package-info.java

+// Licensed under the MIT License.
+
+/**
+ * This package contains classes for the container registry project.


Suggested change

* This package contains classes for the container registry project.

* This package contains authentication classes used by Azure Container Registry.

``` #Resolved

alzimmermsft · 2021-03-19T22:30:05Z

sdk/containerregistry/azure-containers-containerregistry/src/main/java/module-info.java

@@ -2,5 +2,6 @@
 // Licensed under the MIT License.

 module com.azure.containers.containerregistry {
+    requires transitive com.azure.core;
    exports com.azure.containers.containerregistry;


If you want the classes in com.azure.containers.containerregistry.authentication to be available outside of the project you'll need to add it as an export here. #Resolved

Don't want that right now.

In reply to: 598005797 [](ancestors = 598005797)

pallavit · 2021-03-22T19:08:10Z

/check-enforcer evaluate

pallavit · 2021-03-22T20:19:42Z

/check-enforcer evaluate

New API Path added "datawarehousequeries" in sqlpool (Azure#19926) * Path datawarehousequeries added in sqlpool * Resolved Conflict - removed space * Changes in definitions * Changes in example-getdatawarehousequeries.json * added object in sqlpool * Updated datawarehousequeries json * modified sqlpool definitions * Removed properties in example json * added Removed items * added response in example json for dataWarehouseQueries * Added properties in definitions * updated mismatch type * modifications in id path * Properties changes * changed Value to Properties * properties to value changes * removed Nextlink * Removed array * updated description * Added type in response * chnages in responses * changes in readme * Null Changes * Changes in Readme added Datawarehousequeries * added missing tag in batch * Changed Value Object * Reversed type changes * Camel case changes * Camel Changes * Removed Extra Value Properties * Added Properties * Description Changes * resourceClass values * Added Custom Words Co-authored-by: VamshiKrishna Chirra (Quadrant Resource) <vamshikrishna.chirra@quadrantresource.com>

pallavit added the Container Registry label Mar 17, 2021

pallavit requested a review from alzimmermsft March 17, 2021 22:02

pallavit self-assigned this Mar 17, 2021

pallavit requested a review from jianghaolu as a code owner March 17, 2021 22:02

pallavit commented Mar 17, 2021

View reviewed changes

alzimmermsft reviewed Mar 18, 2021

View reviewed changes

pallavit force-pushed the pallavit/acrsdkauth branch from 7cbec67 to c052370 Compare March 19, 2021 18:57

pallavit requested review from JonathanGiles and mssfang as code owners March 19, 2021 18:57

pallavit linked an issue Mar 19, 2021 that may be closed by this pull request

Design and implement AAD auth #19939

Closed

pallavit force-pushed the pallavit/acrsdkauth branch from d43ea1e to a1afb2f Compare March 19, 2021 20:24

srnagar reviewed Mar 19, 2021

View reviewed changes

alzimmermsft reviewed Mar 19, 2021

View reviewed changes

pallavit force-pushed the pallavit/acrsdkauth branch 3 times, most recently from 3495ee9 to e287bcd Compare March 22, 2021 17:03

pallavit requested review from danieljurek, JimSuplizio, mitchdenny and weshaggard as code owners March 22, 2021 17:03

ACR authentication support

7559df9

pallavit force-pushed the pallavit/acrsdkauth branch from e287bcd to 7559df9 Compare March 22, 2021 17:17

annelo-msft mentioned this pull request Mar 22, 2021

Add ACR challenge-based authentication and remove basic auth policy Azure/azure-sdk-for-net#19696

Merged

pallavit removed request for mitchdenny, danieljurek, weshaggard and JimSuplizio March 22, 2021 20:17

pallavit removed request for mssfang, jianghaolu and JonathanGiles March 22, 2021 20:17

alzimmermsft approved these changes Mar 22, 2021

View reviewed changes

pallavit merged commit 5423e12 into Azure:master Mar 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authentication support for ACR #19926

Add authentication support for ACR #19926

pallavit commented Mar 17, 2021 •

edited

Loading

pallavit Mar 17, 2021 •

edited

Loading

g2vinay Mar 18, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

pallavit Mar 17, 2021 •

edited

Loading

pallavit Mar 18, 2021

alzimmermsft Mar 18, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

srnagar Mar 19, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

pallavit Mar 17, 2021 •

edited

Loading

alzimmermsft Mar 18, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

alzimmermsft Mar 18, 2021 •

edited by pallavit

Loading

alzimmermsft Mar 18, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

srnagar Mar 19, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

alzimmermsft Mar 19, 2021 •

edited by pallavit

Loading

alzimmermsft Mar 19, 2021 •

edited by pallavit

Loading

alzimmermsft Mar 19, 2021 •

edited by pallavit

Loading

pallavit Mar 19, 2021

pallavit commented Mar 22, 2021

pallavit commented Mar 22, 2021

		@@ -0,0 +1,123 @@
		// Copyright (c) Microsoft Corporation. All rights reserved.

	* This package contains classes for the container registry project.
	* This package contains authentication classes used by Azure Container Registry.
	``` #Resolved

Add authentication support for ACR #19926

Add authentication support for ACR #19926

Conversation

pallavit commented Mar 17, 2021 • edited Loading

pallavit Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

g2vinay Mar 18, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pallavit Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alzimmermsft Mar 18, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srnagar Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pallavit Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

alzimmermsft Mar 18, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alzimmermsft Mar 18, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

alzimmermsft Mar 18, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

srnagar Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srnagar Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

srnagar Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srnagar Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

srnagar Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srnagar Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srnagar Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

srnagar Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alzimmermsft Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

alzimmermsft Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

alzimmermsft Mar 19, 2021 • edited by pallavit Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pallavit commented Mar 22, 2021

pallavit commented Mar 22, 2021

pallavit commented Mar 17, 2021 •

edited

Loading

pallavit Mar 17, 2021 •

edited

Loading

g2vinay Mar 18, 2021 •

edited by pallavit

Loading

pallavit Mar 17, 2021 •

edited

Loading

alzimmermsft Mar 18, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

pallavit Mar 17, 2021 •

edited

Loading

alzimmermsft Mar 18, 2021 •

edited by pallavit

Loading

alzimmermsft Mar 18, 2021 •

edited by pallavit

Loading

alzimmermsft Mar 18, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

srnagar Mar 19, 2021 •

edited by pallavit

Loading

alzimmermsft Mar 19, 2021 •

edited by pallavit

Loading

alzimmermsft Mar 19, 2021 •

edited by pallavit

Loading

alzimmermsft Mar 19, 2021 •

edited by pallavit

Loading