From 1d0a37c4600650cc98fc664005d533fffedd8cb8 Mon Sep 17 00:00:00 2001 From: Jake Sanders Date: Fri, 26 Jan 2018 14:38:04 -0800 Subject: [PATCH 1/2] Use Get rather than GetAll to retrieve credentials from credential helpers. Signed-off-by: Jake Sanders --- cli/config/configfile/file.go | 38 ++--- cli/config/configfile/file_test.go | 216 ++++++++++++++++++++++++++++- 2 files changed, 237 insertions(+), 17 deletions(-) diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index 4f1dc1ae5b..f513144bdc 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -251,24 +251,29 @@ func decodeAuth(authStr string) (string, string, error) { // GetCredentialsStore returns a new credentials store from the settings in the // configuration file -func (configFile *ConfigFile) GetCredentialsStore(serverAddress string) credentials.Store { - if helper := getConfiguredCredentialStore(configFile, serverAddress); helper != "" { - return credentials.NewNativeStore(configFile, helper) +func (configFile *ConfigFile) GetCredentialsStore(registryHostname string) credentials.Store { + if helper := getConfiguredCredentialStore(configFile, registryHostname); helper != "" { + return newNativeStore(configFile, helper) } return credentials.NewFileStore(configFile) } +// var for unit testing. +var newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store { + return credentials.NewNativeStore(configFile, helperSuffix) +} + // GetAuthConfig for a repository from the credential store -func (configFile *ConfigFile) GetAuthConfig(serverAddress string) (types.AuthConfig, error) { - return configFile.GetCredentialsStore(serverAddress).Get(serverAddress) +func (configFile *ConfigFile) GetAuthConfig(registryHostname string) (types.AuthConfig, error) { + return configFile.GetCredentialsStore(registryHostname).Get(registryHostname) } // getConfiguredCredentialStore returns the credential helper configured for the // given registry, the default credsStore, or the empty string if neither are // configured. -func getConfiguredCredentialStore(c *ConfigFile, serverAddress string) string { - if c.CredentialHelpers != nil && serverAddress != "" { - if helper, exists := c.CredentialHelpers[serverAddress]; exists { +func getConfiguredCredentialStore(c *ConfigFile, registryHostname string) string { + if c.CredentialHelpers != nil && registryHostname != "" { + if helper, exists := c.CredentialHelpers[registryHostname]; exists { return helper } } @@ -285,19 +290,20 @@ func (configFile *ConfigFile) GetAllCredentials() (map[string]types.AuthConfig, } } - for registry := range configFile.CredentialHelpers { - helper := configFile.GetCredentialsStore(registry) - newAuths, err := helper.GetAll() - if err != nil { - return nil, err - } - addAll(newAuths) - } defaultStore := configFile.GetCredentialsStore("") newAuths, err := defaultStore.GetAll() if err != nil { return nil, err } addAll(newAuths) + + // Auth configs from a registry-specific helper should override those from the default store. + for registryHostname := range configFile.CredentialHelpers { + newAuth, err := configFile.GetAuthConfig(registryHostname) + if err != nil { + return nil, err + } + auths[registryHostname] = newAuth + } return auths, nil } diff --git a/cli/config/configfile/file_test.go b/cli/config/configfile/file_test.go index f2a61b1794..75df18c49a 100644 --- a/cli/config/configfile/file_test.go +++ b/cli/config/configfile/file_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/docker/cli/cli/config/credentials" "github.com/docker/docker/api/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -142,7 +143,37 @@ func TestConfigFile(t *testing.T) { assert.Equal(t, configFilename, configFile.Filename) } -func TestGetAllCredentials(t *testing.T) { +type mockNativeStore struct { + GetAllCallCount int + authConfigs map[string]types.AuthConfig +} + +func (c *mockNativeStore) Erase(registryHostname string) error { + delete(c.authConfigs, registryHostname) + return nil +} + +func (c *mockNativeStore) Get(registryHostname string) (types.AuthConfig, error) { + return c.authConfigs[registryHostname], nil +} + +func (c *mockNativeStore) GetAll() (map[string]types.AuthConfig, error) { + c.GetAllCallCount = c.GetAllCallCount + 1 + return c.authConfigs, nil +} + +func (c *mockNativeStore) Store(authConfig types.AuthConfig) error { + return nil +} + +// make sure it satisfies the interface +var _ credentials.Store = (*mockNativeStore)(nil) + +func NewMockNativeStore(authConfigs map[string]types.AuthConfig) credentials.Store { + return &mockNativeStore{authConfigs: authConfigs} +} + +func TestGetAllCredentialsFileStoreOnly(t *testing.T) { configFile := New("filename") exampleAuth := types.AuthConfig{ Username: "user", @@ -157,3 +188,186 @@ func TestGetAllCredentials(t *testing.T) { expected["example.com/foo"] = exampleAuth assert.Equal(t, expected, authConfigs) } + +func TestGetAllCredentialsCredsStore(t *testing.T) { + configFile := New("filename") + configFile.CredentialsStore = "test_creds_store" + testRegistryHostname := "example.com" + expectedAuth := types.AuthConfig{ + Username: "user", + Password: "pass", + } + + testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: expectedAuth}) + + tmpNewNativeStore := newNativeStore + defer func() { newNativeStore = tmpNewNativeStore }() + newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store { + return testCredsStore + } + + authConfigs, err := configFile.GetAllCredentials() + require.NoError(t, err) + + expected := make(map[string]types.AuthConfig) + expected[testRegistryHostname] = expectedAuth + assert.Equal(t, expected, authConfigs) + assert.Equal(t, 1, testCredsStore.(*mockNativeStore).GetAllCallCount) +} + +func TestGetAllCredentialsCredHelper(t *testing.T) { + testCredHelperSuffix := "test_cred_helper" + testCredHelperRegistryHostname := "credhelper.com" + testExtraCredHelperRegistryHostname := "somethingweird.com" + + unexpectedCredHelperAuth := types.AuthConfig{ + Username: "file_store_user", + Password: "file_store_pass", + } + expectedCredHelperAuth := types.AuthConfig{ + Username: "cred_helper_user", + Password: "cred_helper_pass", + } + + configFile := New("filename") + configFile.CredentialHelpers = map[string]string{testCredHelperRegistryHostname: testCredHelperSuffix} + + testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{ + testCredHelperRegistryHostname: expectedCredHelperAuth, + // Add in an extra auth entry which doesn't appear in CredentialHelpers section of the configFile. + // This verifies that only explicitly configured registries are being requested from the cred helpers. + testExtraCredHelperRegistryHostname: unexpectedCredHelperAuth, + }) + + tmpNewNativeStore := newNativeStore + defer func() { newNativeStore = tmpNewNativeStore }() + newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store { + return testCredHelper + } + + authConfigs, err := configFile.GetAllCredentials() + require.NoError(t, err) + + expected := make(map[string]types.AuthConfig) + expected[testCredHelperRegistryHostname] = expectedCredHelperAuth + assert.Equal(t, expected, authConfigs) + assert.Equal(t, 0, testCredHelper.(*mockNativeStore).GetAllCallCount) +} + +func TestGetAllCredentialsFileStoreAndCredHelper(t *testing.T) { + testFileStoreRegistryHostname := "example.com" + testCredHelperSuffix := "test_cred_helper" + testCredHelperRegistryHostname := "credhelper.com" + + expectedFileStoreAuth := types.AuthConfig{ + Username: "file_store_user", + Password: "file_store_pass", + } + expectedCredHelperAuth := types.AuthConfig{ + Username: "cred_helper_user", + Password: "cred_helper_pass", + } + + configFile := New("filename") + configFile.CredentialHelpers = map[string]string{testCredHelperRegistryHostname: testCredHelperSuffix} + configFile.AuthConfigs[testFileStoreRegistryHostname] = expectedFileStoreAuth + + testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testCredHelperRegistryHostname: expectedCredHelperAuth}) + + newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store { + return testCredHelper + } + + tmpNewNativeStore := newNativeStore + defer func() { newNativeStore = tmpNewNativeStore }() + authConfigs, err := configFile.GetAllCredentials() + require.NoError(t, err) + + expected := make(map[string]types.AuthConfig) + expected[testFileStoreRegistryHostname] = expectedFileStoreAuth + expected[testCredHelperRegistryHostname] = expectedCredHelperAuth + assert.Equal(t, expected, authConfigs) + assert.Equal(t, 0, testCredHelper.(*mockNativeStore).GetAllCallCount) +} + +func TestGetAllCredentialsCredStoreAndCredHelper(t *testing.T) { + testCredStoreSuffix := "test_creds_store" + testCredStoreRegistryHostname := "credstore.com" + testCredHelperSuffix := "test_cred_helper" + testCredHelperRegistryHostname := "credhelper.com" + + configFile := New("filename") + configFile.CredentialsStore = testCredStoreSuffix + configFile.CredentialHelpers = map[string]string{testCredHelperRegistryHostname: testCredHelperSuffix} + + expectedCredStoreAuth := types.AuthConfig{ + Username: "cred_store_user", + Password: "cred_store_pass", + } + expectedCredHelperAuth := types.AuthConfig{ + Username: "cred_helper_user", + Password: "cred_helper_pass", + } + + testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testCredHelperRegistryHostname: expectedCredHelperAuth}) + testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testCredStoreRegistryHostname: expectedCredStoreAuth}) + + tmpNewNativeStore := newNativeStore + defer func() { newNativeStore = tmpNewNativeStore }() + newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store { + if helperSuffix == testCredHelperSuffix { + return testCredHelper + } + return testCredsStore + } + + authConfigs, err := configFile.GetAllCredentials() + require.NoError(t, err) + + expected := make(map[string]types.AuthConfig) + expected[testCredStoreRegistryHostname] = expectedCredStoreAuth + expected[testCredHelperRegistryHostname] = expectedCredHelperAuth + assert.Equal(t, expected, authConfigs) + assert.Equal(t, 1, testCredsStore.(*mockNativeStore).GetAllCallCount) + assert.Equal(t, 0, testCredHelper.(*mockNativeStore).GetAllCallCount) +} + +func TestGetAllCredentialsCredHelperOverridesDefaultStore(t *testing.T) { + testCredStoreSuffix := "test_creds_store" + testCredHelperSuffix := "test_cred_helper" + testRegistryHostname := "example.com" + + configFile := New("filename") + configFile.CredentialsStore = testCredStoreSuffix + configFile.CredentialHelpers = map[string]string{testRegistryHostname: testCredHelperSuffix} + + unexpectedCredStoreAuth := types.AuthConfig{ + Username: "cred_store_user", + Password: "cred_store_pass", + } + expectedCredHelperAuth := types.AuthConfig{ + Username: "cred_helper_user", + Password: "cred_helper_pass", + } + + testCredHelper := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: expectedCredHelperAuth}) + testCredsStore := NewMockNativeStore(map[string]types.AuthConfig{testRegistryHostname: unexpectedCredStoreAuth}) + + tmpNewNativeStore := newNativeStore + defer func() { newNativeStore = tmpNewNativeStore }() + newNativeStore = func(configFile *ConfigFile, helperSuffix string) credentials.Store { + if helperSuffix == testCredHelperSuffix { + return testCredHelper + } + return testCredsStore + } + + authConfigs, err := configFile.GetAllCredentials() + require.NoError(t, err) + + expected := make(map[string]types.AuthConfig) + expected[testRegistryHostname] = expectedCredHelperAuth + assert.Equal(t, expected, authConfigs) + assert.Equal(t, 1, testCredsStore.(*mockNativeStore).GetAllCallCount) + assert.Equal(t, 0, testCredHelper.(*mockNativeStore).GetAllCallCount) +} From e1b607a3511dfe7259434de959cda87ec9e0bd34 Mon Sep 17 00:00:00 2001 From: Jake Sanders Date: Fri, 9 Feb 2018 13:48:01 -0800 Subject: [PATCH 2/2] defaultIndexserver -> defaultIndexServer Signed-off-by: Jake Sanders --- cli/config/configfile/file.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index f513144bdc..9338bc4e66 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -19,7 +19,7 @@ const ( // This constant is only used for really old config files when the // URL wasn't saved as part of the config file and it was just // assumed to be this value. - defaultIndexserver = "https://index.docker.io/v1/" + defaultIndexServer = "https://index.docker.io/v1/" ) // ConfigFile ~/.docker/config.json file info @@ -87,8 +87,8 @@ func (configFile *ConfigFile) LegacyLoadFromReader(configData io.Reader) error { if err != nil { return err } - authConfig.ServerAddress = defaultIndexserver - configFile.AuthConfigs[defaultIndexserver] = authConfig + authConfig.ServerAddress = defaultIndexServer + configFile.AuthConfigs[defaultIndexServer] = authConfig } else { for k, authConfig := range configFile.AuthConfigs { authConfig.Username, authConfig.Password, err = decodeAuth(authConfig.Auth)