fix: docker auth for docker.io images #3482
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbit
WalkthroughDocker image authentication logic now remaps images with the docker.io host to the configured default registry before performing credential lookup. A new test verifies that DockerImageAuth returns credentials matching the default registry for a docker.io image. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthLayer as Auth Resolver
participant CredStore as Credential Store
rect rgb(235,248,255)
Client->>AuthLayer: Request auth for image "docker.io/my/image:latest"
note right of AuthLayer: New step: remap docker.io -> defaultRegistry()
AuthLayer->>AuthLayer: if host == "docker.io" then host = defaultRegistry()
end
AuthLayer->>CredStore: Lookup credentials for remapped host
CredStore-->>AuthLayer: Credentials (username, password, auth)
AuthLayer-->>Client: Return registry, username, password, encoded Auth
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LaurentGoderre
changed the title
LaurentGoderre
force-pushed
the
fix-docker-dot-io
branch
from
bf4458c to
df49f93
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker_auth.go(1 hunks)docker_auth_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker_auth_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: lint (modules/dynamodb) / lint: modules/dynamodb
- GitHub Check: lint (modulegen) / lint: modulegen
- GitHub Check: lint (modules/dind) / lint: modules/dind
- GitHub Check: lint (modules/registry) / lint: modules/registry
- GitHub Check: lint (modules/dockermodelrunner) / lint: modules/dockermodelrunner
- GitHub Check: lint (modules/postgres) / lint: modules/postgres
- GitHub Check: lint (modules/qdrant) / lint: modules/qdrant
- GitHub Check: lint (modules/pinecone) / lint: modules/pinecone
- GitHub Check: lint (modules/redpanda) / lint: modules/redpanda
- GitHub Check: lint (modules/solace) / lint: modules/solace
- GitHub Check: lint (modules/memcached) / lint: modules/memcached
- GitHub Check: lint (modules/openfga) / lint: modules/openfga
- GitHub Check: lint (modules/pulsar) / lint: modules/pulsar
- GitHub Check: lint (modules/artemis) / lint: modules/artemis
- GitHub Check: lint (modules/influxdb) / lint: modules/influxdb
- GitHub Check: lint (modules/rabbitmq) / lint: modules/rabbitmq
- GitHub Check: lint (modules/dolt) / lint: modules/dolt
- GitHub Check: lint (modules/mariadb) / lint: modules/mariadb
- GitHub Check: lint (modules/consul) / lint: modules/consul
- GitHub Check: Analyze (go)
| if reg == "docker.io" { | ||
| reg = defaultRegistry | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix correctly remaps docker.io to canonical registry address.
The remapping ensures credential lookup uses the canonical registry address (https://index.docker.io/v1) where Docker stores authentication data, addressing the stated issue.
Please verify the following edge cases:
- Case sensitivity: Should the comparison be case-insensitive? Docker registries treat hostnames as case-insensitive.
- Other Docker registry aliases: Does
registry-1.docker.ioor other aliases require similar remapping?
Consider extracting the hardcoded string to a constant for better maintainability:
+const dockerIORegistry = "docker.io"
+
func dockerImageAuth(ctx context.Context, image string, configs map[string]registry.AuthConfig) (string, registry.AuthConfig, error) {
defaultRegistry := defaultRegistryFn(ctx)
reg := core.ExtractRegistry(image, defaultRegistry)
- if reg == "docker.io" {
+ if reg == dockerIORegistry {
reg = defaultRegistry
}🏁 Script executed:
#!/bin/bash
# Verify if core.ExtractRegistry normalizes case and check for other registry aliases
echo "=== Searching for registry extraction logic ==="
rg -n -C5 'func ExtractRegistry'
echo -e "\n=== Searching for other docker.io references ==="
rg -n -C3 'docker\.io|registry-1\.docker\.io' --type go -g '!*_test.go'
echo -e "\n=== Checking for registry-related constants ==="
rg -n -C3 'const.*[Rr]egistry|IndexDockerIO' --type goLength of output: 7590
Now I need to check the actual implementation of ExtractRegistry and see if there are test cases demonstrating the expected behavior.
🏁 Script executed:
#!/bin/bash
# Check full ExtractRegistry implementation
echo "=== ExtractRegistry full implementation ==="
sed -n '95,120p' internal/core/images.go
echo -e "\n=== Check how dockerImageAuth is tested with different registry cases ==="
rg -n -A 10 -B 2 'dockerImageAuth' docker_auth_test.go | head -60
echo -e "\n=== Search for test cases with case variations ==="
rg -n 'docker\.io|Docker\.IO' docker_auth_test.go internal/core/images_test.goLength of output: 1274
Fix addresses docker.io remapping but has exploitable gaps that need addressing.
The remapping correctly uses the canonical registry for credential lookup. However, the current implementation has two issues:
-
Case sensitivity: The comparison
reg == "docker.io"is case-sensitive, so images using mixed case (e.g.,Docker.io/myimage) won't be remapped. SinceExtractRegistrydoesn't normalize case, this could cause credential lookups to fail for such images. -
Incomplete alias coverage: The code only handles
docker.io, butregistry.hub.docker.com(visible inoptions.go:233) is another Docker Hub alias that should receive the same treatment.
Recommended fixes:
// Make comparison case-insensitive and handle both aliases
if strings.EqualFold(reg, "docker.io") || strings.EqualFold(reg, "registry.hub.docker.com") {
reg = defaultRegistry
}Or, consider normalizing registry names consistently in ExtractRegistry by using strings.ToLower().
🤖 Prompt for AI Agents
In docker_auth.go around lines 45 to 47, the remapping only checks for the
literal "docker.io" which is case-sensitive and misses the alias
registry.hub.docker.com; update the check to treat registry names
case-insensitively and include both aliases (e.g., use strings.EqualFold for
"docker.io" and "registry.hub.docker.com") or normalize registry names with
strings.ToLower earlier (such as in ExtractRegistry) so both mixed-case names
and the hub alias are remapped to defaultRegistry.
1 participant
What does this PR do?
When doing a Docker pull on an image with the default registry
docker.io, the auth helper doesn't match the image to the authentication data stored by the engine underhttps://index.docker.io/v1causing the error:credentials not found in native keychainWhy is it important?
This is important to support the default registry prefix.
How to test this PR
Unit test added