registry: normalize server address to lowercase for credential lookup - #7184
registry: normalize server address to lowercase for credential lookup#7184HarnageaGabriel wants to merge 1 commit into
Conversation
| if opts.serverAddress != "" && opts.serverAddress != registry.DefaultNamespace { | ||
| serverAddress = opts.serverAddress | ||
| serverAddress = strings.ToLower(opts.serverAddress) |
There was a problem hiding this comment.
Perhaps a better place for lookup would be where we do the other normalisation; see
cli/cli/config/configfile/file.go
Lines 44 to 49 in 1289207
Something like;
func getAuthConfigKey(domainName string) string {
domainName = strings.ToLower(domainName)
if domainName == "docker.io" || domainName == "index.docker.io" {
return authConfigKey
}
return domainName
}
|
OH! When updating; could you remove the Also can you update your sign-off to use your real name, not your GitHub handle? |
6a93873 to
c5ee2ff
Compare
| if serverAddress != "" { | ||
| serverAddress = strings.ToLower(serverAddress) | ||
| } |
There was a problem hiding this comment.
Was this still needed? Or is this because we hit the Store implementation directly here?
| hostnameAddress = credentials.ConvertToHostname(serverAddress) | ||
| // the tries below are kept for backward compatibility where a user could have | ||
| // saved the registry in one of the following format. | ||
| regsToLogout = append(regsToLogout, hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress) |
There was a problem hiding this comment.
If that's the case, then perhaps we should append lowercase as additional variant.
If we only try lowercase, than a previously successful docker login REGISTRY.EXAMPLE.COM would not be removed (only registry.example.com).
regsToLogout = append(regsToLogout,
hostnameAddress,
"http://"+hostnameAddress,
"https://"+hostnameAddress,
"http://"+strings.ToLower(hostnameAddress),
"https://"+strings.ToLower(hostnameAddress),
)
c5ee2ff to
de7eba1
Compare
| "registry-1.docker.io": {"auth": "dXNlcm5hbWU6cGFzc3dvcmQ="}, | ||
| "registry.hub.docker.com": {"auth": "dXNlcm5hbWU6cGFzc3dvcmQ="} | ||
| "registry.hub.docker.com": {"auth": "dXNlcm5hbWU6cGFzc3dvcmQ="}, | ||
| "test.registry.org": {"auth": "dXNlcm5hbWU6cGFzc3dvcmQ="} |
There was a problem hiding this comment.
registry.org is a registered domain; https://who.is/whois/registry.org
While this is a unit-test, and shouldn't be making network connections; we should avoid using actual domains (the docker.com ones are a slight exception here, but even those can probably be replaced at some point);
could you use one of the designated domains for testing and documentation? See https://datatracker.ietf.org/doc/html/rfc2606
e.g. registry.example.com or registry.test
| expectedAuthCfg: registry.AuthConfig{Username: "username", Password: "password", ServerAddress: "registry.hub.docker.com"}, | ||
| }, | ||
| { | ||
| prefix: "Test.Registry.org", |
de7eba1 to
dc7e5b1
Compare
Login credentials fail when the registry hostname's case at `docker login` differs from the case used in an image reference at push/pull time (e.g. `docker login Registry.Example.com` then `docker push Registry.Example.com/img`). distribution/reference does not lowercase the domain component of a reference, so credentials ended up stored and looked up under different keys depending on casing. Normalize the lookup key to lowercase in the config file's canonical credential-lookup key (getAuthConfigKey), covering all callers of GetAuthConfig/GetCredentialsStore. Also lowercase the server address at the credential-store write path in `docker login`, since the file store's Store/Erase use an exact-match key and are not routed through the lookup normalization. `docker logout` hits the same Store/Erase path directly, so it needs the same treatment. Rather than replacing the looked-up address with its lowercase form (which would stop `docker logout` from finding credentials stored under their original case, e.g. by a version of the CLI predating this normalization), try the lowercase variants in addition to the original-case ones. Signed-off-by: HarnageaGabriel <gabriel.harnagea06@gmail.com>
dc7e5b1 to
2976407
Compare
2 participants
Closes #2753
- What I did
Registry login credentials were stored/looked-up under a case-sensitive
key. If the hostname's case used at
docker logindiffers from thecase used in an image reference at push/pull time (e.g.
docker login Test.Registry.orgthendocker push Test.Registry.org/img), authentication fails even thoughcredentials for that registry exist.
Root cause:
distribution/referencedoes not lowercase the domaincomponent of a reference (only the repository path must be lowercase),
so the same registry hostname can produce different credential-store
keys depending on how it was typed.
This revisits the approach from #2782 (2020, closed unmerged by the
stale bot, but previously reviewed and approved) and adapts it to the
current codebase, which has changed substantially since then (device-
flow login, new credential-store abstraction, content trust moved to
the separate
cmd/docker-trustbinary).- How I did it
Normalize the server address to lowercase at:
docker login/docker logout, before it's used as acredential-store key
RetrieveAuthTokenFromImage, which resolves auth for an imagereference and is shared by push, pull, service create/update, stack
deploy, and plugin install
getAuthConfigKeyincli/config/configfile, the canonicalcredential-lookup key used by
GetAuthConfig, as defense-in-depthfor any other caller
- How to verify it
Added/extended unit tests covering mixed-case server addresses for
login, logout, image-reference auth resolution, and the config-file
lookup key.
go build ./...and the affected package tests pass.- Description for the changelog
Registry login/logout and image push/pull now resolve stored
credentials case-insensitively by registry hostname.