From 8845ccd60f67a22ff9cec9d8ea68bc1e59030f95 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 Apr 2025 16:37:08 +0200 Subject: [PATCH 1/2] cli/command/registry: login: add unit test for flag validation Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login_test.go | 49 ++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index b12b5b6623..b745e06169 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "path/filepath" "testing" "time" @@ -539,3 +540,51 @@ func TestIsOauthLoginDisabled(t *testing.T) { assert.Equal(t, disabled, tc.disabled) } } + +func TestLoginValidateFlags(t *testing.T) { + for _, tc := range []struct { + name string + args []string + expectedErr string + }{ + { + name: "--password-stdin without --username", + args: []string{"--password-stdin"}, + expectedErr: `Must provide --username with --password-stdin`, + }, + { + name: "--password-stdin with empty --username", + args: []string{"--password-stdin", "--username", ""}, + expectedErr: `Must provide --username with --password-stdin`, + }, + { + name: "--username without value", + args: []string{"--username"}, + expectedErr: `flag needs an argument: --username`, + }, + { + name: "conflicting options --password-stdin and --password", + args: []string{"--password-stdin", "--password", ""}, + expectedErr: `Must provide --username with --password-stdin`, + }, + { + name: "--password without value", + args: []string{"--password"}, + expectedErr: `flag needs an argument: --password`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + cmd := NewLoginCommand(test.NewFakeCli(&fakeClient{})) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs(tc.args) + + err := cmd.Execute() + if tc.expectedErr != "" { + assert.Check(t, is.ErrorContains(err, tc.expectedErr)) + } else { + assert.Check(t, is.Nil(err)) + } + }) + } +} From e7af1812cfbfc585feba9687e3dffaac22699442 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 25 Apr 2025 17:11:05 +0200 Subject: [PATCH 2/2] cli/command/registry: login: improve flag validation Before this change, some errors could be ambiguous as they did not distinguish a flag to be omitted, or set, but with an empty value. For example, if a user would try to loging but with an empty username, the error would suggest that the `--username` flag was not set (which it was); I don't have `MY_USERNAME` set in this shell; printenv MY_USERNAME || echo 'variable not set' variable not set Now, attempting to do a non-interactive login would result in an ambiguous error; echo "supersecret" | docker login --password-stdin --username "$MY_USERNAME" Must provide --username with --password-stdin With this patch applied, the error indicates that the username was empty, or not set; echo "supersecret" | docker login --password-stdin --username "$MY_USERNAME" username is empty echo "supersecret" | docker login --password-stdin the --password-stdin option requires --username to be set echo "supersecret" | docker login --password-stdin --password "supersecret" conflicting options: cannot specify both --password and --password-stdin Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login.go | 32 +++++++++++++++++++++++++----- cli/command/registry/login_test.go | 16 ++++++++++++--- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index 701978c1cd..c33de68b5f 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -21,6 +21,7 @@ import ( "github.com/docker/docker/registry" "github.com/pkg/errors" "github.com/spf13/cobra" + "github.com/spf13/pflag" ) type loginOptions struct { @@ -43,6 +44,9 @@ func NewLoginCommand(dockerCLI command.Cli) *cobra.Command { if len(args) > 0 { opts.serverAddress = args[0] } + if err := verifyLoginFlags(cmd.Flags(), opts); err != nil { + return err + } return runLogin(cmd.Context(), dockerCLI, opts) }, Annotations: map[string]string{ @@ -60,17 +64,35 @@ func NewLoginCommand(dockerCLI command.Cli) *cobra.Command { return cmd } -func verifyLoginOptions(dockerCLI command.Cli, opts *loginOptions) error { +// verifyLoginFlags validates flags set on the command. +// +// TODO(thaJeztah); combine with verifyLoginOptions, but this requires rewrites of many tests. +func verifyLoginFlags(flags *pflag.FlagSet, opts loginOptions) error { + if flags.Changed("password-stdin") { + if flags.Changed("password") { + return errors.New("conflicting options: cannot specify both --password and --password-stdin") + } + if !flags.Changed("username") { + return errors.New("the --password-stdin option requires --username to be set") + } + } + if flags.Changed("username") && opts.user == "" { + return errors.New("username is empty") + } + if flags.Changed("password") && opts.password == "" { + return errors.New("password is empty") + } + return nil +} + +func verifyLoginOptions(dockerCLI command.Streams, opts *loginOptions) error { if opts.password != "" { _, _ = fmt.Fprintln(dockerCLI.Err(), "WARNING! Using --password via the CLI is insecure. Use --password-stdin.") - if opts.passwordStdin { - return errors.New("--password and --password-stdin are mutually exclusive") - } } if opts.passwordStdin { if opts.user == "" { - return errors.New("Must provide --username with --password-stdin") + return errors.New("username is empty") } contents, err := io.ReadAll(dockerCLI.In()) diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index b745e06169..58ca73c6a6 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -550,12 +550,17 @@ func TestLoginValidateFlags(t *testing.T) { { name: "--password-stdin without --username", args: []string{"--password-stdin"}, - expectedErr: `Must provide --username with --password-stdin`, + expectedErr: `the --password-stdin option requires --username to be set`, }, { name: "--password-stdin with empty --username", args: []string{"--password-stdin", "--username", ""}, - expectedErr: `Must provide --username with --password-stdin`, + expectedErr: `username is empty`, + }, + { + name: "empty --username", + args: []string{"--username", ""}, + expectedErr: `username is empty`, }, { name: "--username without value", @@ -565,7 +570,12 @@ func TestLoginValidateFlags(t *testing.T) { { name: "conflicting options --password-stdin and --password", args: []string{"--password-stdin", "--password", ""}, - expectedErr: `Must provide --username with --password-stdin`, + expectedErr: `conflicting options: cannot specify both --password and --password-stdin`, + }, + { + name: "empty --password", + args: []string{"--password", ""}, + expectedErr: `password is empty`, }, { name: "--password without value",