From 91bd66ae0468773047e7839c49f060929c4fbb1a Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Tue, 3 Sep 2024 20:47:38 +0100 Subject: [PATCH] chore: improve errors Improve the reported errors so that consumers can understand why failure occurs. This is vital for credential lookup failures, which is impossible to debug otherwise, for example permission error when using gcloud as credentials helper as identified debugging an issue in testcontainers. Eliminate wrapping errors with "error ..." as that results in messages which are hard to understand in a multi wrap. Add tests to cover credentials helper, ensuring that command handling behaves correctly. Modernise error checks to use errors.Is. Leverage doc comment reference linking instead of backticks. Use Auth fallback if credentials store reports not found. Remove impossible decode length check and ensure decoded data is sized correctly, eliminating need for \x00 trim. Add github action to run test and lint. Fix from base64 auth tests not running due to missing parameter. Fix some comment typos. --- .github/workflows/go.yaml | 39 ++++++++ auth.go | 127 ++++++++++++++++---------- auth_test.go | 182 +++++++++++++++++++++++++++++++++++++- config.go | 10 +-- load.go | 22 +++-- testdata/config.json | 17 ++++ 6 files changed, 332 insertions(+), 65 deletions(-) create mode 100644 .github/workflows/go.yaml create mode 100644 testdata/config.json diff --git a/.github/workflows/go.yaml b/.github/workflows/go.yaml new file mode 100644 index 0000000..ee05373 --- /dev/null +++ b/.github/workflows/go.yaml @@ -0,0 +1,39 @@ +# This workflow will build a golang project +# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-go + +name: CI + +on: + push: + branches: ["main"] + pull_request: + branches: ["main"] + +env: + GO_VERSION: "1.23" + GOLANGCI_VERSION: "v1.59.1" + +jobs: + lint-test-go: + runs-on: ubuntu-latest + steps: + - name: Check out code + uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version: ${{ env.GO_VERSION }} + + - name: Validate go mod tidy + run: | + go mod tidy + git --no-pager diff && [[ 0 -eq $(git status --porcelain | wc -l) ]] + + - name: Go Lint + uses: golangci/golangci-lint-action@v6 + with: + version: ${{ env.GOLANGCI_VERSION }} + args: --out-format=colored-line-number + + - name: Test + run: go test -race ./... diff --git a/auth.go b/auth.go index 5a9891b..106ab84 100644 --- a/auth.go +++ b/auth.go @@ -1,40 +1,43 @@ package dockercfg import ( + "bytes" "encoding/base64" "encoding/json" "errors" "fmt" - "os" + "io/fs" "os/exec" "runtime" "strings" ) -// This is used by the docker CLI in casses where an oauth identity token is used. -// In that case the username is stored litterally as `` -// When fetching the credentials we check for this value to determine if +// This is used by the docker CLI in cases where an oauth identity token is used. +// In that case the username is stored literally as `` +// When fetching the credentials we check for this value to determine if. const tokenUsername = "" // GetRegistryCredentials gets registry credentials for the passed in registry host. // -// This will use `LoadDefaultConfig` to read registry auth details from the config. +// This will use [LoadDefaultConfig] to read registry auth details from the config. // If the config doesn't exist, it will attempt to load registry credentials using the default credential helper for the platform. func GetRegistryCredentials(hostname string) (string, string, error) { cfg, err := LoadDefaultConfig() if err != nil { - if !os.IsNotExist(err) { - return "", "", err + if !errors.Is(err, fs.ErrNotExist) { + return "", "", fmt.Errorf("load default config: %w", err) } + return GetCredentialsFromHelper("", hostname) } + return cfg.GetRegistryCredentials(hostname) } // ResolveRegistryHost can be used to transform a docker registry host name into what is used for the docker config/cred helpers // // This is useful for using with containerd authorizers. -// Natrually this only transforms docker hub URLs. +// Naturally this only transforms docker hub URLs. func ResolveRegistryHost(host string) string { switch host { case "index.docker.io", "docker.io", "https://index.docker.io/v1/", "registry-1.docker.io": @@ -43,9 +46,9 @@ func ResolveRegistryHost(host string) string { return host } -// GetRegistryCredentials gets credentials, if any, for the provided hostname +// GetRegistryCredentials gets credentials, if any, for the provided hostname. // -// Hostnames should already be resolved using `ResolveRegistryAuth` +// Hostnames should already be resolved using [ResolveRegistryHost]. // // If the returned username string is empty, the password is an identity token. func (c *Config) GetRegistryCredentials(hostname string) (string, string, error) { @@ -55,7 +58,14 @@ func (c *Config) GetRegistryCredentials(hostname string) (string, string, error) } if c.CredentialsStore != "" { - return GetCredentialsFromHelper(c.CredentialsStore, hostname) + username, password, err := GetCredentialsFromHelper(c.CredentialsStore, hostname) + if err != nil { + return "", "", fmt.Errorf("get credentials from store: %w", err) + } + + if username != "" || password != "" { + return username, password, nil + } } auth, ok := c.AuthConfigs[hostname] @@ -87,78 +97,96 @@ func DecodeBase64Auth(auth AuthConfig) (string, string, error) { decoded := make([]byte, decLen) n, err := base64.StdEncoding.Decode(decoded, []byte(auth.Auth)) if err != nil { - return "", "", fmt.Errorf("error decoding auth from file: %w", err) + return "", "", fmt.Errorf("decode auth: %w", err) } - if n > decLen { - return "", "", fmt.Errorf("decoded value is longer than expected length, expected: %d, actual: %d", decLen, n) - } + decoded = decoded[:n] - split := strings.SplitN(string(decoded), ":", 2) - if len(split) != 2 { - return "", "", errors.New("invalid auth string") + const sep = ":" + user, pass, found := strings.Cut(string(decoded), sep) + if !found { + return "", "", fmt.Errorf("invalid auth: missing %q separator", sep) } - return split[0], strings.Trim(split[1], "\x00"), nil + return user, pass, nil } -// Errors from credential helpers +// Errors from credential helpers. var ( ErrCredentialsNotFound = errors.New("credentials not found in native keychain") ErrCredentialsMissingServerURL = errors.New("no credentials server URL") ) +//nolint:gochecknoglobals // These are used to mock exec in tests. +var ( + // execLookPath is a variable that can be used to mock exec.LookPath in tests. + execLookPath = exec.LookPath + // execCommand is a variable that can be used to mock exec.Command in tests. + execCommand = exec.Command +) + // GetCredentialsFromHelper attempts to lookup credentials from the passed in docker credential helper. // -// The credential helpoer should just be the suffix name (no "docker-credential-"). +// The credential helper should just be the suffix name (no "docker-credential-"). // If the passed in helper program is empty this will look up the default helper for the platform. // // If the credentials are not found, no error is returned, only empty credentials. // -// Hostnames should already be resolved using `ResolveRegistryAuth` +// Hostnames should already be resolved using [ResolveRegistryHost] // // If the username string is empty, the password string is an identity token. func GetCredentialsFromHelper(helper, hostname string) (string, string, error) { if helper == "" { - helper = getCredentialHelper() - } - if helper == "" { - return "", "", nil + helper, helperErr := getCredentialHelper() + if helperErr != nil { + return "", "", fmt.Errorf("get credential helper: %w", helperErr) + } + + if helper == "" { + return "", "", nil + } } - p, err := exec.LookPath("docker-credential-" + helper) + helper = "docker-credential-" + helper + p, err := execLookPath(helper) if err != nil { + if !errors.Is(err, exec.ErrNotFound) { + return "", "", fmt.Errorf("look up %q: %w", helper, err) + } + return "", "", nil } - cmd := exec.Command(p, "get") + var outBuf, errBuf bytes.Buffer + cmd := execCommand(p, "get") cmd.Stdin = strings.NewReader(hostname) + cmd.Stdout = &outBuf + cmd.Stderr = &errBuf - b, err := cmd.Output() - if err != nil { - s := strings.TrimSpace(string(b)) - - switch s { + if err = cmd.Run(); err != nil { + out := strings.TrimSpace(outBuf.String()) + switch out { case ErrCredentialsNotFound.Error(): return "", "", nil case ErrCredentialsMissingServerURL.Error(): - return "", "", errors.New(s) + return "", "", ErrCredentialsMissingServerURL default: + return "", "", fmt.Errorf("execute %q stdout: %q stderr: %q: %w", + helper, out, strings.TrimSpace(errBuf.String()), err, + ) } - - return "", "", err } var creds struct { - Username string - Secret string + Username string `json:"Username"` + Secret string `json:"Secret"` } - if err := json.Unmarshal(b, &creds); err != nil { - return "", "", err + if err = json.Unmarshal(outBuf.Bytes(), &creds); err != nil { + return "", "", fmt.Errorf("unmarshal credentials from: %q: %w", helper, err) } - // When tokenUsername is used, the output is an identity token and the username is garbage + // When tokenUsername is used, the output is an identity token and the username is garbage. if creds.Username == tokenUsername { creds.Username = "" } @@ -167,18 +195,21 @@ func GetCredentialsFromHelper(helper, hostname string) (string, string, error) { } // getCredentialHelper gets the default credential helper name for the current platform. -func getCredentialHelper() string { +func getCredentialHelper() (string, error) { switch runtime.GOOS { case "linux": - if _, err := exec.LookPath("pass"); err == nil { - return "pass" + if _, err := exec.LookPath("pass"); err != nil { + if errors.Is(err, exec.ErrNotFound) { + return "secretservice", nil + } + return "", fmt.Errorf(`look up "pass": %w`, err) } - return "secretservice" + return "pass", nil case "darwin": - return "osxkeychain" + return "osxkeychain", nil case "windows": - return "wincred" + return "wincred", nil default: - return "" + return "", nil } } diff --git a/auth_test.go b/auth_test.go index 2fb0937..6ad601d 100644 --- a/auth_test.go +++ b/auth_test.go @@ -2,6 +2,11 @@ package dockercfg import ( "encoding/base64" + "errors" + "io" + "os" + "os/exec" + "strconv" "testing" ) @@ -13,10 +18,10 @@ func TestDecodeBase64Auth(t *testing.T) { } } -func TestGetRegistryCredentials(t *testing.T) { +func TestConfig_GetRegistryCredentials(t *testing.T) { t.Run("from base64 auth", func(t *testing.T) { for _, tc := range base64TestCases() { - t.Run(tc.name, func(T *testing.T) { + t.Run(tc.name, func(t *testing.T) { config := Config{ AuthConfigs: map[string]AuthConfig{ "some.domain": tc.config, @@ -24,7 +29,7 @@ func TestGetRegistryCredentials(t *testing.T) { } testBase64Case(tc, func() (string, string, error) { return config.GetRegistryCredentials("some.domain") - }) + })(t) }) } }) @@ -57,6 +62,8 @@ type testAuthFn func() (string, string, error) func testBase64Case(tc base64TestCase, authFn testAuthFn) func(t *testing.T) { return func(t *testing.T) { + t.Helper() + u, p, err := authFn() if tc.expErr && err == nil { t.Fatal("expected error") @@ -67,3 +74,172 @@ func testBase64Case(tc base64TestCase, authFn testAuthFn) func(t *testing.T) { } } } + +// validateAuth is a helper function to validate the username and password for a given hostname. +func validateAuth(t *testing.T, hostname, expectedUser, expectedPass string) { + t.Helper() + + username, password, err := GetRegistryCredentials(hostname) + if err != nil { + t.Fatalf("get registry credentials: %v", err) + } + + if username != expectedUser { + t.Fatalf("expected username: %q, got username: %q", expectedUser, username) + } + + if password != expectedPass { + t.Fatalf("expected password: %q, got password: %q", expectedPass, password) + } +} + +// validateAuthError is a helper function to validate we get an error for the given hostname. +func validateAuthError(t *testing.T, hostname string, expectedErr error) { + t.Helper() + + username, password, err := GetRegistryCredentials(hostname) + if err == nil || err.Error() != expectedErr.Error() { + t.Fatalf("expected error: %q got: %v", expectedErr, err) + } + + if username != "" || password != "" { + t.Fatalf("expected empty username and password, got username: %q, password: %q", username, password) + } +} + +// mockExecCommand is a helper function to mock exec.LookPath and exec.Command for testing. +func mockExecCommand(t *testing.T, env ...string) { + t.Helper() + + execLookPath = func(file string) (string, error) { + switch file { + case "docker-credential-helper": + return os.Args[0], nil + case "docker-credential-error": + return "", errors.New("lookup error") + } + + return "", exec.ErrNotFound + } + + execCommand = func(name string, arg ...string) *exec.Cmd { + cmd := exec.Command(name, arg...) + cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1") + cmd.Env = append(cmd.Env, env...) + return cmd + } + + t.Cleanup(func() { + execLookPath = exec.LookPath + execCommand = exec.Command + }) +} + +func TestGetRegistryCredentials(t *testing.T) { + t.Setenv("DOCKER_CONFIG", "testdata") + + t.Run("auths/user-pass", func(t *testing.T) { + validateAuth(t, "userpass.io", "user", "pass") + }) + + t.Run("auths/auth", func(t *testing.T) { + validateAuth(t, "auth.io", "auth", "authsecret") + }) + + t.Run("credsStore", func(t *testing.T) { + validateAuth(t, "credstore.io", "", "") + }) + + t.Run("credHelpers/user-pass", func(t *testing.T) { + mockExecCommand(t, `HELPER_STDOUT={"Username":"credhelper","Secret":"credhelpersecret"}`) + validateAuth(t, "helper.io", "credhelper", "credhelpersecret") + }) + + t.Run("credHelpers/token", func(t *testing.T) { + mockExecCommand(t, `HELPER_STDOUT={"Username":"", "Secret":"credhelpersecret"}`) + validateAuth(t, "helper.io", "", "credhelpersecret") + }) + + t.Run("credHelpers/not-found", func(t *testing.T) { + mockExecCommand(t, "HELPER_STDOUT="+ErrCredentialsNotFound.Error(), "HELPER_EXIT_CODE=1") + validateAuth(t, "helper.io", "", "") + }) + + t.Run("credHelpers/missing-url", func(t *testing.T) { + mockExecCommand(t, "HELPER_STDOUT="+ErrCredentialsMissingServerURL.Error(), "HELPER_EXIT_CODE=1") + validateAuthError(t, "helper.io", ErrCredentialsMissingServerURL) + }) + + t.Run("credHelpers/other-error", func(t *testing.T) { + mockExecCommand(t, "HELPER_STDOUT=output", "HELPER_STDERR=my error", "HELPER_EXIT_CODE=10") + expectedErr := errors.New(`execute "docker-credential-helper" stdout: "output" stderr: "my error": exit status 10`) + validateAuthError(t, "helper.io", expectedErr) + }) + + t.Run("credHelpers/lookup-not-found", func(t *testing.T) { + mockExecCommand(t, "HELPER_STDOUT=output", "HELPER_STDERR=my error", "HELPER_EXIT_CODE=10") + validateAuth(t, "other.io", "", "") + }) + + t.Run("credHelpers/lookup-error", func(t *testing.T) { + mockExecCommand(t, "HELPER_STDOUT=output", "HELPER_STDERR=my error", "HELPER_EXIT_CODE=10") + expectedErr := errors.New(`look up "docker-credential-error": lookup error`) + validateAuthError(t, "error.io", expectedErr) + }) + + t.Run("credHelpers/decode-json", func(t *testing.T) { + mockExecCommand(t, "HELPER_STDOUT=bad-json") + expectedErr := errors.New(`unmarshal credentials from: "docker-credential-helper": invalid character 'b' looking for beginning of value`) + validateAuthError(t, "helper.io", expectedErr) + }) + + t.Run("config/not-found", func(t *testing.T) { + t.Setenv("DOCKER_CONFIG", "testdata/missing") + validateAuth(t, "userpass.io", "", "") + }) + + t.Run("config/invalid", func(t *testing.T) { + t.Setenv("DOCKER_CONFIG", "/dev/null") + expectedErr := errors.New("load default config: open config: open /dev/null/config.json: not a directory") + validateAuthError(t, "helper.io", expectedErr) + }) +} + +// TestMain is hijacked so we can run a test helper which can write +// cleanly to stdout and stderr. +func TestMain(m *testing.M) { + pid := os.Getpid() + if os.Getenv("GO_EXEC_TEST_PID") == "" { + os.Setenv("GO_EXEC_TEST_PID", strconv.Itoa(pid)) + // Run the tests. + os.Exit(m.Run()) + } + + // Run the helper which slurps stdin and writes to stdout and stderr. + if _, err := io.Copy(io.Discard, os.Stdin); err != nil { + if _, err = os.Stderr.WriteString(err.Error()); err != nil { + panic(err) + } + } + + if out := os.Getenv("HELPER_STDOUT"); out != "" { + if _, err := os.Stdout.WriteString(out); err != nil { + panic(err) + } + } + + if out := os.Getenv("HELPER_STDERR"); out != "" { + if _, err := os.Stderr.WriteString(out); err != nil { + panic(err) + } + } + + if code := os.Getenv("HELPER_EXIT_CODE"); code != "" { + code, err := strconv.Atoi(code) + if err != nil { + panic(err) + } + + os.Exit(code) + } +} diff --git a/config.go b/config.go index 88736ca..5e53907 100644 --- a/config.go +++ b/config.go @@ -13,7 +13,7 @@ type Config struct { DetachKeys string `json:"detachKeys,omitempty"` CredentialsStore string `json:"credsStore,omitempty"` CredentialHelpers map[string]string `json:"credHelpers,omitempty"` - Filename string `json:"-"` // Note: for internal use only + Filename string `json:"-"` // Note: for internal use only. ServiceInspectFormat string `json:"serviceInspectFormat,omitempty"` ServicesFormat string `json:"servicesFormat,omitempty"` TasksFormat string `json:"tasksFormat,omitempty"` @@ -30,7 +30,7 @@ type Config struct { Aliases map[string]string `json:"aliases,omitempty"` } -// ProxyConfig contains proxy configuration settings +// ProxyConfig contains proxy configuration settings. type ProxyConfig struct { HTTPProxy string `json:"httpProxy,omitempty"` HTTPSProxy string `json:"httpsProxy,omitempty"` @@ -38,7 +38,7 @@ type ProxyConfig struct { FTPProxy string `json:"ftpProxy,omitempty"` } -// AuthConfig contains authorization information for connecting to a Registry +// AuthConfig contains authorization information for connecting to a Registry. type AuthConfig struct { Username string `json:"username,omitempty"` Password string `json:"password,omitempty"` @@ -55,11 +55,11 @@ type AuthConfig struct { // an access token for the registry. IdentityToken string `json:"identitytoken,omitempty"` - // RegistryToken is a bearer token to be sent to a registry + // RegistryToken is a bearer token to be sent to a registry. RegistryToken string `json:"registrytoken,omitempty"` } -// KubernetesConfig contains Kubernetes orchestrator settings +// KubernetesConfig contains Kubernetes orchestrator settings. type KubernetesConfig struct { AllNamespaces string `json:"allNamespaces,omitempty"` } diff --git a/load.go b/load.go index 262545b..a1c4dca 100644 --- a/load.go +++ b/load.go @@ -11,7 +11,7 @@ import ( func UserHomeConfigPath() (string, error) { home, err := os.UserHomeDir() if err != nil { - return "", fmt.Errorf("error looking up user home dir: %w", err) + return "", fmt.Errorf("user home dir: %w", err) } return filepath.Join(home, ".docker", "config.json"), nil @@ -19,7 +19,7 @@ func UserHomeConfigPath() (string, error) { // ConfigPath returns the path to the docker cli config. // -// It will either use the DOCKER_CONFIG env var if set, or the value from `UserHomeConfigPath` +// It will either use the DOCKER_CONFIG env var if set, or the value from [UserHomeConfigPath] // DOCKER_CONFIG would be the dir path where `config.json` is stored, this returns the path to config.json. func ConfigPath() (string, error) { if p := os.Getenv("DOCKER_CONFIG"); p != "" { @@ -28,24 +28,28 @@ func ConfigPath() (string, error) { return UserHomeConfigPath() } -// LoadDefaultConfig loads the docker cli config from the path returned from `ConfigPath` +// LoadDefaultConfig loads the docker cli config from the path returned from [ConfigPath]. func LoadDefaultConfig() (Config, error) { var cfg Config p, err := ConfigPath() if err != nil { - return cfg, err + return cfg, fmt.Errorf("config path: %w", err) } + return cfg, FromFile(p, &cfg) } -// FromFile loads config from the specified path into cfg +// FromFile loads config from the specified path into cfg. func FromFile(configPath string, cfg *Config) error { f, err := os.Open(configPath) if err != nil { - return err + return fmt.Errorf("open config: %w", err) + } + defer f.Close() + + if err = json.NewDecoder(f).Decode(&cfg); err != nil { + return fmt.Errorf("decode config: %w", err) } - err = json.NewDecoder(f).Decode(&cfg) - f.Close() - return err + return nil } diff --git a/testdata/config.json b/testdata/config.json new file mode 100644 index 0000000..653a253 --- /dev/null +++ b/testdata/config.json @@ -0,0 +1,17 @@ +{ + "auths": { + "userpass.io": { + "username": "user", + "password": "pass" + }, + "auth.io": { + "auth": "YXV0aDphdXRoc2VjcmV0" + } + }, + "credHelpers": { + "helper.io": "helper", + "other.io": "other", + "error.io": "error" + }, + "credsStore": "desktop" + }