From 4c9fc240edfcadabf140807cbdd418306f7bd202 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 28 May 2023 12:09:16 +0200 Subject: [PATCH 1/9] client: use os/exec/Cmd.Environ() instead of os.Environ() Don't set Env if not set; the default is already handled if it's nil; from the documentation: https://pkg.go.dev/os/exec@go1.20.4#Cmd.Env // If Env is nil, the new process uses the current process's // environment. Use `os/exec/Cmd.Environ()` instead of `os.Environ()`, which was added in go1.19, and handles additional environment variables, such as `PWD` on POSIX systems, and `SYSTEMROOT` on Windows. https://pkg.go.dev/os/exec@go1.20.4#Cmd.Environ Also remove a redundant `fmt.Sprintf()`, as we're only concatenating strings. Signed-off-by: Sebastiaan van Stijn --- client/command.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/client/command.go b/client/command.go index 8da3343..0794d31 100644 --- a/client/command.go +++ b/client/command.go @@ -1,7 +1,6 @@ package client import ( - "fmt" "io" "os" "os/exec" @@ -30,10 +29,9 @@ func NewShellProgramFuncWithEnv(name string, env *map[string]string) ProgramFunc func createProgramCmdRedirectErr(commandName string, args []string, env *map[string]string) *exec.Cmd { programCmd := exec.Command(commandName, args...) - programCmd.Env = os.Environ() if env != nil { for k, v := range *env { - programCmd.Env = append(programCmd.Env, fmt.Sprintf("%s=%s", k, v)) + programCmd.Env = append(programCmd.Environ(), k+"="+v) } } programCmd.Stderr = os.Stderr From 94483d2d23b3794f6bcadb1e16f04c67fd61f2ad Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 28 May 2023 12:13:49 +0200 Subject: [PATCH 2/9] godoc: credentials helper -> credentials-helper Signed-off-by: Sebastiaan van Stijn --- client/client_test.go | 6 +++--- client/command.go | 6 +++--- credentials/credentials.go | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index 322d7f0..a7fad8b 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -21,14 +21,14 @@ const ( var errProgramExited = fmt.Errorf("exited 1") // mockProgram simulates interactions between the docker client and a remote -// credentials helper. +// credentials-helper. // Unit tests inject this mocked command into the remote to control execution. type mockProgram struct { arg string input io.Reader } -// Output returns responses from the remote credentials helper. +// Output returns responses from the remote credentials-helper. // It mocks those responses based in the input in the mock. func (m *mockProgram) Output() ([]byte, error) { in, err := io.ReadAll(m.input) @@ -80,7 +80,7 @@ func (m *mockProgram) Output() ([]byte, error) { return []byte(fmt.Sprintf("unknown argument %q with %q", m.arg, inS)), errProgramExited } -// Input sets the input to send to a remote credentials helper. +// Input sets the input to send to a remote credentials-helper. func (m *mockProgram) Input(in io.Reader) { m.input = in } diff --git a/client/command.go b/client/command.go index 0794d31..1936234 100644 --- a/client/command.go +++ b/client/command.go @@ -38,17 +38,17 @@ func createProgramCmdRedirectErr(commandName string, args []string, env *map[str return programCmd } -// Shell invokes shell commands to talk with a remote credentials helper. +// Shell invokes shell commands to talk with a remote credentials-helper. type Shell struct { cmd *exec.Cmd } -// Output returns responses from the remote credentials helper. +// Output returns responses from the remote credentials-helper. func (s *Shell) Output() ([]byte, error) { return s.cmd.Output() } -// Input sets the input to send to a remote credentials helper. +// Input sets the input to send to a remote credentials-helper. func (s *Shell) Input(in io.Reader) { s.cmd.Stdin = in } diff --git a/credentials/credentials.go b/credentials/credentials.go index 91d9d4b..7bb0495 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -43,7 +43,7 @@ func SetCredsLabel(label string) { CredsLabel = label } -// Serve initializes the credentials helper and parses the action argument. +// Serve initializes the credentials-helper and parses the action argument. // This function is designed to be called from a command line interface. // It uses os.Args[1] as the key for the action. // It uses os.Stdin as input and os.Stdout as output. From 19557f8fff7e15c7bfec205c148ae4810207fa41 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 28 May 2023 12:33:58 +0200 Subject: [PATCH 3/9] fix some errCheck warnings, and update examples - Explicitly suppress some unhandled errors - Use "pass" credentials helper in examples, which is available on more platforms than "secretservice" (only supporte on Linux) - Update domain and username in examples. Signed-off-by: Sebastiaan van Stijn --- client/client_test.go | 22 +++++++++++----------- credentials/credentials.go | 6 +++--- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/client/client_test.go b/client/client_test.go index a7fad8b..ebb2a6b 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -92,16 +92,16 @@ func mockProgramFn(args ...string) Program { } func ExampleStore() { - p := NewShellProgramFunc("docker-credential-secretservice") + p := NewShellProgramFunc("docker-credential-pass") c := &credentials.Credentials{ - ServerURL: "https://example.com", - Username: "calavera", + ServerURL: "https://registry.example.com", + Username: "exampleuser", Secret: "my super secret token", } if err := Store(p, c); err != nil { - fmt.Println(err) + _, _ = fmt.Println(err) } } @@ -129,14 +129,14 @@ func TestStore(t *testing.T) { } func ExampleGet() { - p := NewShellProgramFunc("docker-credential-secretservice") + p := NewShellProgramFunc("docker-credential-pass") - creds, err := Get(p, "https://example.com") + creds, err := Get(p, "https://registry.example.com") if err != nil { - fmt.Println(err) + _, _ = fmt.Println(err) } - fmt.Printf("Got credentials for user `%s` in `%s`\n", creds.Username, creds.ServerURL) + _, _ = fmt.Printf("Got credentials for user `%s` in `%s`\n", creds.Username, creds.ServerURL) } func TestGet(t *testing.T) { @@ -190,10 +190,10 @@ func TestGet(t *testing.T) { } func ExampleErase() { - p := NewShellProgramFunc("docker-credential-secretservice") + p := NewShellProgramFunc("docker-credential-pass") - if err := Erase(p, "https://example.com"); err != nil { - fmt.Println(err) + if err := Erase(p, "https://registry.example.com"); err != nil { + _, _ = fmt.Println(err) } } diff --git a/credentials/credentials.go b/credentials/credentials.go index 7bb0495..078f224 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -59,7 +59,7 @@ func Serve(helper Helper) { } if err != nil { - fmt.Fprintf(os.Stdout, "%v\n", err) + _, _ = fmt.Fprintln(os.Stdout, err) os.Exit(1) } } @@ -143,7 +143,7 @@ func Get(helper Helper, reader io.Reader, writer io.Writer) error { return err } - fmt.Fprint(writer, buffer.String()) + _, _ = fmt.Fprint(writer, buffer.String()) return nil } @@ -181,6 +181,6 @@ func List(helper Helper, writer io.Writer) error { // PrintVersion outputs the current version. func PrintVersion(writer io.Writer) error { - fmt.Fprintf(writer, "%s (%s) %s\n", Name, Package, Version) + _, _ = fmt.Fprintf(writer, "%s (%s) %s\n", Name, Package, Version) return nil } From c324fe0a6fd1e99ebe26974ec84b172ccda79181 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 28 May 2023 12:38:05 +0200 Subject: [PATCH 4/9] credentials: Get(): remove intermediate variable Signed-off-by: Sebastiaan van Stijn --- credentials/credentials.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/credentials/credentials.go b/credentials/credentials.go index 078f224..c2bcfb7 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -132,14 +132,13 @@ func Get(helper Helper, reader io.Reader, writer io.Writer) error { return err } - resp := Credentials{ + buffer.Reset() + err = json.NewEncoder(buffer).Encode(Credentials{ ServerURL: serverURL, Username: username, Secret: secret, - } - - buffer.Reset() - if err := json.NewEncoder(buffer).Encode(resp); err != nil { + }) + if err != nil { return err } From 0dbcdb66a707dbfb2f4970e153d405e011e18fe8 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 28 May 2023 13:00:22 +0200 Subject: [PATCH 5/9] credentials: Serve(): use "Name instead of "os.Args[0]" for usage output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GNU guidelines describes; https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion The program’s name should be a constant string; don’t compute it from argv[0]. The idea is to state the standard or canonical name for the program, not its file name. Although the above recommendation is for `--version` output, it probably makes sense to do the same for the "usage" output. Before this change: /usr/local/bin/docker-credential-osxkeychain invalid command Usage: /usr/local/bin/docker-credential-osxkeychain /Applications/Docker.app/Contents/Resources/bin/docker-credential-osxkeychain invalid command Usage: /Applications/Docker.app/Contents/Resources/bin/docker-credential-osxkeychain With this patch: /usr/local/bin/docker-credential-osxkeychain invalid command Usage: docker-credential-osxkeychain /Applications/Docker.app/Contents/Resources/bin/docker-credential-osxkeychain invalid command Usage: docker-credential-osxkeychain Signed-off-by: Sebastiaan van Stijn --- credentials/credentials.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/credentials/credentials.go b/credentials/credentials.go index c2bcfb7..11da850 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -51,7 +51,7 @@ func SetCredsLabel(label string) { func Serve(helper Helper) { var err error if len(os.Args) != 2 { - err = fmt.Errorf("Usage: %s ", os.Args[0]) + err = fmt.Errorf("Usage: %s ", Name) } if err == nil { From db0ac44c97c71bcd536c3a7df9f8791792ef22b1 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 28 May 2023 13:19:24 +0200 Subject: [PATCH 6/9] credentials: Serve(): simplify error-handling logic Don't use an err if we can print the error immediately :) Signed-off-by: Sebastiaan van Stijn --- credentials/credentials.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/credentials/credentials.go b/credentials/credentials.go index 11da850..92a567b 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -49,21 +49,21 @@ func SetCredsLabel(label string) { // It uses os.Stdin as input and os.Stdout as output. // This function terminates the program with os.Exit(1) if there is an error. func Serve(helper Helper) { - var err error if len(os.Args) != 2 { - err = fmt.Errorf("Usage: %s ", Name) + _, _ = fmt.Fprintln(os.Stdout, usage()) + os.Exit(1) } - if err == nil { - err = HandleCommand(helper, os.Args[1], os.Stdin, os.Stdout) - } - - if err != nil { + if err := HandleCommand(helper, os.Args[1], os.Stdin, os.Stdout); err != nil { _, _ = fmt.Fprintln(os.Stdout, err) os.Exit(1) } } +func usage() string { + return fmt.Sprintf("Usage: %s ", Name) +} + // HandleCommand uses a helper and a key to run a credential action. func HandleCommand(helper Helper, key string, in io.Reader, out io.Writer) error { switch key { From ae1d1ec0137667ce817f7c662f20a407e35bc641 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 28 May 2023 13:32:34 +0200 Subject: [PATCH 7/9] credentials: HandleCommand(): improve error for unknown command/action - renamed the "key" variable, which was slightly confusing - include the name of the binary in the error Before this change: docker-credential-osxkeychain nosuchaction Unknown credential action `nosuchaction` After this change: docker-credential-osxkeychain nosuchaction docker-credential-osxkeychain: unknown action: nosuchaction Signed-off-by: Sebastiaan van Stijn --- credentials/credentials.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/credentials/credentials.go b/credentials/credentials.go index 92a567b..6155e49 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -64,9 +64,9 @@ func usage() string { return fmt.Sprintf("Usage: %s ", Name) } -// HandleCommand uses a helper and a key to run a credential action. -func HandleCommand(helper Helper, key string, in io.Reader, out io.Writer) error { - switch key { +// HandleCommand runs a helper to execute a credential action. +func HandleCommand(helper Helper, action string, in io.Reader, out io.Writer) error { + switch action { case "store": return Store(helper, in) case "get": @@ -77,8 +77,9 @@ func HandleCommand(helper Helper, key string, in io.Reader, out io.Writer) error return List(helper, out) case "version": return PrintVersion(out) + default: + return fmt.Errorf("%s: unknown action: %s", Name, action) } - return fmt.Errorf("Unknown credential action `%s`", key) } // Store uses a helper and an input reader to save credentials. From 99079cafd2c145877a530ffc3748d44937adf467 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 28 May 2023 13:41:23 +0200 Subject: [PATCH 8/9] credentials: Serve(): implement "--version, -v", and "--help, -h" flags As recommended in the GNU documentation; - https://www.gnu.org/prep/standards/standards.html#g_t_002d_002dversion - https://www.gnu.org/prep/standards/standards.html#g_t_002d_002dhelp With this patch: $ docker-credential-osxkeychain --version docker-credential-osxkeychain (github.com/docker/docker-credential-helpers) v0.7.0-51-g26c426e.m $ docker-credential-osxkeychain -v docker-credential-osxkeychain (github.com/docker/docker-credential-helpers) v0.7.0-51-g26c426e.m $ docker-credential-osxkeychain --help Usage: docker-credential-osxkeychain $ docker-credential-osxkeychain -h Usage: docker-credential-osxkeychain Signed-off-by: Sebastiaan van Stijn --- credentials/credentials.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/credentials/credentials.go b/credentials/credentials.go index 6155e49..b3acd83 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -54,6 +54,15 @@ func Serve(helper Helper) { os.Exit(1) } + switch os.Args[1] { + case "--version", "-v": + _ = PrintVersion(os.Stdout) + os.Exit(0) + case "--help", "-h": + _, _ = fmt.Fprintln(os.Stdout, usage()) + os.Exit(0) + } + if err := HandleCommand(helper, os.Args[1], os.Stdin, os.Stdout); err != nil { _, _ = fmt.Fprintln(os.Stdout, err) os.Exit(1) From 129017a3cdb99cd8190c525a75d4da6e1d8a9506 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 28 May 2023 13:47:57 +0200 Subject: [PATCH 9/9] credentials: define consts for supported actions (sub-commands) Signed-off-by: Sebastiaan van Stijn --- client/client.go | 8 ++++---- credentials/credentials.go | 26 ++++++++++++++++++++------ 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/client/client.go b/client/client.go index d1d0434..678153c 100644 --- a/client/client.go +++ b/client/client.go @@ -26,7 +26,7 @@ func isValidCredsMessage(msg string) error { // Store uses an external program to save credentials. func Store(program ProgramFunc, creds *credentials.Credentials) error { - cmd := program("store") + cmd := program(credentials.ActionStore) buffer := new(bytes.Buffer) if err := json.NewEncoder(buffer).Encode(creds); err != nil { @@ -50,7 +50,7 @@ func Store(program ProgramFunc, creds *credentials.Credentials) error { // Get executes an external program to get the credentials from a native store. func Get(program ProgramFunc, serverURL string) (*credentials.Credentials, error) { - cmd := program("get") + cmd := program(credentials.ActionGet) cmd.Input(strings.NewReader(serverURL)) out, err := cmd.Output() @@ -81,7 +81,7 @@ func Get(program ProgramFunc, serverURL string) (*credentials.Credentials, error // Erase executes a program to remove the server credentials from the native store. func Erase(program ProgramFunc, serverURL string) error { - cmd := program("erase") + cmd := program(credentials.ActionErase) cmd.Input(strings.NewReader(serverURL)) out, err := cmd.Output() if err != nil { @@ -99,7 +99,7 @@ func Erase(program ProgramFunc, serverURL string) error { // List executes a program to list server credentials in the native store. func List(program ProgramFunc) (map[string]string, error) { - cmd := program("list") + cmd := program(credentials.ActionList) cmd.Input(strings.NewReader("unused")) out, err := cmd.Output() if err != nil { diff --git a/credentials/credentials.go b/credentials/credentials.go index b3acd83..eac5518 100644 --- a/credentials/credentials.go +++ b/credentials/credentials.go @@ -10,6 +10,20 @@ import ( "strings" ) +// Action defines the name of an action (sub-command) supported by a +// credential-helper binary. It is an alias for "string", and mostly +// for convenience. +type Action = string + +// List of actions (sub-commands) supported by credential-helper binaries. +const ( + ActionStore Action = "store" + ActionGet Action = "get" + ActionErase Action = "erase" + ActionList Action = "list" + ActionVersion Action = "version" +) + // Credentials holds the information shared between docker and the credentials store. type Credentials struct { ServerURL string @@ -74,17 +88,17 @@ func usage() string { } // HandleCommand runs a helper to execute a credential action. -func HandleCommand(helper Helper, action string, in io.Reader, out io.Writer) error { +func HandleCommand(helper Helper, action Action, in io.Reader, out io.Writer) error { switch action { - case "store": + case ActionStore: return Store(helper, in) - case "get": + case ActionGet: return Get(helper, in, out) - case "erase": + case ActionErase: return Erase(helper, in) - case "list": + case ActionList: return List(helper, out) - case "version": + case ActionVersion: return PrintVersion(out) default: return fmt.Errorf("%s: unknown action: %s", Name, action)