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

Expose azcontainerregistry.authenticationClient #20571

Closed
PSanetra opened this issue Apr 7, 2023 · 25 comments
Closed

Expose azcontainerregistry.authenticationClient #20571

PSanetra opened this issue Apr 7, 2023 · 25 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Container Registry customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@PSanetra
Copy link

PSanetra commented Apr 7, 2023

Feature Request

To interact with the ACR api it is needed to get an ACR refresh token via the /oauth2/exchange endpoint and then using the refresh token to get an access token via the /oauth2/endpoint, like it is described in the ACR docs.

Third party libraries which interact with container and artifact registry like oras-go already support using refresh tokens to get an access token, but currently the call to /oauth2/exchange needs to be implemented manually. It would be great if the azcontainerregistry.authenticationClient was exposed, so that ExchangeAADAccessTokenForACRRefreshToken can be used from the offical Azure go sdk.

Related: oras-project/oras-go#476

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Apr 7, 2023
@ghost
Copy link

ghost commented Apr 7, 2023

Hi @PSanetra. Thank you for your feedback and we will look into it soon. Meanwhile, feel free to share your experience using the Azure SDK in this survey.

@jhendrixMSFT jhendrixMSFT added Container Registry Client This issue points to a problem in the data-plane of the library. and removed needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Apr 7, 2023
@ghost ghost added the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Apr 7, 2023
@tadelesh
Copy link
Member

tadelesh commented Apr 10, 2023

@PSanetra Thank you for the feature request. When we reviewed our public surface of azcontianerregistry, we think the auth process should be transparent to our SDK users (when using any registry API, SDK will help to do the auth correctly), so we decided to hide the auth related API. Could you elaborate your scenario of using token exchange API that we could evaluate whether expose those API?

cc: @JeffreyRichter

@PSanetra
Copy link
Author

@tadelesh yes, see for example the discussion in the oras go library: oras-project/oras-go#476

Due to missing support to call the token exchage via the SDK, it is needed to implement the token exchange manually. I think generally manual implementation is currently needed when a client should be implemented that should be able to communicate with generic container or artifact registries.

@tadelesh
Copy link
Member

If I understand correctly, you mean you need to use oras-go to communicate with generic registries, only need azure sdk to do the auth, right?

@JeffreyRichter could you help to evaluate this request?

@PSanetra
Copy link
Author

@tadelesh exactly 🙂

@JeffreyRichter
Copy link
Member

Let me see if I understand...
Ther is a package (oras-go) that works for any kind of container registry (Azure or not) but, to authenticate with Azure, having access to our Go SDK's authenticationClient would make thing easier for people using the oras-go (or similar) package and so some developer might want to take a dependency on the entire Azure Go container registry package just to use this one authenticationClient type defined within it. Am I understanding this right?

If I am, this dependency brings in a ton of other dependencies (like Azure Core with HTTP pipeline/policy functionality, etc.).

We also do not test this scenario (using our authenticationClient with some other non-Azure SDK package) and can't formally support this scenario even if we were to export this type.

@PSanetra
Copy link
Author

@JeffreyRichter yes, it would be nice if accessing Azure APIs, especially proprietary authentication, would be supported by the Azure Go SDK. For generic interaction with the container registry a generic library like oras is used, which supports more features than the azcontainerregistry library. E.g. copying artifacts between registries.

As far as I see the token exchange endpoint is a public Azure Container Registry API (See docs). That api is just not publicly exposed in the azcontainerregistry library.

@PSanetra
Copy link
Author

@JeffreyRichter I have to add in my use case that specific endpoint is not the only reason to reference the azure-sdk-for-go. I am also using azidentity library to use a TokenCredential to acquire an AAD access token, which is a prerequisite to use the endpoint mentioned in this issue.

Additionally the azcontainerregistry library is already in use to reference the required token audience via cloud.Configuration.AzCloudConfig.Services[azcontainerregistry.ServiceName].Audience, which depends on the Azure cloud environment (public, government, china). So in practice using that endpoint is not the only reason to use azcontainerregistry.

@tadelesh
Copy link
Member

tadelesh commented Jun 5, 2023

@PSanetra I fully understand your requirement here. Since we want to provide a fully tested SDKs, we are still waiting for more feedbacks to see if it is a common scenario for our customers. I'll keep update in this issue.

@akashsinghal
Copy link

+1 on exposing the authentication client in the sdk. I'm contributing to project Ratify. We also leverage oras-go to interact with ACR. Currently we are using a deprecated 2019 preview version of the sdk which does expose the client to exchange the AAD token for the registry refresh token. If we want to upgrade to the latest sdk, we need this functionality.

Any update on supporting this?

@tadelesh
Copy link
Member

@MartinForReal could you also put your requirement and user scenario here? we could evaluate it again.

@stevekuznetsov
Copy link
Contributor

This is identical to a requirement we have in github.com/Azure/MSI-ACRPull - we are attempting to project ACR credentials into a Kubernetes secret to allow the Kubelet to use managed identities and workload identities for image pulls. We do not require an ACR client, but only the ability to do the AAD access token -> ACR refresh token -> ACR access token flow ourselves. We will manage rotation as necessary.

I understand the hesitation to expose direct access to this information for an ACR client, as transparent credential management is nice. I hope you'd be OK with a separate package that exposes the authentication code in a stand-alone capacity, which the ACR client happens to use under the hood.

@tadelesh
Copy link
Member

tadelesh commented Aug 2, 2024

@JeffreyRichter Since the requirement is only exposing the authentication client (the auto generated code is public, but we hide them manually), and we have tests for the client, I think we could do that. What's your opinion?

@stevekuznetsov
Copy link
Contributor

stevekuznetsov commented Aug 2, 2024

@tadelesh @JeffreyRichter please take a look at a proof-of-concept decoupling of the client policy and token caching and management in my PR #23272.

We certainly could take the route that @tadelesh mentions and simply expose the *authenticationClient methods, requests, and responses, and perhaps that is a good idea regardless (why would we hide publicly-available APIs generated from OpenAPI spec from consumers?), I do imagine most consumers will end up re-implementing the caching, so I believe exposing that would also be excellent, especially since the *temporal.Resource is from an internal package, and because ACR doesn't seem to divulge the refresh token expiry directly, so users managing this will need to parse the JWT.

@JeffreyRichter
Copy link
Member

@chlowell can you please review this request since it is auth related.

@stevekuznetsov
Copy link
Contributor

I put up a PR to expose the auth API client, constructor, options, etc #23285

@RickWinter RickWinter assigned chlowell and unassigned tadelesh Aug 5, 2024
@chlowell
Copy link
Member

Closed by #23285

@tadelesh
Copy link
Member

@chlowell shall we have an ad-hoc release or release with next cycle?

@chlowell
Copy link
Member

Next cycle. So far as I know, no one is blocked on a published release.

@akashsinghal
Copy link

Thanks for adding this support! @chlowell is there an approximate time frame of the next release?

@chlowell
Copy link
Member

September 17 is the next scheduled release date.

@shahramk64
Copy link

@chlowell Just pinging to see if the sdk is released? I can't seem to find it in the new releases.

@chlowell
Copy link
Member

Thanks for pinging! It appears we all assumed someone else was shipping this one. I've started the release process, it will publish tomorrow as v0.2.2

@shahramk64
Copy link

@chlowell Thanks.

@chlowell
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Container Registry customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

8 participants