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

feat: use credential helper in docker config, even if auth is empty in .docker/config.json #1079

Merged
merged 14 commits into from
Aug 9, 2023

Conversation

rokjoana
Copy link
Contributor

@rokjoana rokjoana commented Apr 18, 2023

What does this PR do?

If when retrieving the docker configuration, when there's no auth field on the user's docker config and if the user has configured credential helpers, the code will use the credential helpers to retrieve the auth information for the image repository instead of setting it to empty.

Why is it important?

Credential helpers are often used by third party docker repository providers to perform access control on the img repository.
In my case I'm using GCP but this is the case also for aws and azure. This change will extend the possibility of usage of images that are located in those repositories.

Related issues

Closes #1078

How to test this PR

This is a tricky one to test because of the way the credentials are retrieved using these helpers.
cpuguy83/dockercfg replicates the usage the docker CLI command: it calls an executable located in the PATH, which needs to output the credentials to stdout. This is tricky to emulate on go because one would need to use a script to link an executable that would mock the behaviour of a credential helper to the PATH when running the test.
The other solution would involve having a private registry setup in a private registry provider such as GCP and use the credential helper of the provider. That would also imply some dependencies on the tests.
Fortunately I had access to a private registry, so I was able to verify it by just using my fork in the project where I was using an image of the priv repository in my tests.

@rokjoana rokjoana requested a review from a team as a code owner April 18, 2023 14:35
@mdelapenya
Copy link
Collaborator

@rokjoana this LGTM at first sight, but wonder if you could cover the changes with some tests if possible (I could understand that mocking GCR would be difficult, but in any case, let's think if a test could be added).

In any case, thanks for the quick fix!

@sonarcloud
Copy link

sonarcloud bot commented Apr 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@netlify
Copy link

netlify bot commented Apr 18, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit d36d411
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/64d33fb61b595c0007d1e225
😎 Deploy Preview https://deploy-preview-1079--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rokjoana
Copy link
Contributor Author

I've described two test possibilities but both are tricky and dirty. I can try the dirtiest one, where we could define a mock credential helper executable that we build before the tests and add it to the path so that cpuguy83/dockercfg can pick it up when calling in the path using the name in the configurations.

@mdelapenya
Copy link
Collaborator

I've described two test possibilities but both are tricky and dirty. I can try the dirtiest one, where we could define a mock credential helper executable that we build before the tests and add it to the path so that cpuguy83/dockercfg can pick it up when calling in the path using the name in the configurations.

Dirty but working :) Thanks for taking the time to explain and develop the fix

@mdelapenya
Copy link
Collaborator

BTW there is a file that has not been gofmt-ed, as golangci-lint is complaining in the GH action. Could you take a look 🙏 ?

@mdelapenya mdelapenya changed the title use credential helper in docker config, even if auth is empty in .doc… use credential helper in docker config, even if auth is empty in .docker/config.json Apr 20, 2023
@mdelapenya mdelapenya changed the title use credential helper in docker config, even if auth is empty in .docker/config.json feat: use credential helper in docker config, even if auth is empty in .docker/config.json Apr 20, 2023
@mdelapenya mdelapenya self-assigned this Apr 20, 2023
@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Apr 20, 2023
@mdelapenya
Copy link
Collaborator

@rokjoana I think it would be great to resolve this bug for the next release. Are you able to continue or would you need more time for it? I can take the lead and continue on top of your work if needed.

docker_auth.go Outdated Show resolved Hide resolved
docker_auth.go Outdated Show resolved Hide resolved
@moneymikeMD
Copy link

I have a similar but different problem and was hoping this PR would fix it. Sadly, it did not.

I tested this PR locally but my issue persisted. I am on a Mac (Ventura 13.4) and was getting an error:

Failed to get image auth for docker.io. Setting empty credentials for the image: docker.io/testcontainers/ryuk:0.5.1. Error is:credentials not found in native keychain

I dug into it a little more. My current theory is in the docker_auth.go file, the DockerImageAuth function is looking for the registry for the image. The default registry on Mac is https://index.docker.io/v1/ while the ryuk image starts with docker.io . The means the check for cfgs[registry] is never finding a matching AuthConfig and thus returns dockercfg.ErrCredentialsNotFound.
So, possible solutions are to make the registry matching looser, or to make the fallback condition of testcontainersdocker.ExtractRegistry easier to hit.

@rokjoana
Copy link
Contributor Author

I do apologize for the delay. I had some personal stuff to deal with. I'll have a look at your comments and finish the PR soon.

@mdelapenya
Copy link
Collaborator

Failed to get image auth for docker.io. Setting empty credentials for the image: docker.io/testcontainers/ryuk:0.5.1. Error is:credentials not found in native keychain

@moneymikeMD this log message is printed out in the case there is an error getting the DockerAuth, BUT the pull will take place:

Please see

testcontainers-go/docker.go

Lines 962 to 977 in c0e1c32

registry, imageAuth, err := DockerImageAuth(ctx, req.Image)
if err != nil {
p.Logger.Printf("Failed to get image auth for %s. Setting empty credentials for the image: %s. Error is:%s", registry, req.Image, err)
} else {
// see https://github.com/docker/docs/blob/e8e1204f914767128814dca0ea008644709c117f/engine/api/sdk/examples.md?plain=1#L649-L657
encodedJSON, err := json.Marshal(imageAuth)
if err != nil {
p.Logger.Printf("Failed to marshal image auth. Setting empty credentials for the image: %s. Error is:%s", req.Image, err)
} else {
pullOpt.RegistryAuth = base64.URLEncoding.EncodeToString(encodedJSON)
}
}
if err := p.attemptToPullImage(ctx, tag, pullOpt); err != nil {
return nil, err
}

Is Ryuk effectively pulled into your system?

@mdelapenya
Copy link
Collaborator

I do apologize for the delay. I had some personal stuff to deal with. I'll have a look at your comments and finish the PR soon.

I totally get it, no worries at all 😊

@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@rokjoana
Copy link
Contributor Author

@mdelapenya I had a look and the only way for us to specifically test this is to, during the tests, register an executable in the PATH that would output the credentials of a mocked private registry repository (just like the docker credential helpers do).
Not only you can't do this on go, I think its something that's prone to be unsafe in the long run (the mocked binary can be easily tampered). I checked also the testcontainer libraries for Java and I couldn't find a direct test for this case (I'm also not sure if it was corrected in them). I don't know what we should do in this case but I leave up to you the decision of merging the fix.

@moneymikeMD
Copy link

moneymikeMD commented Jun 23, 2023

Is Ryuk effectively pulled into your system?

No, it doesn't pull the image because its not using credentials. Then, I get the original Please run 'docker login': failed to create container.

@rokjoana
Copy link
Contributor Author

rokjoana commented Jun 25, 2023

@moneymikeMD I can confirm that the log occurs but on my machine the images are pulled. As far as the issue is about, I think it could be something unrelated. Could you share the format of your .docker/config.json?

@mdelapenya
Copy link
Collaborator

@rokjoana after #1394, the types.AuthConfig type has been moved to registry.AuthConfig. Would you mind updating the PR with that change? 🙏

I've come back from parental leave yesterday, so I can now focus in reviewing this PR. Thanks in advance

@sonarcloud
Copy link

sonarcloud bot commented Aug 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your patience during the review, it took a while, having summer and parental leave in between, but here we go 🚀

@mdelapenya mdelapenya merged commit de74538 into testcontainers:main Aug 9, 2023
106 checks passed
@mdelapenya
Copy link
Collaborator

@moneymikeMD I'd appreciate if you can double check what @rokjoana asked here, in order to try to debug the error you are experiencing:

Could you share the format of your .docker/config.json?

It will need opening a different GH issue for tracking, I can do the linking if needed 🙏

mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Aug 9, 2023
* main:
  Simplify dependabot updates sorting (testcontainers#1460)
  feat: use credential helper in docker config, even if auth is empty in .docker/config.json (testcontainers#1079)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 (testcontainers#1457)
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Aug 10, 2023
* main: (29 commits)
  Add support for MongoDB testing module (testcontainers#1447)
  Support groups in dependabot updates (testcontainers#1459)
  chore: run modulegen tests on Windows (testcontainers#1478)
  Add default labels when Ryuk is disabled (testcontainers#1339)
  feat: add clickhouse module (testcontainers#1372)
  chore: increase timeout for go test and GH action steps (testcontainers#1475)
  chore: triple max timeout for the workflow run, which takes +10m (testcontainers#1474)
  chore(deps): bump github.com/aws dependencies in /modules/localstack (testcontainers#1472)
  chore(deps): bump Google emulators dependencies in /examples (testcontainers#1471)
  all: fix goroutine leaks (testcontainers#1358)
  chore(deps): bump github.com/neo4j/neo4j-go-driver/v5 in /modules/neo4j (testcontainers#1427)
  chore(deps): bump github.com/tidwall/gjson from 1.14.4 to 1.15.0 in /modules/vault (testcontainers#1428)
  chore: add a GH action for release drafter (testcontainers#1470)
  chore(deps): bump mkdocs-material from 3.2.0 to 8.2.7 (testcontainers#1468)
  Add global testcontainers header to docs (testcontainers#1308)
  Simplify dependabot updates sorting (testcontainers#1460)
  feat: use credential helper in docker config, even if auth is empty in .docker/config.json (testcontainers#1079)
  chore(deps): bump github.com/aws/aws-sdk-go-v2/service/s3 (testcontainers#1457)
  Revert "chore: run Windows tests on a Linux container (testcontainers#1456)"
  chore: run Windows tests on a Linux container (testcontainers#1456)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Failed authentication from private repository with credHelpers
4 participants