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

[Enhancement]: Support identitytoken based authentication (OAuth2) #841

Closed
lbruun opened this issue Mar 19, 2023 · 17 comments · Fixed by #1124
Closed

[Enhancement]: Support identitytoken based authentication (OAuth2) #841

lbruun opened this issue Mar 19, 2023 · 17 comments · Fixed by #1124
Labels
enhancement New feature or request

Comments

@lbruun
Copy link

lbruun commented Mar 19, 2023

Problem

Some containers registries use OAuth2 protocol for authentication. For example Azure Container Registry.
After obtaining such credentials, Docker's config.json file look something like this:

{
    "auths": {
        "myprivateregistry.azurecr.io": {
            "auth": "MDAwMDAwMDAtMDAwMC0wMDAwLTAwMDAtMDAwMDAwMDAwMDAwOg==",
            "identitytoken": "gwHEWT356ASfg....eUWR"
        }
    }
}

This is a use-case which testcontainers-dotnet cannot handle at the moment.

With the above, testcontainers-dotnet will produce the following exception:

Docker.DotNet.DockerApiException : Docker API responded with status code=InternalServerError, response=
{"message":"Head \"https://myprivateregistry.azurecr.io/v2/myapp/manifests/latest\": 
unauthorized: authentication required, visit https://aka.ms/acr/authorization for more information."}

This is because testcontainers-dotnet completely ignores the identitytoken. It solely looks at the auth value and attempts to extract username/password from that. In the above case the username is 00000000-0000-0000-0000-000000000000 and the password is empty string. It will use those values for authentication which will of course fail.

Note: The above config file works with the Docker CLI, but doesn't work with testcontainers-dotnet.

Solution

Use the identitytoken value if present. Only try to extract username/password from the auth property if the identitytoken is not present.

See https://docs.docker.com/engine/api/v1.42/#section/Authentication

The fix is trivial as Docker.DotNet library has full support for this use-case.

Benefit

Support for Azure Container Registry.
(especially from a CI pipeline)

Alternatives

No known workarounds.

It would actually work as-is if the CI pipeline had a credstore available, because testcontainers-dotnet actually already support this use-case with identitytoken. However, CI workflows typically do not have such facility available, hence the docker login command will put the identitytoken directly in the config file.

Would you like to help contributing this enhancement?

Yes

@lbruun lbruun added the enhancement New feature or request label Mar 19, 2023
@lbruun
Copy link
Author

lbruun commented Mar 20, 2023

Created PR #842 as a proposed fix.

@HofmeisterAn
Copy link
Collaborator

May I ask in which CI environment you are running?

@lbruun
Copy link
Author

lbruun commented Mar 20, 2023

May I ask in which CI environment you are running?

GitHub.

@HofmeisterAn
Copy link
Collaborator

Why don't you use the Docker login action?

@lyager
Copy link

lyager commented Mar 20, 2023

AFAIK, when having the Docker Registry in Azure, the way to login is with az acr, which leaves you with the identitytoken Dockers config.json file?

@lbruun
Copy link
Author

lbruun commented Mar 20, 2023

We can try the Docker login action.

We are currently doing it like this in the GitHub workflow:

  1. First a general login to Azure using the AZ-login action.
  2. Then an az acr login command.

I would guess that the Docker login action simply conflates the two so we would end up with the same ~/.docker/config.json file. But will give it a try.

@lbruun
Copy link
Author

lbruun commented Mar 20, 2023

@HofmeisterAn

Yes, you right.

Doing like this in a GitHub Workflow will not work

 - name: 'Azure CLI login'
   uses: azure/login@v1
   with:
     ...

 - name: 'ACR login'
   run: az acr login --name ${{ vars.ACR_NAME }}

but this will:

- name: Login to ACR
   uses: docker/login-action@v2
   with:
     registry: ${{ vars.ACR_NAME }}
     username: ${{ secrets.AZURE_CLIENT_ID }}
     password: ${{ secrets.AZURE_CLIENT_SECRET }}   

So basically, it really needs to be username/password authentication in order for testcontainers-dotnet to understand it.

Since the change is so trivial (see the PR) and since I think testcontainers-dotnet should have the goal of supporting the same authentication that Docker CLI does, then I think the PR is still worthwhile.

But thanks, @HofmeisterAn , the docker/login-action will do the trick and is a valid workaround for us.

@HofmeisterAn
Copy link
Collaborator

HofmeisterAn commented Mar 20, 2023

Since the change is so trivial (see the PR) and since I think testcontainers-dotnet should have the goal of supporting the same authentication that Docker CLI does, then I think the PR is still worthwhile.

The issue is that I am unsure whether the configuration you provided above is correct or not. It does not seem right to me. In my understanding, az acr login is intended for an individual AD identity, whereas a service principle should be used instead. I could be mistaken on this matter. Nevertheless, having more information and clarity about the appropriate approach would help me in determining the validity of the proposal. I would like to first understand the correct approach. Currently, it seems like more of a workaround, and other (CLI) tools that rely on Docker's login mechanism could also fail with the configuration you have provided. @cristianrgreco @eddumelendez @mdelapenya @kiview any experiences, ideas or thoughts on that topic?

@lbruun
Copy link
Author

lbruun commented Mar 20, 2023

Well, the config I've provided works with the Docker CLI.

From what I can tell from the Docker CLI source code, is that their AuthConfig object will be populated with all fields found in the config file (so including the identitytoken) but always with the username/password too. So in this respect somewhat different from my PR.

This is what I believe the Docker CLI sends to the endpoint with my example:

{
  "username": "00000000-0000-0000-0000-000000000000",
  "serveraddress": "...",
  "identitytoken": "gwHEWT356ASfg....eUWR"
}

(the password - as derived from the auth property - is an empty string so the Docker CLI will leave it out of the JSON object).

So, yes, slightly different than my PR where it would be sending solely this to the endpoint:

{
  "identitytoken": "gwHEWT356ASfg....eUWR"
}

For Azure Container Registry at least, both forms seem to work. But the way the Docker CLI does it should probably be the benchmark.

I've looked at testcontainers-java. Crucial points are here and here. My conclusion is that the underlying Docker library, docker-java, is prepared for it, but testcontainers-java never populates the identitytoken field. So similar situation to testcontainers-dotnet. Therefore, @HofmeisterAn, I think you are right when you say that other libraries might also fail on the config I've provided. But the Docker CLI doesn't and that is the important thing.

I'll update the PR to mimic more precisely what the Docker CLI does.

@mdelapenya
Copy link
Member

In Testcontainers for Go, when autodiscovering the docker auth configs, we leverage a library to read from the credential helpers, and in there, the identity token takes precedence when present (see https://github.com/cpuguy83/dockercfg/blob/main/auth.go#L66-L68 and https://github.com/testcontainers/testcontainers-go/blob/main/docker_auth.go#L53).

@HofmeisterAn
Copy link
Collaborator

Considering @mdelapenya's response, I think we can add another provider to DockerRegistryAuthenticationProvider that resolves the identity token from the Docker config incl. tests:

new CredsHelperProvider(dockerConfigJsonDocument, this.logger),
new CredsStoreProvider(dockerConfigJsonDocument, this.logger),
new Base64Provider(dockerConfigJsonDocument, this.logger),
new Base64Provider(dockerAuthConfigJsonDocument, this.logger),

Somehow, my assumption was resolving the identity token goes always through the credential helpers and I was wondering e.g. why Java does not handle it (AFAIK). Not a proper solution, but you can mimic the credential helper in the meantime too. Something we do in our tests too.

@eddumelendez
Copy link
Member

I did some tests last week which are related to this. But, azure was not in that test

GCP

cat token.json | docker login https://us-east1-docker.pkg.dev --username _json_key_base64 --password-stdin

AWS

aws ecr-public get-login-password --region <my-region> | docker login <registry> --username AWS --password-stdin

and what I can see in both cases by executing echo "https://us-east1-docker.pkg.dev" | docker-credential-desktop get is the response with SeverURL, Username and Secret returned. I expect something similar in azure

@HofmeisterAn
Copy link
Collaborator

@eddumelendez you're using a credential helper. This issue is explicit for an environment without credential helper. Which stores the identity token in the Docker config.

@lbruun
Copy link
Author

lbruun commented Mar 22, 2023

@HofmeisterAn : Regarding creating a new Provider class vs amending the existing provider (Base64Provider). This is an implementation concern. You can find this type consideration already mentioned in the PR (see "Implementation notes"). (spoiler alert: a kinda agree with you).

@PSanetra
Copy link
Contributor

PSanetra commented Oct 24, 2023

We are still experiencing this issue with version 3.5.0 and are transforming the docker auth config via the following script locally as a workaround. It would be nice if the Testcontainers library could support the identitytoken natively.

#!/bin/sh -e

test -n "${ACR_NAME}" || ( echo "ACR_NAME is not defined!" && false )
test -n "${ARM_SUBSCRIPTION_ID}" || ( echo "ARM_SUBSCRIPTION_ID is not defined!" && false )

az acr login --name "${ACR_NAME}" --subscription "${ARM_SUBSCRIPTION_ID}"

TOKEN=$(jq -r ".auths[\"${ACR_NAME}.azurecr.io\"].identitytoken" ~/.docker/config.json)

DOCKER_CONFIG=$(cat ~/.docker/config.json | jq "del(.auths[\"${ACR_NAME}.azurecr.io\"].identitytoken)")

AUTH=$(echo -n "00000000-0000-0000-0000-000000000000:${TOKEN}" | base64 --wrap=0 -)

DOCKER_CONFIG=$(echo "${DOCKER_CONFIG}" | jq ".auths[\"${ACR_NAME}.azurecr.io\"].auth = \"${AUTH}\"")

echo "${DOCKER_CONFIG}" > ~/.docker/config.json

jq . ~/.docker/config.json

@HofmeisterAn
Copy link
Collaborator

There was a pull request (#842) some time ago. I think we can merge it once we add the missing test.

@philip-reed
Copy link

Running into the same issue currently and wondering if anyone has a source for this comment from op:

The fix is trivial as Docker.DotNet library has full support for this use-case.

There's a similar thread over on Docker.DotNet about the same issue:
dotnet/Docker.DotNet#295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants