Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli/config/credentials: refactor DetectDefaultStore and add tests #5568

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 58 additions & 9 deletions cli/config/credentials/default_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +14 to 18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sidenote, but

func bork(foo string) string {
	if foo != "" {
		return foo
	}
	...
}

is definitely something 😅 we should see if we could change this for the next major.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so I the logic here was that if the config-file has a credentials-store configured, we accept it whatever it is, and consider it an option set by the user; when used, it may error, but that's intentional (you configured a helper, we tried using it, and it failed! intervention is needed).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But now the fun bit;

cli/cli/config/config.go

Lines 162 to 172 in 32ff200

func LoadDefaultConfigFile(stderr io.Writer) *configfile.ConfigFile {
configFile, err := load(Dir())
if err != nil {
// FIXME(thaJeztah): we should not proceed here to prevent overwriting existing (but malformed) config files; see https://github.com/docker/cli/issues/5075
_, _ = fmt.Fprintln(stderr, "WARNING: Error", err)
}
if !configFile.ContainsAuth() {
configFile.CredentialsStore = credentials.DetectDefaultStore(configFile.CredentialsStore)
}
return configFile
}

See what's wrong in the logic there? Hint:

if !configFile.ContainsAuth() {

That nice porcelain ContainsAuth() function checks if either a credentials-store is configured or per-registry helpers, or contains credentials;

func (configFile *ConfigFile) ContainsAuth() bool {
return configFile.CredentialsStore != "" ||
len(configFile.CredentialHelpers) > 0 ||
len(configFile.AuthConfigs) > 0
}

I guess we could consider it an extra line of defence to never overwrite what's there, but definitely a bit iffy, and looks like it was there from the start moby/moby@cf721c2

if preferred := findPreferredHelper(); preferred != "" {
return preferred
}
if defaultHelper == "" {
return ""
}
if _, err := exec.LookPath(remoteCredentialsPrefix + defaultHelper); err != nil {
return ""
}
return defaultHelper
}
Comment on lines +19 to +29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we testing for defaultHelper's existence, but not preferred's?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I see that the test for the preferred helper is inside findPreferredHelper. I find this a bit hard to read, maybe because it mixes levels of abstraction. Maybe instead we could do something like

Suggested change
if preferred := findPreferredHelper(); preferred != "" {
return preferred
}
if defaultHelper == "" {
return ""
}
if _, err := exec.LookPath(remoteCredentialsPrefix + defaultHelper); err != nil {
return ""
}
return defaultHelper
}
if preferred := findPreferredHelper(); preferred != "" {
return preferred
}
return findDefaultHelper()
}
func findDefaultHelper() string {
if defaultHelper == "" {
return ""
}
if _, err := exec.LookPath(remoteCredentialsPrefix + defaultHelper); err != nil {
return ""
}
return defaultHelper
}


// overridePreferred is used to override the preferred helper in tests.
var overridePreferred string

platformDefault := defaultCredentialsStore()
if platformDefault == "" {
// 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.
Comment on lines +52 to +55
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "detection" logic below is generic, right? It's only used for linux (because linux is the only platform with a preferred helper), but would work for any other platform all the same?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// a "preferred" and "default" helper. This logic may change if a similar
// order of preference is needed on other platforms.

If this comment as a whole is about the preferred vs default helper order of preference, then I'd suggest placing it above, inside DetectDefaultStore near the operations regarding both the preferred and default helpers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "detection" logic below is generic, right? It's only used for linux (because linux is the only platform with a preferred helper), but would work for any other platform all the same?

AAAHH, this is about checking for both pass and docker-credential-pass. I wonder if we can make this clearer. Maybe separate this comment from the comment about the order (preferred vs default)? It was somewhat hard to figure out what this comment was about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize you moved this here in order to make this easier to test cross-platform, but this is exactly the type of thing that would make sense to put in a _linux.go file and not leak to the rest of the code – otherwise, we need to have it here with a number of comments explaining why, while it's unnecessary for anyone worrying about any other platform to look at).


// 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 ""
}

if _, err := exec.LookPath(remoteCredentialsPrefix + platformDefault); err != nil {
// 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 platformDefault

return preferred
}
7 changes: 4 additions & 3 deletions cli/config/credentials/default_store_darwin.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package credentials

func defaultCredentialsStore() string {
return "osxkeychain"
}
const (
preferredHelper = ""
defaultHelper = "osxkeychain"
)
13 changes: 3 additions & 10 deletions cli/config/credentials/default_store_linux.go
Original file line number Diff line number Diff line change
@@ -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"
}
74 changes: 74 additions & 0 deletions cli/config/credentials/default_store_test.go
Original file line number Diff line number Diff line change
@@ -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))
})
}
7 changes: 4 additions & 3 deletions cli/config/credentials/default_store_unsupported.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

package credentials

func defaultCredentialsStore() string {
return ""
}
const (
preferredHelper = ""
defaultHelper = ""
)
7 changes: 4 additions & 3 deletions cli/config/credentials/default_store_windows.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package credentials

func defaultCredentialsStore() string {
return "wincred"
}
const (
preferredHelper = ""
defaultHelper = "wincred"
)
Loading