From d23ab524c320ff7ba9a2dd5ab2c4bb9c8534f7fe Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 19 Oct 2024 11:46:21 +0200 Subject: [PATCH 1/8] cli/command: PromptUserForCredentials: remove named output variables This function has multiple conditional branches, which makes it harder to see at a glance whether authConfig may be partially populated. This patch instead returns a fresh instance for error returns to prevent any confusion. It also removes the named output variables, as they're now no longer used, and the returned types should already be descriptive enough to understand what's returned. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 3a8485085d94c85dae8078ee36310baa69a21217) Signed-off-by: Sebastiaan van Stijn --- cli/command/registry.go | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/cli/command/registry.go b/cli/command/registry.go index 3ec07e801d..642667a11f 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -112,7 +112,7 @@ func ConfigureAuth(ctx context.Context, cli Cli, flUser, flPassword string, auth // If defaultUsername is not empty, the username prompt includes that username // and the user can hit enter without inputting a username to use that default // username. -func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword, defaultUsername, serverAddress string) (authConfig registrytypes.AuthConfig, err error) { +func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword, defaultUsername, serverAddress string) (registrytypes.AuthConfig, error) { // On Windows, force the use of the regular OS stdin stream. // // See: @@ -144,38 +144,41 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword } else { prompt = fmt.Sprintf("Username (%s): ", defaultUsername) } + + var err error argUser, err = PromptForInput(ctx, cli.In(), cli.Out(), prompt) if err != nil { - return authConfig, err + return registrytypes.AuthConfig{}, err } if argUser == "" { argUser = defaultUsername } } if argUser == "" { - return authConfig, errors.Errorf("Error: Non-null Username Required") + return registrytypes.AuthConfig{}, errors.Errorf("Error: Non-null Username Required") } if argPassword == "" { restoreInput, err := DisableInputEcho(cli.In()) if err != nil { - return authConfig, err + return registrytypes.AuthConfig{}, err } defer restoreInput() argPassword, err = PromptForInput(ctx, cli.In(), cli.Out(), "Password: ") if err != nil { - return authConfig, err + return registrytypes.AuthConfig{}, err } fmt.Fprint(cli.Out(), "\n") if argPassword == "" { - return authConfig, errors.Errorf("Error: Password Required") + return registrytypes.AuthConfig{}, errors.Errorf("Error: Password Required") } } - authConfig.Username = argUser - authConfig.Password = argPassword - authConfig.ServerAddress = serverAddress - return authConfig, nil + return registrytypes.AuthConfig{ + Username: argUser, + Password: argPassword, + ServerAddress: serverAddress, + }, nil } // RetrieveAuthTokenFromImage retrieves an encoded auth token given a complete From 05455f850573d649236b93835531e4db50c6c67d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 19 Oct 2024 11:58:42 +0200 Subject: [PATCH 2/8] cli/command: PromptUserForCredentials: inline isDefaultRegistry remove isDefaultRegistry and inline it where it's used; the code-comment already outlines what we're looking for, so the intermediate var didn't add much currently. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit a55cfe5f82a13105ca49ee70f72ff6ef00400917) Signed-off-by: Sebastiaan van Stijn --- cli/command/registry.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cli/command/registry.go b/cli/command/registry.go index 642667a11f..51fff10c5f 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -125,11 +125,10 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword cli.SetIn(streams.NewIn(os.Stdin)) } - isDefaultRegistry := serverAddress == registry.IndexServer defaultUsername = strings.TrimSpace(defaultUsername) if argUser = strings.TrimSpace(argUser); argUser == "" { - if isDefaultRegistry { + if serverAddress == registry.IndexServer { // if this is a default registry (docker hub), then display the following message. fmt.Fprintln(cli.Out(), "Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.") if hints.Enabled() { From 97c25b75748ee396ac6153c9447eaa47ad33cbc9 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 19 Oct 2024 12:01:25 +0200 Subject: [PATCH 3/8] cli/command: PromptUserForCredentials: move "post" check for empty name move the "post" check for username being empty inside the branch that's handling the username, as it's the only branch where username is mutated after checking if it's empty. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 581cf36bd4206b4cccf3cd1ad93162bbbffaad37) Signed-off-by: Sebastiaan van Stijn --- cli/command/registry.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cli/command/registry.go b/cli/command/registry.go index 51fff10c5f..11fad3fe1f 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -152,10 +152,11 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword if argUser == "" { argUser = defaultUsername } + if argUser == "" { + return registrytypes.AuthConfig{}, errors.Errorf("Error: Non-null Username Required") + } } - if argUser == "" { - return registrytypes.AuthConfig{}, errors.Errorf("Error: Non-null Username Required") - } + if argPassword == "" { restoreInput, err := DisableInputEcho(cli.In()) if err != nil { From 78dbcca264a4ce7f0b6f53559d6dff3d7e16e205 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 19 Oct 2024 12:07:51 +0200 Subject: [PATCH 4/8] cli/command: PromptUserForCredentials: move trimming where it's used - move trimming defaultUsername inside the if-branch, as it's the only location where the result of the trimmed username is use. - do the reverse for trimming argUser, because the result of trimming argUser is used outside of the if-branch (not just for the condition). putting it inside the condition makes it easy to assume the result is only used locally. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit eda78e9cdc79e4ec2d886e0a75c0abe06dd5489e) Signed-off-by: Sebastiaan van Stijn --- cli/command/registry.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cli/command/registry.go b/cli/command/registry.go index 11fad3fe1f..39e2e0ddb1 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -125,9 +125,8 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword cli.SetIn(streams.NewIn(os.Stdin)) } - defaultUsername = strings.TrimSpace(defaultUsername) - - if argUser = strings.TrimSpace(argUser); argUser == "" { + argUser = strings.TrimSpace(argUser) + if argUser == "" { if serverAddress == registry.IndexServer { // if this is a default registry (docker hub), then display the following message. fmt.Fprintln(cli.Out(), "Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.") @@ -138,6 +137,7 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword } var prompt string + defaultUsername = strings.TrimSpace(defaultUsername) if defaultUsername == "" { prompt = "Username: " } else { From 1377310110548af0639ddc1096949fb95d4ed8e4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 19 Oct 2024 12:10:46 +0200 Subject: [PATCH 5/8] cli/command: PromptUserForCredentials: always trim password we don't support empty passwords; when prompting the user for a password, we already trim the result, but we didn't do the same for a password that's passed through stdin or through the `-p` / `--password` flag. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit a21a5f42433267052441cdb7ebe7604dfcc7f159) Signed-off-by: Sebastiaan van Stijn --- cli/command/registry.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/command/registry.go b/cli/command/registry.go index 39e2e0ddb1..177c03cbb5 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -157,6 +157,7 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword } } + argPassword = strings.TrimSpace(argPassword) if argPassword == "" { restoreInput, err := DisableInputEcho(cli.In()) if err != nil { From da3a1c3027058d32e06ac462de09e1c79b0fabe5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 19 Oct 2024 12:49:44 +0200 Subject: [PATCH 6/8] cli/command: PromptUserForCredentials: print error on terminal restore fail If restoring the terminal state fails, "echo" no longer works, which means that anything the user types is no longer shown. The login itself may already have succeeded, so we should not fail the command, but it's good to inform the user that this happened, which may give them a clue why things no longer work as they expect them to work. With this patch: docker login -u yourname Password: Error: failed to restore terminal state to echo input: something bad happened Login Succeeded We should consider printing instructions how to restore this manually (other than restarting the shell). e.g., 'run stty echo' when in a Linux or macOS shell, but PowerShell and CMD.exe may need different instructions. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 3d8b49523d565867ee7a6f3e0a7c795b5aec4e79) Signed-off-by: Sebastiaan van Stijn --- cli/command/registry.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cli/command/registry.go b/cli/command/registry.go index 177c03cbb5..a9c19b8c71 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -163,7 +163,15 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword if err != nil { return registrytypes.AuthConfig{}, err } - defer restoreInput() + defer func() { + if err := restoreInput(); err != nil { + // TODO(thaJeztah): we should consider printing instructions how + // to restore this manually (other than restarting the shell). + // e.g., 'run stty echo' when in a Linux or macOS shell, but + // PowerShell and CMD.exe may need different instructions. + _, _ = fmt.Fprintln(cli.Err(), "Error: failed to restore terminal state to echo input:", err) + } + }() argPassword, err = PromptForInput(ctx, cli.In(), cli.Out(), "Password: ") if err != nil { From ddeb7eb4edc5d22154da5b5bdb9c074611fcefc3 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 19 Oct 2024 13:23:29 +0200 Subject: [PATCH 7/8] cli/command: PromptUserForCredentials: use consts for all hints This message resulted in code-lines that were too long; move it to a const together with the other hint. While at it, also suppress unhandled error, and touch-up the code-comment. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 378a3d7d3608308aa8ac01a954497916e0110653) Signed-off-by: Sebastiaan van Stijn --- cli/command/registry.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/cli/command/registry.go b/cli/command/registry.go index a9c19b8c71..55754a8449 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -19,9 +19,13 @@ import ( "github.com/pkg/errors" ) -const patSuggest = "You can log in with your password or a Personal Access " + - "Token (PAT). Using a limited-scope PAT grants better security and is required " + - "for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/" +const ( + registerSuggest = "Log in with your Docker ID or email address to push and pull images from Docker Hub. " + + "If you don't have a Docker ID, head over to https://hub.docker.com/ to create one." + patSuggest = "You can log in with your password or a Personal Access " + + "Token (PAT). Using a limited-scope PAT grants better security and is required " + + "for organizations using SSO. Learn more at https://docs.docker.com/go/access-tokens/" +) // RegistryAuthenticationPrivilegedFunc returns a RequestPrivilegeFunc from the specified registry index info // for the given command. @@ -128,8 +132,10 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword argUser = strings.TrimSpace(argUser) if argUser == "" { if serverAddress == registry.IndexServer { - // if this is a default registry (docker hub), then display the following message. - fmt.Fprintln(cli.Out(), "Log in with your Docker ID or email address to push and pull images from Docker Hub. If you don't have a Docker ID, head over to https://hub.docker.com/ to create one.") + // When signing in to the default (Docker Hub) registry, we display + // hints for creating an account, and (if hints are enabled), using + // a token instead of a password. + _, _ = fmt.Fprintln(cli.Out(), registerSuggest) if hints.Enabled() { fmt.Fprintln(cli.Out(), patSuggest) fmt.Fprintln(cli.Out()) From 168a4bdc05a3a1d5ee86906b76980702970605d1 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 19 Oct 2024 12:52:32 +0200 Subject: [PATCH 8/8] cli/command: PromptUserForCredentials: suppress unhandled errors Keep the linters (and my IDE) happy; these errors should be safe to ignore. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 4b7a1e461308dfe1c5d620d07fb2b756bd944a7d) Signed-off-by: Sebastiaan van Stijn --- cli/command/registry.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cli/command/registry.go b/cli/command/registry.go index 55754a8449..cb966be1a7 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -31,12 +31,12 @@ const ( // for the given command. func RegistryAuthenticationPrivilegedFunc(cli Cli, index *registrytypes.IndexInfo, cmdName string) types.RequestPrivilegeFunc { return func(ctx context.Context) (string, error) { - fmt.Fprintf(cli.Out(), "\nLogin prior to %s:\n", cmdName) + _, _ = fmt.Fprintf(cli.Out(), "\nLogin prior to %s:\n", cmdName) indexServer := registry.GetAuthConfigKey(index) isDefaultRegistry := indexServer == registry.IndexServer authConfig, err := GetDefaultAuthConfig(cli.ConfigFile(), true, indexServer, isDefaultRegistry) if err != nil { - fmt.Fprintf(cli.Err(), "Unable to retrieve stored credentials for %s, error: %s.\n", indexServer, err) + _, _ = fmt.Fprintf(cli.Err(), "Unable to retrieve stored credentials for %s, error: %s.\n", indexServer, err) } select { @@ -137,8 +137,8 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword // a token instead of a password. _, _ = fmt.Fprintln(cli.Out(), registerSuggest) if hints.Enabled() { - fmt.Fprintln(cli.Out(), patSuggest) - fmt.Fprintln(cli.Out()) + _, _ = fmt.Fprintln(cli.Out(), patSuggest) + _, _ = fmt.Fprintln(cli.Out()) } } @@ -183,7 +183,7 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword if err != nil { return registrytypes.AuthConfig{}, err } - fmt.Fprint(cli.Out(), "\n") + _, _ = fmt.Fprintln(cli.Out()) if argPassword == "" { return registrytypes.AuthConfig{}, errors.Errorf("Error: Password Required") }