From 365e7a5a4e64192aae3e1d0d7efa946f1fe98431 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 19 Oct 2024 13:59:49 +0200 Subject: [PATCH 1/3] cli/config/credentials: skip saving config-file if credentials didn't change Before this change, the config-file was always updated, even if there were no changes to save. This could cause issues when the config-file already had credentials set and was read-only for the current user. For example, on NixOS, this poses a problem because `config.json` is a symlink to a write-protected file; $ readlink ~/.docker/config.json /home/username/.config/sops-nix/secrets/ghcr_auth $ readlink -f ~/.docker/config.json /run/user/1000/secrets.d/28/ghcr_auth Which causes `docker login` to fail, even if no changes were to be made; Error saving credentials: rename /home/derek/.docker/config.json2180380217 /home/username/.config/sops-nix/secrets/ghcr_auth: invalid cross-device link This patch updates the code to only update the config file if changes were detected. It there's nothing to save, it skips updating the file, as well as skips printing the warning about credentials being stored insecurely. With this patch applied: $ docker login -u yourname Password: WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'. Configure a credential helper to remove this warning. See https://docs.docker.com/go/credential-store/ Login Succeeded $ docker login -u yourname Password: Login Succeeded Signed-off-by: Sebastiaan van Stijn (cherry picked from commit d3f6867e4d7f5018ae4c0fbc709934893f0e95a2) Signed-off-by: Sebastiaan van Stijn --- cli/config/credentials/file_store.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/cli/config/credentials/file_store.go b/cli/config/credentials/file_store.go index 3b8955994dc2..5eca79dd12c5 100644 --- a/cli/config/credentials/file_store.go +++ b/cli/config/credentials/file_store.go @@ -27,6 +27,10 @@ func NewFileStore(file store) Store { // Erase removes the given credentials from the file store. func (c *fileStore) Erase(serverAddress string) error { + if _, exists := c.file.GetAuthConfigs()[serverAddress]; !exists { + // nothing to do; no credentials found for the given serverAddress + return nil + } delete(c.file.GetAuthConfigs(), serverAddress) return c.file.Save() } @@ -52,9 +56,14 @@ func (c *fileStore) GetAll() (map[string]types.AuthConfig, error) { return c.file.GetAuthConfigs(), nil } -// Store saves the given credentials in the file store. +// Store saves the given credentials in the file store. This function is +// idempotent and does not update the file if credentials did not change. func (c *fileStore) Store(authConfig types.AuthConfig) error { authConfigs := c.file.GetAuthConfigs() + if oldAuthConfig, ok := authConfigs[authConfig.ServerAddress]; ok && oldAuthConfig == authConfig { + // Credentials didn't change, so skip updating the configuration file. + return nil + } authConfigs[authConfig.ServerAddress] = authConfig return c.file.Save() } From 720da3c65aa266c3ea18f94878148b8d3ba9312f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 22 Oct 2024 10:58:29 +0200 Subject: [PATCH 2/3] cil/config/credentials: remove newStore() test-utility This function was names slightly confusing, as it returns a fakeStore, and it didn't do any constructing, so didn't provide value above just constructing the type. I'm planning to add more functionality to the fakeStore, but don't want to maintain a full-fledged constructor for all of that, so let's remove this utility. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 0dd6f7f1b33bb6791bb4b22e84918bf7ff3b2510) Signed-off-by: Sebastiaan van Stijn --- cli/config/credentials/file_store_test.go | 18 +++++------- cli/config/credentials/native_store_test.go | 32 ++++++++++----------- 2 files changed, 23 insertions(+), 27 deletions(-) diff --git a/cli/config/credentials/file_store_test.go b/cli/config/credentials/file_store_test.go index 94e505e13c3d..077d89dc8e10 100644 --- a/cli/config/credentials/file_store_test.go +++ b/cli/config/credentials/file_store_test.go @@ -24,12 +24,8 @@ func (f *fakeStore) GetFilename() string { return "/tmp/docker-fakestore" } -func newStore(auths map[string]types.AuthConfig) store { - return &fakeStore{configs: auths} -} - func TestFileStoreAddCredentials(t *testing.T) { - f := newStore(make(map[string]types.AuthConfig)) + f := &fakeStore{configs: map[string]types.AuthConfig{}} s := NewFileStore(f) auth := types.AuthConfig{ @@ -47,13 +43,13 @@ func TestFileStoreAddCredentials(t *testing.T) { } func TestFileStoreGet(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", ServerAddress: "https://example.com", }, - }) + }} s := NewFileStore(f) a, err := s.Get("https://example.com") @@ -71,7 +67,7 @@ func TestFileStoreGet(t *testing.T) { func TestFileStoreGetAll(t *testing.T) { s1 := "https://example.com" s2 := "https://example2.example.com" - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ s1: { Auth: "super_secret_token", Email: "foo@example.com", @@ -82,7 +78,7 @@ func TestFileStoreGetAll(t *testing.T) { Email: "foo@example2.com", ServerAddress: "https://example2.example.com", }, - }) + }} s := NewFileStore(f) as, err := s.GetAll() @@ -107,13 +103,13 @@ func TestFileStoreGetAll(t *testing.T) { } func TestFileStoreErase(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ "https://example.com": { Auth: "super_secret_token", Email: "foo@example.com", ServerAddress: "https://example.com", }, - }) + }} s := NewFileStore(f) err := s.Erase("https://example.com") diff --git a/cli/config/credentials/native_store_test.go b/cli/config/credentials/native_store_test.go index 5abcca3587e0..f55d269ed59f 100644 --- a/cli/config/credentials/native_store_test.go +++ b/cli/config/credentials/native_store_test.go @@ -91,7 +91,7 @@ func mockCommandFn(args ...string) client.Program { } func TestNativeStoreAddCredentials(t *testing.T) { - f := newStore(make(map[string]types.AuthConfig)) + f := &fakeStore{configs: map[string]types.AuthConfig{}} s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -116,7 +116,7 @@ func TestNativeStoreAddCredentials(t *testing.T) { } func TestNativeStoreAddInvalidCredentials(t *testing.T) { - f := newStore(make(map[string]types.AuthConfig)) + f := &fakeStore{configs: map[string]types.AuthConfig{}} s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -132,11 +132,11 @@ func TestNativeStoreAddInvalidCredentials(t *testing.T) { } func TestNativeStoreGet(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, fileStore: NewFileStore(f), @@ -154,11 +154,11 @@ func TestNativeStoreGet(t *testing.T) { } func TestNativeStoreGetIdentityToken(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress2: { Email: "foo@example2.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -176,11 +176,11 @@ func TestNativeStoreGetIdentityToken(t *testing.T) { } func TestNativeStoreGetAll(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -217,11 +217,11 @@ func TestNativeStoreGetAll(t *testing.T) { } func TestNativeStoreGetMissingCredentials(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -232,11 +232,11 @@ func TestNativeStoreGetMissingCredentials(t *testing.T) { } func TestNativeStoreGetInvalidAddress(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -247,11 +247,11 @@ func TestNativeStoreGetInvalidAddress(t *testing.T) { } func TestNativeStoreErase(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, @@ -263,11 +263,11 @@ func TestNativeStoreErase(t *testing.T) { } func TestNativeStoreEraseInvalidAddress(t *testing.T) { - f := newStore(map[string]types.AuthConfig{ + f := &fakeStore{configs: map[string]types.AuthConfig{ validServerAddress: { Email: "foo@example.com", }, - }) + }} s := &nativeStore{ programFunc: mockCommandFn, From 6736be779a228d70c9104aa9cd81d984e9749661 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 22 Oct 2024 12:11:36 +0200 Subject: [PATCH 3/3] cli/config/credentials: add test for save being idempotent Test case for d3f6867e4d7f5018ae4c0fbc709934893f0e95a2 Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 3c78069240cf69713ffabe1e0212c02e0766de53) Signed-off-by: Sebastiaan van Stijn --- cli/config/credentials/file_store.go | 3 +- cli/config/credentials/file_store_test.go | 79 ++++++++++++++++++++++- 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/cli/config/credentials/file_store.go b/cli/config/credentials/file_store.go index 5eca79dd12c5..95406281501c 100644 --- a/cli/config/credentials/file_store.go +++ b/cli/config/credentials/file_store.go @@ -25,7 +25,8 @@ func NewFileStore(file store) Store { return &fileStore{file: file} } -// Erase removes the given credentials from the file store. +// Erase removes the given credentials from the file store.This function is +// idempotent and does not update the file if credentials did not change. func (c *fileStore) Erase(serverAddress string) error { if _, exists := c.file.GetAuthConfigs()[serverAddress]; !exists { // nothing to do; no credentials found for the given serverAddress diff --git a/cli/config/credentials/file_store_test.go b/cli/config/credentials/file_store_test.go index 077d89dc8e10..466fa054a03e 100644 --- a/cli/config/credentials/file_store_test.go +++ b/cli/config/credentials/file_store_test.go @@ -10,9 +10,15 @@ import ( type fakeStore struct { configs map[string]types.AuthConfig + saveFn func(*fakeStore) error } func (f *fakeStore) Save() error { + if f.saveFn != nil { + // Pass a reference to the fakeStore itself in case saveFn + // wants to access it. + return f.saveFn(f) + } return nil } @@ -21,7 +27,78 @@ func (f *fakeStore) GetAuthConfigs() map[string]types.AuthConfig { } func (f *fakeStore) GetFilename() string { - return "/tmp/docker-fakestore" + return "no-config.json" +} + +// TestFileStoreIdempotent verifies that the config-file isn't updated +// if nothing changed. +func TestFileStoreIdempotent(t *testing.T) { + var saveCount, expectedSaveCount int + + s := NewFileStore(&fakeStore{ + configs: map[string]types.AuthConfig{}, + saveFn: func(*fakeStore) error { + saveCount++ + return nil + }, + }) + authOne := types.AuthConfig{ + Auth: "super_secret_token", + Email: "foo@example.com", + ServerAddress: "https://example.com", + } + authTwo := types.AuthConfig{ + Auth: "also_super_secret_token", + Email: "bar@example.com", + ServerAddress: "https://other.example.com", + } + + expectedSaveCount = 1 + t.Run("store new credentials", func(t *testing.T) { + assert.NilError(t, s.Store(authOne)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, authOne)) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) + t.Run("store same credentials is a no-op", func(t *testing.T) { + assert.NilError(t, s.Store(authOne)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, authOne)) + assert.Check(t, is.Equal(saveCount, expectedSaveCount), "should not have saved if nothing changed") + }) + t.Run("store other credentials", func(t *testing.T) { + expectedSaveCount++ + assert.NilError(t, s.Store(authTwo)) + retrievedAuth, err := s.Get(authTwo.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, authTwo)) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) + t.Run("erase credentials", func(t *testing.T) { + expectedSaveCount++ + assert.NilError(t, s.Erase(authOne.ServerAddress)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, types.AuthConfig{})) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) + t.Run("erase non-existing credentials is a no-op", func(t *testing.T) { + assert.NilError(t, s.Erase(authOne.ServerAddress)) + retrievedAuth, err := s.Get(authOne.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, types.AuthConfig{})) + assert.Check(t, is.Equal(saveCount, expectedSaveCount), "should not have saved if nothing changed") + }) + t.Run("erase other credentials", func(t *testing.T) { + expectedSaveCount++ + assert.NilError(t, s.Erase(authTwo.ServerAddress)) + retrievedAuth, err := s.Get(authTwo.ServerAddress) + assert.NilError(t, err) + assert.Check(t, is.Equal(retrievedAuth, types.AuthConfig{})) + assert.Check(t, is.Equal(saveCount, expectedSaveCount)) + }) } func TestFileStoreAddCredentials(t *testing.T) {