cli/command/registry: preserve all whitespace in secrets

Preserve all whitespace and treat the secret as an opaque value,
leaving it to the registry to (in)validate. We still check for
empty values in some places.

This partially reverts a21a5f4,
but checks for empty (whitespace-only) passwords without mutating
the value.

This better aligns with [NIST SP 800-63B §5.1.1.2], which describes
that the value should be treated as opaque, preserving any other whitespace,
including newlines. 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

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Author: thaJeztah

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

cli/command/registry/login.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"strings"
 
 	"github.com/containerd/errdefs"
 	"github.com/docker/cli/cli"
@@ -88,6 +87,42 @@ 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
+	}
+
+	n := len(b)
+	switch b[n-1] {
+	case '\n':
+		if n >= 2 && b[n-2] == '\r' {
+			b = b[:n-2]
+		} else {
+			b = b[:n-1]
+		}
+	case '\r':
+		b = b[:n-1]
+	}
+
+	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 +132,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
 }

cli/command/registry/login_test.go

@@ -1,6 +1,7 @@
 package registry
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -287,6 +288,56 @@ 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{},
+			input: loginOptions{
+				serverAddress: "reg1",
+				user:          "my-username",
+				passwordStdin: true,
+				password:      "  my password with spaces  \r\n",
+			},
+			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{},
+			input: loginOptions{
+				serverAddress: "reg1",
+				user:          "my-username",
+				passwordStdin: true,
+				password:      "  my password\nwith spaces  \r\n\r\n",
+			},
+			expectedCredentials: map[string]configtypes.AuthConfig{
+				"reg1": {
+					Username:      "my-username",
+					Password:      "  my password\nwith spaces  \r\n",
+					ServerAddress: "reg1",
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -295,6 +346,9 @@ func TestRunLogin(t *testing.T) {
 			cfg := configfile.New(filepath.Join(tmpDir, "config.json"))
 			cli := test.NewFakeCli(&fakeClient{})
 			cli.SetConfigFile(cfg)
+			if tc.input.passwordStdin {
+				cli.SetIn(streams.NewIn(io.NopCloser(bytes.NewBufferString(tc.input.password))))
+			}
 
 			for _, priorCred := range tc.priorCredentials {
 				assert.NilError(t, cfg.GetCredentialsStore(priorCred.ServerAddress).Store(priorCred))