Skip to content

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

Merged
thaJeztah merged 1 commit into
docker:23.0from
thaJeztah:23.0_backport_4653-fix-credential-helper
Jan 24, 2024
Merged

[23.0 backport] Fix setting ServerAddress property in NativeStore#4823
thaJeztah merged 1 commit into
docker:23.0from
thaJeztah:23.0_backport_4653-fix-credential-helper

Conversation

@thaJeztah

Copy link
Copy Markdown
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 <eric.bode@foundries.io>
(cherry picked from commit b24e7f8)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@codecov-commenter

Copy link
Copy Markdown

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
@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