Skip to content

Commit

Permalink
Merge pull request #3766 from apostasie/fix-3748
Browse files Browse the repository at this point in the history
When the port is 443, also save credentials without port
  • Loading branch information
AkihiroSuda authored Jan 7, 2025
2 parents 1b98a5e + 7a69f5d commit 9ffc5c1
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 8 deletions.
11 changes: 11 additions & 0 deletions pkg/cmd/login/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,17 @@ func Login(ctx context.Context, options types.LoginCommandOptions, stdout io.Wri
return fmt.Errorf("error saving credentials: %w", err)
}

// When the port is the https default (443), other clients cannot be expected to necessarily lookup the variants with port
// so save it both with and without port.
// This is the case for at least buildctl: https://github.com/containerd/nerdctl/issues/3748
if registryURL.Port() == dockerconfigresolver.StandardHTTPSPort {
registryURL.Host = registryURL.Hostname()
err = credStore.Store(registryURL, credentials)
if err != nil {
return fmt.Errorf("error saving credentials: %w", err)
}
}

_, err = fmt.Fprintln(stdout, "Login Succeeded")

return err
Expand Down
7 changes: 4 additions & 3 deletions pkg/imgutil/dockerconfigresolver/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ import "errors"
type scheme string

const (
standardHTTPSPort = "443"
schemeHTTP scheme = "http"
schemeHTTPS scheme = "https"
StandardHTTPSPort = "443"

schemeHTTPS scheme = "https"
schemeHTTP scheme = "http"
// schemeNerdctlExperimental is currently provisional, to unlock namespace based host authentication
// This may change or break without notice, and you should have no expectations that credentials saved like that
// will be supported in the future
Expand Down
2 changes: 1 addition & 1 deletion pkg/imgutil/dockerconfigresolver/hostsstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func hostDirsFromRoot(registryURL *RegistryURL, dirs []string) (string, error) {
return found, err
}
// If not found, and the port is standard, try again without the port
if registryURL.Port() == standardHTTPSPort {
if registryURL.Port() == StandardHTTPSPort {
found, err = config.HostDirFromRoot(hostsDir)(registryURL.Hostname())
if (err != nil && !errors.Is(err, errdefs.ErrNotFound)) || (found != "") {
return found, err
Expand Down
8 changes: 4 additions & 4 deletions pkg/imgutil/dockerconfigresolver/registryurl.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func Parse(address string) (*RegistryURL, error) {
}
// If it has no port, add the standard port explicitly
if u.Port() == "" {
u.Host = u.Hostname() + ":" + standardHTTPSPort
u.Host = u.Hostname() + ":" + StandardHTTPSPort
}
reg := &RegistryURL{URL: *u}
queryParams := u.Query()
Expand All @@ -74,7 +74,7 @@ type RegistryURL struct {
// CanonicalIdentifier returns the identifier expected to be used to save credentials to docker auth config
func (rn *RegistryURL) CanonicalIdentifier() string {
// If it is the docker index over https, port 443, on the /v1/ path, we use the docker fully qualified identifier
if rn.Scheme == string(schemeHTTPS) && rn.Hostname() == "index.docker.io" && rn.Path == "/v1/" && rn.Port() == standardHTTPSPort ||
if rn.Scheme == string(schemeHTTPS) && rn.Hostname() == "index.docker.io" && rn.Path == "/v1/" && rn.Port() == StandardHTTPSPort ||
rn.URL.String() == dockerIndexServer {
return dockerIndexServer
}
Expand Down Expand Up @@ -102,7 +102,7 @@ func (rn *RegistryURL) AllIdentifiers() []string {

// Docker behavior: if the domain was index.docker.io over 443, we are allowed to additionally read the canonical
// docker credentials
if rn.Port() == standardHTTPSPort {
if rn.Port() == StandardHTTPSPort {
if rn.Hostname() == "index.docker.io" || rn.Hostname() == "registry-1.docker.io" {
fullList = append(fullList, dockerIndexServer)
}
Expand All @@ -116,7 +116,7 @@ func (rn *RegistryURL) AllIdentifiers() []string {

// Note that docker does not try to be smart wrt explicit port vs. implied port
// If standard port, allow retrieving credentials from the variant without a port as well
if rn.Port() == standardHTTPSPort {
if rn.Port() == StandardHTTPSPort {
fullList = append(
fullList,
rn.Hostname(),
Expand Down

0 comments on commit 9ffc5c1

Please sign in to comment.