Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[23.0 backport] Fix setting ServerAddress property in NativeStore #4823

Merged

Conversation

thaJeztah
Copy link
Member

This will return the ServerAddress property when using the NativeStore. This happens when you use docker credential helpers, not the credential store.

The reason this fix is needed is because it needs to be propagated properly down towards moby/moby project in the following logic:

func authorizationCredsFromAuthConfig(authConfig registrytypes.AuthConfig) docker.AuthorizerOpt {
	cfgHost := registry.ConvertToHostname(authConfig.ServerAddress)
	if cfgHost == "" || cfgHost == registry.IndexHostname {
		cfgHost = registry.DefaultRegistryHost
	}

	return docker.WithAuthCreds(func(host string) (string, string, error) {
		if cfgHost != host {
			logrus.WithFields(logrus.Fields{
				"host":    host,
				"cfgHost": cfgHost,
			}).Warn("Host doesn't match")
			return "", "", nil
		}
		if authConfig.IdentityToken != "" {
			return "", authConfig.IdentityToken, nil
		}
		return authConfig.Username, authConfig.Password, nil
	})
}

This logic resides in the following file :
daemon/containerd/resolver.go .

In the case when using the containerd storage feature when setting the cfgHost variable from the authConfig.ServerAddress it will always be empty. Since it will never be returned from the NativeStore currently. Therefore Docker Hub images will work fine, but anything else will fail since the cfgHost will always be the registry.DefaultRegistryHost.

(cherry picked from commit b24e7f8)

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

This will return the ServerAddress property when using the NativeStore.
This happens when you use docker credential helpers, not the credential
store.

The reason this fix is needed is because it needs to be propagated
properly down towards `moby/moby` project in the following logic:

```golang
func authorizationCredsFromAuthConfig(authConfig registrytypes.AuthConfig) docker.AuthorizerOpt {
	cfgHost := registry.ConvertToHostname(authConfig.ServerAddress)
	if cfgHost == "" || cfgHost == registry.IndexHostname {
		cfgHost = registry.DefaultRegistryHost
	}

	return docker.WithAuthCreds(func(host string) (string, string, error) {
		if cfgHost != host {
			logrus.WithFields(logrus.Fields{
				"host":    host,
				"cfgHost": cfgHost,
			}).Warn("Host doesn't match")
			return "", "", nil
		}
		if authConfig.IdentityToken != "" {
			return "", authConfig.IdentityToken, nil
		}
		return authConfig.Username, authConfig.Password, nil
	})
}
```
This logic resides in the following file :
`daemon/containerd/resolver.go` .

In the case when using the containerd storage feature when setting the
`cfgHost` variable from the `authConfig.ServerAddress` it will always be
empty. Since it will never be returned from the NativeStore currently.
Therefore Docker Hub images will work fine, but anything else will fail
since the `cfgHost` will always be the `registry.DefaultRegistryHost`.

Signed-off-by: Eric Bode <[email protected]>
(cherry picked from commit b24e7f8)
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #4823 (ddaded1) into 23.0 (3edca7e) will increase coverage by 0.36%.
Report is 1 commits behind head on 23.0.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             23.0    #4823      +/-   ##
==========================================
+ Coverage   58.62%   58.99%   +0.36%     
==========================================
  Files         286      286              
  Lines       24817    24825       +8     
==========================================
+ Hits        14549    14645      +96     
+ Misses       9388     9298      -90     
- Partials      880      882       +2     

@thaJeztah thaJeztah merged commit 4d6486d into docker:23.0 Jan 24, 2024
74 checks passed
@thaJeztah thaJeztah deleted the 23.0_backport_4653-fix-credential-helper branch January 24, 2024 15:52
@thaJeztah thaJeztah modified the milestones: 23.0.9, 23.0.10 Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants