From 7931b93f0840f9d36570a7db1340cddb10b9ab85 Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Sat, 7 Sep 2024 17:55:57 +0100 Subject: [PATCH] fix: docker config error handling Handle file not exist error in getDockerAuthConfigs, treating it as if no authentication was provided. Use config directly for cache instead of loading the file a second time which may be the wrong file if loaded from the environment. Correctly handle json decode errors in getDockerConfig instead of falling back to the default config, which would result in unexpected behaviour. Tests refactored to ensure all edge cases for getDockerConfig and getDockerAuthConfigs are handled. Fixes #2767 --- docker_auth.go | 49 ++--- docker_auth_test.go | 218 ++++++++++++-------- testdata/invalid-config/.docker/config.json | 3 + 3 files changed, 159 insertions(+), 111 deletions(-) create mode 100644 testdata/invalid-config/.docker/config.json diff --git a/docker_auth.go b/docker_auth.go index 99e2d2fdba..af0d415de9 100644 --- a/docker_auth.go +++ b/docker_auth.go @@ -8,7 +8,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "net/url" "os" "sync" @@ -137,24 +136,12 @@ func (c *credentialsCache) Get(hostname, configKey string) (string, string, erro return user, password, nil } -// configFileKey returns a key to use for caching credentials based on +// configKey returns a key to use for caching credentials based on // the contents of the currently active config. -func configFileKey() (string, error) { - configPath, err := dockercfg.ConfigPath() - if err != nil { - return "", err - } - - f, err := os.Open(configPath) - if err != nil { - return "", fmt.Errorf("open config file: %w", err) - } - - defer f.Close() - +func configKey(cfg *dockercfg.Config) (string, error) { h := md5.New() - if _, err := io.Copy(h, f); err != nil { - return "", fmt.Errorf("copying config file: %w", err) + if err := json.NewEncoder(h).Encode(cfg); err != nil { + return "", fmt.Errorf("encode config: %w", err) } return hex.EncodeToString(h.Sum(nil)), nil @@ -165,10 +152,14 @@ func configFileKey() (string, error) { func getDockerAuthConfigs() (map[string]registry.AuthConfig, error) { cfg, err := getDockerConfig() if err != nil { + if errors.Is(err, os.ErrNotExist) { + return map[string]registry.AuthConfig{}, nil + } + return nil, err } - configKey, err := configFileKey() + key, err := configKey(cfg) if err != nil { return nil, err } @@ -195,7 +186,7 @@ func getDockerAuthConfigs() (map[string]registry.AuthConfig, error) { switch { case ac.Username == "" && ac.Password == "": // Look up credentials from the credential store. - u, p, err := creds.Get(k, configKey) + u, p, err := creds.Get(k, key) if err != nil { results <- authConfigResult{err: err} return @@ -218,7 +209,7 @@ func getDockerAuthConfigs() (map[string]registry.AuthConfig, error) { go func(k string) { defer wg.Done() - u, p, err := creds.Get(k, configKey) + u, p, err := creds.Get(k, key) if err != nil { results <- authConfigResult{err: err} return @@ -260,20 +251,20 @@ func getDockerAuthConfigs() (map[string]registry.AuthConfig, error) { // 1. the DOCKER_AUTH_CONFIG environment variable, unmarshalling it into a dockercfg.Config // 2. the DOCKER_CONFIG environment variable, as the path to the config file // 3. else it will load the default config file, which is ~/.docker/config.json -func getDockerConfig() (dockercfg.Config, error) { - dockerAuthConfig := os.Getenv("DOCKER_AUTH_CONFIG") - if dockerAuthConfig != "" { - cfg := dockercfg.Config{} - err := json.Unmarshal([]byte(dockerAuthConfig), &cfg) - if err == nil { - return cfg, nil +func getDockerConfig() (*dockercfg.Config, error) { + if env := os.Getenv("DOCKER_AUTH_CONFIG"); env != "" { + var cfg dockercfg.Config + if err := json.Unmarshal([]byte(env), &cfg); err != nil { + return nil, fmt.Errorf("unmarshal DOCKER_AUTH_CONFIG: %w", err) } + + return &cfg, nil } cfg, err := dockercfg.LoadDefaultConfig() if err != nil { - return cfg, err + return nil, fmt.Errorf("load default config: %w", err) } - return cfg, nil + return &cfg, nil } diff --git a/docker_auth_test.go b/docker_auth_test.go index 4e55d2b9bf..7e42ff83b9 100644 --- a/docker_auth_test.go +++ b/docker_auth_test.go @@ -14,7 +14,6 @@ import ( "github.com/docker/docker/api/types/image" "github.com/docker/docker/api/types/registry" "github.com/docker/docker/client" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/testcontainers/testcontainers-go/internal/core" @@ -23,91 +22,87 @@ import ( const exampleAuth = "https://example-auth.com" -var testDockerConfigDirPath = filepath.Join("testdata", ".docker") - -var indexDockerIO = core.IndexDockerIO - -func TestGetDockerConfig(t *testing.T) { - const expectedErrorMessage = "Expected to find %s in auth configs" - - // Verify that the default docker config file exists before any test in this suite runs. - // Then, we can safely run the tests that rely on it. - defaultCfg, err := dockercfg.LoadDefaultConfig() - require.NoError(t, err) - require.NotEmpty(t, defaultCfg) - - t.Run("without DOCKER_CONFIG env var retrieves default", func(t *testing.T) { - t.Setenv("DOCKER_CONFIG", "") +func Test_getDockerConfig(t *testing.T) { + expectedConfig := &dockercfg.Config{ + AuthConfigs: map[string]dockercfg.AuthConfig{ + core.IndexDockerIO: {}, + "https://example.com": {}, + "https://my.private.registry": {}, + }, + CredentialsStore: "desktop", + } + t.Run("HOME/valid", func(t *testing.T) { + testDockerConfigHome(t, "testdata") cfg, err := getDockerConfig() require.NoError(t, err) - require.NotEmpty(t, cfg) + require.Equal(t, expectedConfig, cfg) + }) - assert.Equal(t, defaultCfg, cfg) + t.Run("HOME/not-found", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-found") + + cfg, err := getDockerConfig() + require.ErrorIs(t, err, os.ErrNotExist) + require.Nil(t, cfg) }) - t.Run("with DOCKER_CONFIG env var pointing to a non-existing file raises error", func(t *testing.T) { - t.Setenv("DOCKER_CONFIG", filepath.Join(testDockerConfigDirPath, "non-existing")) + t.Run("HOME/invalid-config", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "invalid-config") cfg, err := getDockerConfig() - require.Error(t, err) - require.Empty(t, cfg) + require.ErrorContains(t, err, "json: cannot unmarshal array") + require.Nil(t, cfg) }) - t.Run("with DOCKER_CONFIG env var", func(t *testing.T) { - t.Setenv("DOCKER_CONFIG", testDockerConfigDirPath) + t.Run("DOCKER_AUTH_CONFIG/valid", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-found") + t.Setenv("DOCKER_AUTH_CONFIG", dockerConfig) cfg, err := getDockerConfig() require.NoError(t, err) - require.NotEmpty(t, cfg) - - assert.Len(t, cfg.AuthConfigs, 3) + require.Equal(t, expectedConfig, cfg) + }) - authCfgs := cfg.AuthConfigs + t.Run("DOCKER_AUTH_CONFIG/invalid-config", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-found") + t.Setenv("DOCKER_AUTH_CONFIG", `{"auths": []}`) - if _, ok := authCfgs[indexDockerIO]; !ok { - t.Errorf(expectedErrorMessage, indexDockerIO) - } - if _, ok := authCfgs["https://example.com"]; !ok { - t.Errorf(expectedErrorMessage, "https://example.com") - } - if _, ok := authCfgs["https://my.private.registry"]; !ok { - t.Errorf(expectedErrorMessage, "https://my.private.registry") - } + cfg, err := getDockerConfig() + require.ErrorContains(t, err, "json: cannot unmarshal array") + require.Nil(t, cfg) }) - t.Run("DOCKER_AUTH_CONFIG env var takes precedence", func(t *testing.T) { - setAuthConfig(t, exampleAuth, "", "") - t.Setenv("DOCKER_CONFIG", testDockerConfigDirPath) + t.Run("DOCKER_CONFIG/valid", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-found") + t.Setenv("DOCKER_CONFIG", filepath.Join("testdata", ".docker")) cfg, err := getDockerConfig() require.NoError(t, err) - require.NotEmpty(t, cfg) - - assert.Len(t, cfg.AuthConfigs, 1) + require.Equal(t, expectedConfig, cfg) + }) - authCfgs := cfg.AuthConfigs + t.Run("DOCKER_CONFIG/invalid-config", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-found") + t.Setenv("DOCKER_CONFIG", filepath.Join("testdata", "invalid-config", ".docker")) - if _, ok := authCfgs[indexDockerIO]; ok { - t.Errorf("Not expected to find %s in auth configs", indexDockerIO) - } - if _, ok := authCfgs[exampleAuth]; !ok { - t.Errorf(expectedErrorMessage, exampleAuth) - } + cfg, err := getDockerConfig() + require.ErrorContains(t, err, "json: cannot unmarshal array") + require.Nil(t, cfg) }) +} +func TestDockerImageAuth(t *testing.T) { t.Run("retrieve auth with DOCKER_AUTH_CONFIG env var", func(t *testing.T) { username, password := "gopher", "secret" creds := setAuthConfig(t, exampleAuth, username, password) registry, cfg, err := DockerImageAuth(context.Background(), exampleAuth+"/my/image:latest") require.NoError(t, err) - require.NotEmpty(t, cfg) - - assert.Equal(t, exampleAuth, registry) - assert.Equal(t, username, cfg.Username) - assert.Equal(t, password, cfg.Password) - assert.Equal(t, creds, cfg.Auth) + require.Equal(t, exampleAuth, registry) + require.Equal(t, username, cfg.Username) + require.Equal(t, password, cfg.Password) + require.Equal(t, creds, cfg.Auth) }) t.Run("match registry authentication by host", func(t *testing.T) { @@ -117,12 +112,10 @@ func TestGetDockerConfig(t *testing.T) { registry, cfg, err := DockerImageAuth(context.Background(), imageReg+imagePath) require.NoError(t, err) - require.NotEmpty(t, cfg) - - assert.Equal(t, imageReg, registry) - assert.Equal(t, "gopher", cfg.Username) - assert.Equal(t, "secret", cfg.Password) - assert.Equal(t, base64, cfg.Auth) + require.Equal(t, imageReg, registry) + require.Equal(t, "gopher", cfg.Username) + require.Equal(t, "secret", cfg.Password) + require.Equal(t, base64, cfg.Auth) }) t.Run("fail to match registry authentication due to invalid host", func(t *testing.T) { @@ -135,8 +128,7 @@ func TestGetDockerConfig(t *testing.T) { registry, cfg, err := DockerImageAuth(context.Background(), imageReg+imagePath) require.ErrorIs(t, err, dockercfg.ErrCredentialsNotFound) require.Empty(t, cfg) - - assert.Equal(t, imageReg, registry) + require.Equal(t, imageReg, registry) }) t.Run("fail to match registry authentication by host with empty URL scheme creds and missing default", func(t *testing.T) { @@ -156,8 +148,7 @@ func TestGetDockerConfig(t *testing.T) { registry, cfg, err := DockerImageAuth(context.Background(), imageReg+imagePath) require.ErrorIs(t, err, dockercfg.ErrCredentialsNotFound) require.Empty(t, cfg) - - assert.Equal(t, imageReg, registry) + require.Equal(t, imageReg, registry) }) } @@ -391,27 +382,90 @@ func localAddress(t *testing.T) string { var dockerConfig string func Test_getDockerAuthConfigs(t *testing.T) { - t.Run("file", func(t *testing.T) { - got, err := getDockerAuthConfigs() + t.Run("HOME/valid", func(t *testing.T) { + testDockerConfigHome(t, "testdata") + + requireValidAuthConfig(t) + }) + + t.Run("HOME/not-found", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-exist") + + authConfigs, err := getDockerAuthConfigs() require.NoError(t, err) - require.NotNil(t, got) + require.NotNil(t, authConfigs) + require.Empty(t, authConfigs) + }) + + t.Run("HOME/invalid-config", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "invalid-config") + + authConfigs, err := getDockerAuthConfigs() + require.ErrorContains(t, err, "json: cannot unmarshal array") + require.Nil(t, authConfigs) }) - t.Run("env", func(t *testing.T) { + t.Run("DOCKER_AUTH_CONFIG/valid", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-exist") t.Setenv("DOCKER_AUTH_CONFIG", dockerConfig) - got, err := getDockerAuthConfigs() - require.NoError(t, err) + requireValidAuthConfig(t) + }) - // We can only check the keys as the values are not deterministic. - expected := map[string]registry.AuthConfig{ - "https://index.docker.io/v1/": {}, - "https://example.com": {}, - "https://my.private.registry": {}, - } - for k := range got { - got[k] = registry.AuthConfig{} - } - require.Equal(t, expected, got) + t.Run("DOCKER_AUTH_CONFIG/invalid-config", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-exist") + t.Setenv("DOCKER_AUTH_CONFIG", `{"auths": []}`) + + authConfigs, err := getDockerAuthConfigs() + require.ErrorContains(t, err, "json: cannot unmarshal array") + require.Nil(t, authConfigs) }) + + t.Run("DOCKER_CONFIG/valid", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-found") + t.Setenv("DOCKER_CONFIG", filepath.Join("testdata", ".docker")) + + requireValidAuthConfig(t) + }) + + t.Run("DOCKER_CONFIG/invalid-config", func(t *testing.T) { + testDockerConfigHome(t, "testdata", "not-found") + t.Setenv("DOCKER_CONFIG", filepath.Join("testdata", "invalid-config", ".docker")) + + cfg, err := getDockerConfig() + require.ErrorContains(t, err, "json: cannot unmarshal array") + require.Nil(t, cfg) + }) +} + +// requireValidAuthConfig checks that the given authConfigs map contains the expected keys. +func requireValidAuthConfig(t *testing.T) { + t.Helper() + + authConfigs, err := getDockerAuthConfigs() + require.NoError(t, err) + + // We can only check the keys as the values are not deterministic as they depend + // on users environment. + expected := map[string]registry.AuthConfig{ + "https://index.docker.io/v1/": {}, + "https://example.com": {}, + "https://my.private.registry": {}, + } + for k := range authConfigs { + authConfigs[k] = registry.AuthConfig{} + } + require.Equal(t, expected, authConfigs) +} + +// testDockerConfigHome sets the user's home directory to the given path +// and unsets the DOCKER_CONFIG and DOCKER_AUTH_CONFIG environment variables. +func testDockerConfigHome(t *testing.T, dirs ...string) { + t.Helper() + + dir := filepath.Join(dirs...) + t.Setenv("DOCKER_AUTH_CONFIG", "") + t.Setenv("DOCKER_CONFIG", "") + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) // Windows } diff --git a/testdata/invalid-config/.docker/config.json b/testdata/invalid-config/.docker/config.json new file mode 100644 index 0000000000..f0f444f355 --- /dev/null +++ b/testdata/invalid-config/.docker/config.json @@ -0,0 +1,3 @@ +{ + "auths": [] +}