From 3a8485085d94c85dae8078ee36310baa69a21217 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 --- 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 7be5f5a290..bd001c251d 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -110,7 +110,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: @@ -142,38 +142,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 a55cfe5f82a13105ca49ee70f72ff6ef00400917 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 --- 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 bd001c251d..92ea17f90c 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -123,11 +123,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 581cf36bd4206b4cccf3cd1ad93162bbbffaad37 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 --- 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 92ea17f90c..b2696d7b1d 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -150,10 +150,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 eda78e9cdc79e4ec2d886e0a75c0abe06dd5489e 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 --- 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 b2696d7b1d..7749f9601a 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -123,9 +123,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.") @@ -136,6 +135,7 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword } var prompt string + defaultUsername = strings.TrimSpace(defaultUsername) if defaultUsername == "" { prompt = "Username: " } else { From a21a5f42433267052441cdb7ebe7604dfcc7f159 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 --- cli/command/registry.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/command/registry.go b/cli/command/registry.go index 7749f9601a..d5e10c191d 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -155,6 +155,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 3d8b49523d565867ee7a6f3e0a7c795b5aec4e79 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 --- 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 d5e10c191d..4699aabb0a 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -161,7 +161,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 378a3d7d3608308aa8ac01a954497916e0110653 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 --- 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 4699aabb0a..618121f283 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -18,9 +18,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. @@ -126,8 +130,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 4b7a1e461308dfe1c5d620d07fb2b756bd944a7d 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 --- 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 618121f283..20a0d428e9 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -30,12 +30,12 @@ const ( // for the given command. func RegistryAuthenticationPrivilegedFunc(cli Cli, index *registrytypes.IndexInfo, cmdName string) registrytypes.RequestAuthConfig { 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 { @@ -135,8 +135,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()) } } @@ -181,7 +181,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") }