fix/Add hostname check in registry URL on login #5055
Conversation
Signed-off-by: Vlad Cherkasov <vpcherk@gmail.com>
kttyt
force-pushed
the
fix/47795--check-hostname-in-registry-url
branch
from
e78f0a3
to
47e373f
Compare
| if opts.serverAddress != "" { | ||
| u, err := url.Parse(opts.serverAddress) | ||
| if err != nil { | ||
| return errors.Errorf("Invalid server address: %s", opts.serverAddress) | ||
| } | ||
| if u.Host == "" { | ||
| return errors.Errorf("Server address must include a hostname: %s", opts.serverAddress) | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure this change is correct; the expected argument is the hostname[:port] of the registry (so only a hostname, and optionally a port). I think the fact it accepts a scheme may be an accidental side-effect of how it's handled further on, but this change likely will break "correct" cases, e.g. docker login docker.io or docker login example.com:5000.
That said; there's definitely ambiguity in some of these areas, so we need to better define these.
There was a problem hiding this comment.
net/url, in accordance with RFC3986, accepts and processes all variations of URLs, including those with or without a scheme and hostname. However, in our case, the hostname is mandatory since it is the only component used for login in the subsequent code. The issue is not the presence of a scheme; the problem lies in the fact that even '/host' is considered a valid URL not related to 'defaultRegistry'. As a result, at the stage indicated in https://github.com/docker/cli/blob/master/cli/command/registry.go#L66, the program follows the wrong path
- What I did: To address the issue related to potential credential leakage when specifying a registry URL without a hostname, I added validation checks for the registry URL's validity and the presence of a hostname when passing the registry address in the CLI.
- How I did it: As fixing the bug on the server side seemed unfeasible due to the data formation for client-side authentication, which results in passing an empty hostname string and attempting login to the default address with private credentials, I incorporated corresponding checks into the code.
- How to verify it: You can verify it by using the command
docker login http:///path, which should output the following message: "Server address must include a hostname: ''".- Description for the changelog:
- Link to the relevant code snippet in Moby: Moby Code - registry/service.go#L55
fixes: moby/moby#47795