diff --git a/cli/command/registry.go b/cli/command/registry.go index 452e2d7354cc..75b979e18561 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -144,8 +144,8 @@ func PromptUserForCredentials(ctx context.Context, cli Cli, argUser, argPassword } } - argPassword = strings.TrimSpace(argPassword) - if argPassword == "" { + isEmpty := strings.TrimSpace(argPassword) == "" + if isEmpty { restoreInput, err := prompt.DisableInputEcho(cli.In()) if err != nil { return registrytypes.AuthConfig{}, err diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index 913c000c4cc2..364107a238ba 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -1,11 +1,11 @@ package registry import ( + "bytes" "context" "errors" "fmt" "io" - "strings" "github.com/containerd/errdefs" "github.com/docker/cli/cli" @@ -88,6 +88,38 @@ func verifyLoginFlags(flags *pflag.FlagSet, opts loginOptions) error { return nil } +// readSecretFromStdin reads the secret from r and returns it as a string. +// It trims terminal line-endings (LF, CRLF, or CR), which may be added when +// inputting interactively or piping input. The value is otherwise treated as +// opaque, preserving any other whitespace, including newlines, per [NIST SP 800-63B §5.1.1.2]. +// Note that trimming whitespace may still happen elsewhere (see [NIST SP 800-63B (revision 4) §3.1.1.2]); +// +// > Verifiers **MAY** make limited allowances for mistyping (e.g., removing +// > leading and trailing whitespace characters before verification, allowing +// > the verification of passwords with differing cases for the leading character) +// +// [NIST SP 800-63B §5.1.1.2]: https://pages.nist.gov/800-63-3/sp800-63b.html#memsecretver +// [NIST SP 800-63B (revision 4) §3.1.1.2]: https://pages.nist.gov/800-63-4/sp800-63b.html#passwordver +func readSecretFromStdin(r io.Reader) (string, error) { + b, err := io.ReadAll(r) + if err != nil { + return "", err + } + if len(b) == 0 { + return "", nil + } + + for _, eol := range [][]byte{[]byte("\r\n"), []byte("\n"), []byte("\r")} { + var ok bool + b, ok = bytes.CutSuffix(b, eol) + if ok { + break + } + } + + return string(b), 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.") @@ -97,14 +129,11 @@ func verifyLoginOptions(dockerCLI command.Streams, opts *loginOptions) error { if opts.user == "" { return errors.New("username is empty") } - - contents, err := io.ReadAll(dockerCLI.In()) + p, err := readSecretFromStdin(dockerCLI.In()) if err != nil { return err } - - opts.password = strings.TrimSuffix(string(contents), "\n") - opts.password = strings.TrimSuffix(opts.password, "\r") + opts.password = p } return nil } diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index 9dfb13dfe751..23294c354b75 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -1,6 +1,7 @@ package registry import ( + "bytes" "context" "errors" "fmt" @@ -10,6 +11,7 @@ import ( "time" "github.com/creack/pty" + "github.com/docker/cli/cli/config/configfile" configtypes "github.com/docker/cli/cli/config/types" "github.com/docker/cli/cli/streams" "github.com/docker/cli/internal/prompt" @@ -19,7 +21,6 @@ import ( "github.com/moby/moby/client" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" - "gotest.tools/v3/fs" ) const ( @@ -71,8 +72,9 @@ func TestLoginWithCredStoreCreds(t *testing.T) { }, } ctx := context.Background() + tmpDir := t.TempDir() cli := test.NewFakeCli(&fakeClient{}) - cli.ConfigFile().Filename = filepath.Join(t.TempDir(), "config.json") + cli.SetConfigFile(configfile.New(filepath.Join(tmpDir, "config.json"))) for _, tc := range testCases { _, err := loginWithStoredCredentials(ctx, cli, tc.inputAuthConfig) if tc.expectedErrMsg != "" { @@ -91,6 +93,7 @@ func TestRunLogin(t *testing.T) { testCases := []struct { doc string priorCredentials map[string]configtypes.AuthConfig + stdIn string input loginOptions expectedCredentials map[string]configtypes.AuthConfig expectedErr string @@ -286,20 +289,72 @@ func TestRunLogin(t *testing.T) { }, }, }, + { + doc: "password with leading and trailing spaces", + priorCredentials: map[string]configtypes.AuthConfig{}, + input: loginOptions{ + serverAddress: "reg1", + user: "my-username", + password: " my password with spaces ", + }, + expectedCredentials: map[string]configtypes.AuthConfig{ + "reg1": { + Username: "my-username", + Password: " my password with spaces ", + ServerAddress: "reg1", + }, + }, + }, + { + doc: "password stdin with line-endings", + priorCredentials: map[string]configtypes.AuthConfig{}, + stdIn: " my password with spaces \r\n", + input: loginOptions{ + serverAddress: "reg1", + user: "my-username", + passwordStdin: true, + }, + expectedCredentials: map[string]configtypes.AuthConfig{ + "reg1": { + Username: "my-username", + Password: " my password with spaces ", + ServerAddress: "reg1", + }, + }, + }, + { + doc: "password stdin with multiple line-endings", + priorCredentials: map[string]configtypes.AuthConfig{}, + stdIn: " my password\nwith spaces \r\n\r\n", + input: loginOptions{ + serverAddress: "reg1", + user: "my-username", + passwordStdin: true, + }, + expectedCredentials: map[string]configtypes.AuthConfig{ + "reg1": { + Username: "my-username", + Password: " my password\nwith spaces \r\n", + ServerAddress: "reg1", + }, + }, + }, } for _, tc := range testCases { t.Run(tc.doc, func(t *testing.T) { - tmpFile := fs.NewFile(t, "test-run-login") - defer tmpFile.Remove() + tmpDir := t.TempDir() + cfg := configfile.New(filepath.Join(tmpDir, "config.json")) cli := test.NewFakeCli(&fakeClient{}) - configfile := cli.ConfigFile() - configfile.Filename = tmpFile.Path() + cli.SetConfigFile(cfg) + if tc.input.passwordStdin { + cli.SetIn(streams.NewIn(io.NopCloser(bytes.NewBufferString(tc.stdIn)))) + } for _, priorCred := range tc.priorCredentials { - assert.NilError(t, configfile.GetCredentialsStore(priorCred.ServerAddress).Store(priorCred)) + assert.NilError(t, cfg.GetCredentialsStore(priorCred.ServerAddress).Store(priorCred)) } - storedCreds, err := configfile.GetAllCredentials() + storedCreds, err := cfg.GetAllCredentials() assert.NilError(t, err) assert.DeepEqual(t, storedCreds, tc.priorCredentials) @@ -310,7 +365,7 @@ func TestRunLogin(t *testing.T) { } assert.NilError(t, loginErr) - outputCreds, err := configfile.GetAllCredentials() + outputCreds, err := cfg.GetAllCredentials() assert.Check(t, err) assert.DeepEqual(t, outputCreds, tc.expectedCredentials) }) @@ -356,11 +411,10 @@ func TestLoginNonInteractive(t *testing.T) { for _, registryAddr := range registries { for _, tc := range testCases { t.Run(tc.doc, func(t *testing.T) { - tmpFile := fs.NewFile(t, "test-run-login") - defer tmpFile.Remove() + tmpDir := t.TempDir() + cfg := configfile.New(filepath.Join(tmpDir, "config.json")) cli := test.NewFakeCli(&fakeClient{}) - cfg := cli.ConfigFile() - cfg.Filename = tmpFile.Path() + cli.SetConfigFile(cfg) options := loginOptions{ serverAddress: registryAddr, } @@ -419,11 +473,10 @@ func TestLoginNonInteractive(t *testing.T) { for _, registryAddr := range registries { for _, tc := range testCases { t.Run(tc.doc, func(t *testing.T) { - tmpFile := fs.NewFile(t, "test-run-login") - defer tmpFile.Remove() + tmpDir := t.TempDir() + cfg := configfile.New(filepath.Join(tmpDir, "config.json")) cli := test.NewFakeCli(&fakeClient{}) - cfg := cli.ConfigFile() - cfg.Filename = tmpFile.Path() + cli.SetConfigFile(cfg) serverAddress := registryAddr if serverAddress == "" { serverAddress = "https://index.docker.io/v1/" @@ -465,17 +518,15 @@ func TestLoginTermination(t *testing.T) { _ = p.Close() }) + tmpDir := t.TempDir() + cfg := configfile.New(filepath.Join(tmpDir, "config.json")) cli := test.NewFakeCli(&fakeClient{}, func(fc *test.FakeCli) { fc.SetOut(streams.NewOut(tty)) fc.SetIn(streams.NewIn(tty)) }) - tmpFile := fs.NewFile(t, "test-login-termination") - defer tmpFile.Remove() - - configFile := cli.ConfigFile() - configFile.Filename = tmpFile.Path() + cli.SetConfigFile(cfg) - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithCancel(t.Context()) t.Cleanup(cancel) runErr := make(chan error)