From 555aba27af1e41ec4ee8c9e52eb7f74c87c9cf6b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 21 Oct 2024 22:47:00 +0200 Subject: [PATCH] cli/config/credentials: refactor DetectDefaultStore and add tests Refactor the DetectDefaultStore to allow testing it cross-platform, and without the actual helpers installed. This also makes a small change in the logic for detecting the preferred helper. Previously, it only detected the "helper" binary ("pass"), but would fall back to using plain-text if the pass credentials-helper was not installed. With this patch, it falls back to the platform default (secretservice), before falling back to using no credentials helper (and storing unencrypted). Signed-off-by: Sebastiaan van Stijn --- cli/config/credentials/default_store.go | 71 +++++++++++++++--- .../credentials/default_store_darwin.go | 7 +- cli/config/credentials/default_store_linux.go | 13 +--- cli/config/credentials/default_store_test.go | 74 +++++++++++++++++++ .../credentials/default_store_unsupported.go | 7 +- .../credentials/default_store_windows.go | 7 +- 6 files changed, 149 insertions(+), 30 deletions(-) create mode 100644 cli/config/credentials/default_store_test.go diff --git a/cli/config/credentials/default_store.go b/cli/config/credentials/default_store.go index a36afc41f4..7f600ee8c5 100644 --- a/cli/config/credentials/default_store.go +++ b/cli/config/credentials/default_store.go @@ -2,21 +2,70 @@ package credentials import "os/exec" -// DetectDefaultStore return the default credentials store for the platform if -// no user-defined store is passed, and the store executable is available. -func DetectDefaultStore(store string) string { - if store != "" { +// DetectDefaultStore returns the credentials store to use if no user-defined +// custom helper is passed. +// +// Some platforms define a preferred helper, in which case it attempts to look +// up the helper binary before falling back to the platform's default. +// +// If no user-defined helper is passed, and no helper is found, it returns an +// empty string, which means credentials are stored unencrypted in the CLI's +// config-file without the use of a credentials store. +func DetectDefaultStore(customStore string) string { + if customStore != "" { // use user-defined - return store + return customStore } - - platformDefault := defaultCredentialsStore() - if platformDefault == "" { + if preferred := findPreferredHelper(); preferred != "" { + return preferred + } + if defaultHelper == "" { return "" } - - if _, err := exec.LookPath(remoteCredentialsPrefix + platformDefault); err != nil { + if _, err := exec.LookPath(remoteCredentialsPrefix + defaultHelper); err != nil { return "" } - return platformDefault + return defaultHelper +} + +// overridePreferred is used to override the preferred helper in tests. +var overridePreferred string + +// findPreferredHelper detects whether the preferred credentials-store and +// its helper binaries are installed. It returns the name of the preferred +// store if found, otherwise returns an empty string to fall back to resolving +// the default helper. +// +// Note that the logic below is currently specific to detection needed for the +// "pass" credentials-helper on Linux (which is the only platform with a preferred +// helper). It is put in a non-platform specific file to allow running tests +// on other platforms. +func findPreferredHelper() string { + preferred := preferredHelper + if overridePreferred != "" { + preferred = overridePreferred + } + if preferred == "" { + return "" + } + + // Note that the logic below is specific to detection needed for the + // "pass" credentials-helper on Linux (which is the only platform with + // a "preferred" and "default" helper. This logic may change if a similar + // order of preference is needed on other platforms. + + // If we don't have the preferred helper installed, there's no need + // to check if its dependencies are installed, instead, try to + // use the default credentials-helper for this platform (if installed). + if _, err := exec.LookPath(remoteCredentialsPrefix + preferred); err != nil { + return "" + } + + // Detect if the helper binary is present as well. This is needed for + // the "pass" credentials helper, which uses this binary. + if _, err := exec.LookPath(preferred); err != nil { + return "" + } + + return preferred } diff --git a/cli/config/credentials/default_store_darwin.go b/cli/config/credentials/default_store_darwin.go index 5d42dec622..1375f534e9 100644 --- a/cli/config/credentials/default_store_darwin.go +++ b/cli/config/credentials/default_store_darwin.go @@ -1,5 +1,6 @@ package credentials -func defaultCredentialsStore() string { - return "osxkeychain" -} +const ( + preferredHelper = "" + defaultHelper = "osxkeychain" +) diff --git a/cli/config/credentials/default_store_linux.go b/cli/config/credentials/default_store_linux.go index a9012c6d4a..390d488ff0 100644 --- a/cli/config/credentials/default_store_linux.go +++ b/cli/config/credentials/default_store_linux.go @@ -1,13 +1,6 @@ package credentials -import ( - "os/exec" +const ( + preferredHelper = "pass" + defaultHelper = "secretservice" ) - -func defaultCredentialsStore() string { - if _, err := exec.LookPath("pass"); err == nil { - return "pass" - } - - return "secretservice" -} diff --git a/cli/config/credentials/default_store_test.go b/cli/config/credentials/default_store_test.go new file mode 100644 index 0000000000..38228b3149 --- /dev/null +++ b/cli/config/credentials/default_store_test.go @@ -0,0 +1,74 @@ +package credentials + +import ( + "os" + "path" + "testing" + + "gotest.tools/v3/assert" +) + +func TestDetectDefaultStore(t *testing.T) { + tmpDir := t.TempDir() + t.Setenv("PATH", tmpDir) + + t.Run("none available", func(t *testing.T) { + const expected = "" + assert.Equal(t, expected, DetectDefaultStore("")) + }) + t.Run("custom helper", func(t *testing.T) { + const expected = "my-custom-helper" + assert.Equal(t, expected, DetectDefaultStore(expected)) + + // Custom helper should be used even if the actual helper exists + createFakeHelper(t, path.Join(tmpDir, remoteCredentialsPrefix+defaultHelper)) + assert.Equal(t, expected, DetectDefaultStore(expected)) + }) + t.Run("default", func(t *testing.T) { + createFakeHelper(t, path.Join(tmpDir, remoteCredentialsPrefix+defaultHelper)) + expected := defaultHelper + assert.Equal(t, expected, DetectDefaultStore("")) + }) + + // On Linux, the "pass" credentials helper requires both a "pass" binary + // to be present and a "docker-credentials-pass" credentials helper to + // be installed. + t.Run("preferred helper", func(t *testing.T) { + // Create the default helper as we need it for the fallback. + createFakeHelper(t, path.Join(tmpDir, remoteCredentialsPrefix+defaultHelper)) + + const testPreferredHelper = "preferred" + overridePreferred = testPreferredHelper + + // Use preferred helper if both binaries exist. + t.Run("success", func(t *testing.T) { + createFakeHelper(t, path.Join(tmpDir, testPreferredHelper)) + createFakeHelper(t, path.Join(tmpDir, remoteCredentialsPrefix+testPreferredHelper)) + expected := testPreferredHelper + assert.Equal(t, expected, DetectDefaultStore("")) + }) + + // Fall back to the default helper if the preferred credentials-helper isn't installed. + t.Run("not installed", func(t *testing.T) { + createFakeHelper(t, path.Join(tmpDir, remoteCredentialsPrefix+testPreferredHelper)) + expected := defaultHelper + assert.Equal(t, expected, DetectDefaultStore("")) + }) + + // Similarly, fall back to the default helper if the preferred credentials-helper + // is installed, but the helper binary isn't found. + t.Run("missing helper", func(t *testing.T) { + createFakeHelper(t, path.Join(tmpDir, testPreferredHelper)) + expected := defaultHelper + assert.Equal(t, expected, DetectDefaultStore("")) + }) + }) +} + +func createFakeHelper(t *testing.T, fileName string) { + t.Helper() + assert.NilError(t, os.WriteFile(fileName, []byte("I'm a credentials-helper executable (really!)"), 0o700)) + t.Cleanup(func() { + assert.NilError(t, os.RemoveAll(fileName)) + }) +} diff --git a/cli/config/credentials/default_store_unsupported.go b/cli/config/credentials/default_store_unsupported.go index 40c16eb837..80aa408372 100644 --- a/cli/config/credentials/default_store_unsupported.go +++ b/cli/config/credentials/default_store_unsupported.go @@ -2,6 +2,7 @@ package credentials -func defaultCredentialsStore() string { - return "" -} +const ( + preferredHelper = "" + defaultHelper = "" +) diff --git a/cli/config/credentials/default_store_windows.go b/cli/config/credentials/default_store_windows.go index bb799ca61b..2008f49fbf 100644 --- a/cli/config/credentials/default_store_windows.go +++ b/cli/config/credentials/default_store_windows.go @@ -1,5 +1,6 @@ package credentials -func defaultCredentialsStore() string { - return "wincred" -} +const ( + preferredHelper = "" + defaultHelper = "wincred" +)