Skip to content

configfile: normalize Docker Hub auth key aliases - #7190

Open
mclean-technology wants to merge 1 commit into
docker:masterfrom
mclean-technology:fix-dockerhub-auth-aliases
Open

configfile: normalize Docker Hub auth key aliases#7190
mclean-technology wants to merge 1 commit into
docker:masterfrom
mclean-technology:fix-dockerhub-auth-aliases

Conversation

@mclean-technology

Copy link
Copy Markdown

Summary

Normalize Docker Hub credential keys to a single canonical value so that the supported aliases all resolve to the same authentication entry:

  • docker.io
  • index.docker.io
  • https://index.docker.io/v1/

This avoids mismatches when credentials are loaded from different sources, including config files, DOCKER_AUTH_CONFIG, and credential helper resolution.

Changes

  • normalize Docker Hub auth keys when reading config entries
  • normalize Docker Hub auth keys when parsing DOCKER_AUTH_CONFIG
  • resolve credential helper overrides using the canonical Docker Hub key
  • add regression tests covering config-file, env, and helper alias behavior

Validation

  • ./scripts/with-go-mod.sh go test ./cli/config/...
  • ./scripts/with-go-mod.sh golangci-lint run ./cli/config/...

Normalize Docker Hub auth credential keys so docker.io, index.docker.io, and https://index.docker.io/v1/ all resolve to the same canonical credential entry. This keeps config-file auths, DOCKER_AUTH_CONFIG, and credential-helper lookups consistent and adds regression tests for the affected alias paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mclean-technology

Copy link
Copy Markdown
Author

Thanks for the review. I normalized the Docker Hub auth key handling so the supported aliases resolve to the same canonical credential entry. This keeps auth lookups consistent across config-file entries, DOCKER_AUTH_CONFIG, and credential-helper resolution, and adds regression tests for the affected alias cases.

Validation:

  • ./scripts/with-go-mod.sh go test ./cli/config/...
  • ./scripts/with-go-mod.sh golangci-lint run ./cli/config/...

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cli/config/configfile/file.go 85.71% 2 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

if acKey == authConfigKey {
for _, candidate := range []string{"docker.io", "index.docker.io", "https://index.docker.io/v1"} {
if candidate == acKey || candidate == "" {
continue

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is all de-centralizing the logic again; I don't think that's good. The earlier implementation you had kept it central, and for the logout ("delete") case, it picked some of the normalisation

Comment on lines -373 to 382
authConfigs[addr] = types.AuthConfig{
normalizedAddr := getAuthConfigKey(addr)
authConfigs[normalizedAddr] = types.AuthConfig{
Username: username,
Password: password,
ServerAddress: addr,
ServerAddress: normalizedAddr,
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here; I don't think we have to account for someone setting a wrong env-var; if it doesn't resolve, they should fix their env-var to use the correct casing.

Comment on lines +45 to +49
switch strings.TrimSpace(domainName) {
case "docker.io", "index.docker.io", "https://index.docker.io/v1", authConfigKey:
return authConfigKey
default:
return domainName

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is now normalising something ... different? And only to lookup the defaults, but not to normalize the other cases? (i.e., getAuthConfigKey("REGISTRY.exAmPLe.COM") returns REGISTRY.exAmPLe.COM as-is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants