diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index 3679e51eddd5..e7a7d834cdf5 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -111,7 +111,9 @@ func runLogin(ctx context.Context, dockerCLI command.Cli, opts loginOptions) err return err } - maybePrintEnvAuthWarning(dockerCLI) + if dockerCLI.ConfigFile().PreferDockerAuthConfig() { + printEnvAuthWarning(dockerCLI) + } var ( serverAddress string diff --git a/cli/command/registry/logout.go b/cli/command/registry/logout.go index 2af2cdad3f3f..caf0691395fa 100644 --- a/cli/command/registry/logout.go +++ b/cli/command/registry/logout.go @@ -36,7 +36,9 @@ func NewLogoutCommand(dockerCli command.Cli) *cobra.Command { } func runLogout(ctx context.Context, dockerCLI command.Cli, serverAddress string) error { - maybePrintEnvAuthWarning(dockerCLI) + if dockerCLI.ConfigFile().PreferDockerAuthConfig() { + printEnvAuthWarning(dockerCLI) + } var isDefaultRegistry bool diff --git a/cli/command/registry/warning.go b/cli/command/registry/warning.go index 22a8c8655be6..40a79f874f8b 100644 --- a/cli/command/registry/warning.go +++ b/cli/command/registry/warning.go @@ -1,18 +1,14 @@ package registry import ( - "os" - "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/internal/tui" ) -// maybePrintEnvAuthWarning if the `DOCKER_AUTH_CONFIG` environment variable is +// printEnvAuthWarning if the `DOCKER_AUTH_CONFIG` environment variable is // set this function will output a warning to stdErr -func maybePrintEnvAuthWarning(out command.Streams) { - if os.Getenv(configfile.DockerEnvConfigKey) != "" { - tui.NewOutput(out.Err()). - PrintWarning("%[1]s is set and takes precedence.\nUnset %[1]s to restore the CLI auth behaviour.\n", configfile.DockerEnvConfigKey) - } +func printEnvAuthWarning(out command.Streams) { + tui.NewOutput(out.Err()). + PrintWarning("%[1]s is set and takes precedence.\nUnset %[1]s to restore the CLI auth behaviour.\n", configfile.DockerEnvConfigKey) } diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index 530c5228561f..07626699ec53 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -287,6 +287,39 @@ func decodeAuth(authStr string) (string, string, error) { return userName, strings.Trim(password, "\x00"), nil } +// PreferDockerAuthConfig returns true if the memory store containing auth +// config read from DOCKER_AUTH_CONFIG should be preferred over the config file. +func (configFile *ConfigFile) PreferDockerAuthConfig() bool { + order := configFile.Features["auth_config_order"] + if v := os.Getenv("DOCKER_AUTH_ORDER"); v != "" { + order = v + } + + order = strings.TrimSpace(order) + if order == "" { + // Temporary fix for clashing with GitLab CI + // Don't enable by default even if DOCKER_AUTH_CONFIG is set + // https://github.com/docker/cli/issues/6156 + if os.Getenv("GITLAB_CI") == "true" { + return false + } + return true + } + + parts := strings.Split(order, ",") + if len(parts) == 2 { + if parts[0] == "env" && parts[1] == "config" { + return true + } + if parts[0] == "config" && parts[1] == "env" { + return false + } + } + + _, _ = fmt.Fprintln(os.Stderr, "Malformed DOCKER_AUTH_ORDER") + return true +} + // GetCredentialsStore returns a new credentials store from the settings in the // configuration file func (configFile *ConfigFile) GetCredentialsStore(registryHostname string) credentials.Store { @@ -307,12 +340,17 @@ func (configFile *ConfigFile) GetCredentialsStore(registryHostname string) crede return store } - // use DOCKER_AUTH_CONFIG if set - // it uses the native or file store as a fallback to fetch and store credentials - envStore, err := memorystore.New( + opts := []memorystore.Options{ memorystore.WithAuthConfig(authConfig), memorystore.WithFallbackStore(store), - ) + } + if !configFile.PreferDockerAuthConfig() { + opts = append(opts, memorystore.WithPreferFallback()) + } + + // use DOCKER_AUTH_CONFIG if set + // it uses the native or file store as a fallback to fetch and store credentials + envStore, err := memorystore.New(opts...) if err != nil { _, _ = fmt.Fprintln(os.Stderr, "Failed to create credential store from DOCKER_AUTH_CONFIG: ", err) return store diff --git a/cli/config/configfile/file_test.go b/cli/config/configfile/file_test.go index 92df02c74352..3132cd578ee4 100644 --- a/cli/config/configfile/file_test.go +++ b/cli/config/configfile/file_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "errors" + "maps" "os" "testing" @@ -584,6 +585,56 @@ func TestGetAllCredentialsFromEnvironment(t *testing.T) { }) } +func TestGitlabCIWithDockerAuthConfig(t *testing.T) { + t.Setenv("GITLAB_CI", "true") + + testAuthConfig := map[string]types.AuthConfig{ + "env.example.test": { + Username: "user", + Password: "pass", + }, + } + configFile := New("filename") + configFile.AuthConfigs = maps.Clone(testAuthConfig) + + checkConfigFirst := func(t *testing.T) { + t.Helper() + authConfigs, err := configFile.GetAllCredentials() + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(authConfigs, testAuthConfig)) + } + + checkEnvFirst := func(t *testing.T) { + t.Helper() + authConfigs, err := configFile.GetAllCredentials() + assert.NilError(t, err) + assert.Check(t, is.DeepEqual(authConfigs, map[string]types.AuthConfig{ + "env.example.test": { + Username: "env_user", + Password: "env_pass", + ServerAddress: "env.example.test", + }, + })) + } + + t.Setenv("DOCKER_AUTH_CONFIG", envTestAuthConfig) + t.Run("config is used by default", func(t *testing.T) { + checkConfigFirst(t) + }) + t.Run("config is used when configured explicitly", func(t *testing.T) { + configFile.Features = map[string]string{ + "auth_config_order": "config,env", + } + checkConfigFirst(t) + }) + t.Run("env is used if opt-in", func(t *testing.T) { + configFile.Features = map[string]string{ + "auth_config_order": "env,config", + } + checkEnvFirst(t) + }) +} + func TestParseEnvConfig(t *testing.T) { t.Run("should error on unexpected fields", func(t *testing.T) { _, err := parseEnvConfig(envTestUserPassConfig) diff --git a/cli/config/memorystore/store.go b/cli/config/memorystore/store.go index 199083464ed8..bd9703354f59 100644 --- a/cli/config/memorystore/store.go +++ b/cli/config/memorystore/store.go @@ -23,6 +23,7 @@ type Config struct { lock sync.RWMutex memoryCredentials map[string]types.AuthConfig fallbackStore credentials.Store + preferFallback bool } func (e *Config) Erase(serverAddress string) error { @@ -43,6 +44,14 @@ func (e *Config) Erase(serverAddress string) error { func (e *Config) Get(serverAddress string) (types.AuthConfig, error) { e.lock.RLock() defer e.lock.RUnlock() + + if e.preferFallback && e.fallbackStore != nil { + authConfig, err := e.fallbackStore.Get(serverAddress) + if err == nil { + return authConfig, nil + } + } + authConfig, ok := e.memoryCredentials[serverAddress] if !ok { if e.fallbackStore != nil { @@ -58,16 +67,22 @@ func (e *Config) GetAll() (map[string]types.AuthConfig, error) { defer e.lock.RUnlock() creds := make(map[string]types.AuthConfig) + if e.preferFallback { + maps.Copy(creds, e.memoryCredentials) + } + if e.fallbackStore != nil { fileCredentials, err := e.fallbackStore.GetAll() if err != nil { _, _ = fmt.Fprintln(os.Stderr, "memorystore: ", err) } else { - creds = fileCredentials + maps.Copy(creds, fileCredentials) } } - maps.Copy(creds, e.memoryCredentials) + if !e.preferFallback { + maps.Copy(creds, e.memoryCredentials) + } return creds, nil } @@ -102,6 +117,26 @@ func WithFallbackStore(store credentials.Store) Options { } } +// WithPreferFallback configures the store to prefer reading credentials from +// the fallback store. +// +// Write operations will be executed on both the memory store and the +// fallback store. +// +// For read operations, the fallback store is checked first. If the credential +// is not found there, the memory store is checked next. +// +// When retrieving all credentials, results from both the memory store and the +// fallback store are combined into a single map. +// +// Credentials in the fallback store will override those in the memory store. +func WithPreferFallback() Options { + return func(s *Config) error { + s.preferFallback = true + return nil + } +} + // WithAuthConfig allows to set the initial credentials in the memory store. func WithAuthConfig(config map[string]types.AuthConfig) Options { return func(s *Config) error { diff --git a/cli/config/memorystore/store_test.go b/cli/config/memorystore/store_test.go index a92a170a3b6b..893526e75e4d 100644 --- a/cli/config/memorystore/store_test.go +++ b/cli/config/memorystore/store_test.go @@ -73,9 +73,9 @@ func TestMemoryStore(t *testing.T) { err = memoryStore.Erase("https://a-new-credential.example.test") assert.NilError(t, err) _, err = memoryStore.Get("https://a-new-credential.example.test") - assert.Check(t, is.ErrorIs(err, errValueNotFound)) + assert.Check(t, IsErrValueNotFound(err)) _, err = fallbackStore.Get("https://a-new-credential.example.test") - assert.Check(t, is.ErrorIs(err, errValueNotFound)) + assert.Check(t, IsErrValueNotFound(err)) }) } @@ -126,6 +126,99 @@ func TestMemoryStoreWithoutFallback(t *testing.T) { err = memoryStore.Erase("https://a-new-credential.example.test") assert.NilError(t, err) _, err = memoryStore.Get("https://a-new-credential.example.test") + assert.Check(t, IsErrValueNotFound(err)) + }) +} + +func TestMemoryStoreWithPreferFallback(t *testing.T) { + config := map[string]types.AuthConfig{ + "https://example.test": { + Username: "memory-user", + ServerAddress: "https://example.test", + Auth: "memory-token", + }, + "https://only-in-memory.example.test": { + Username: "only-memory-user", + ServerAddress: "https://only-in-memory.example.test", + Auth: "only-memory-token", + }, + } + + fallbackConfig := map[string]types.AuthConfig{ + "https://example.test": { + Username: "fallback-user", + ServerAddress: "https://example.test", + Auth: "fallback-token", + }, + "https://only-in-file.example.test": { + Username: "something-something", + ServerAddress: "https://only-in-file.example.test", + Auth: "super_secret_token", + }, + } + + fallbackStore, err := New(WithAuthConfig(fallbackConfig)) + assert.NilError(t, err) + + memoryStore, err := New(WithAuthConfig(config), WithFallbackStore(fallbackStore), WithPreferFallback()) + assert.NilError(t, err) + + t.Run("get credentials prefers fallback store", func(t *testing.T) { + // should get from fallback + c, err := memoryStore.Get("https://example.test") + assert.NilError(t, err) + assert.Equal(t, c.Username, "fallback-user") + + // should get from fallback (only exists there) + c, err = memoryStore.Get("https://only-in-file.example.test") + assert.NilError(t, err) + assert.Equal(t, c.Username, "something-something") + + // should get from memory if not in fallback + c, err = memoryStore.Get("https://only-in-memory.example.test") + assert.NilError(t, err) + assert.Equal(t, c.Username, "only-memory-user") + }) + + t.Run("GetAll prefers fallback store", func(t *testing.T) { + all, err := memoryStore.GetAll() + assert.NilError(t, err) + assert.Equal(t, len(all), 3) + // value from fallback store should be present + assert.Equal(t, all["https://example.test"].Username, "fallback-user") + // value only in fallback should be present + assert.Equal(t, all["https://only-in-file.example.test"].Username, "something-something") + // value only in memory should be present + assert.Equal(t, all["https://only-in-memory.example.test"].Username, "only-memory-user") + }) + + t.Run("storing credentials writes to both stores", func(t *testing.T) { + newCred := types.AuthConfig{ + Username: "new-user", + ServerAddress: "https://new.example.test", + Auth: "new-token", + } + err := memoryStore.Store(newCred) + assert.NilError(t, err) + + // Check both stores to ensure the credential was written + c, err := memoryStore.Get("https://new.example.test") + assert.NilError(t, err) + assert.Equal(t, c, newCred) + + c, err = fallbackStore.Get("https://new.example.test") + assert.NilError(t, err) + assert.Equal(t, c, newCred) + }) + + t.Run("Erase removes from both stores", func(t *testing.T) { + err := memoryStore.Erase("https://example.test") + assert.NilError(t, err) + + _, err = memoryStore.Get("https://example.test") + assert.Check(t, is.ErrorIs(err, errValueNotFound)) + + _, err = fallbackStore.Get("https://example.test") assert.Check(t, is.ErrorIs(err, errValueNotFound)) }) }