Silent login: if user did not provide -u and -p flag for login command but both username and password are retrieved in cred store, docker will automatically use the credentials found in the cred store to log in

Signed-off-by: shhsu@microsoft.com <shhsu@microsoft.com>
Signed-off-by: Peter Hsu <shhsu@microsoft.com>
Signed-off-by: shhsu <shhsu@microsoft.com>
Signed-off-by: Peter Hsu <shhsu@microsoft.com>

cli/command/registry.go

@@ -52,11 +52,15 @@ func RegistryAuthenticationPrivilegedFunc(cli Cli, index *registrytypes.IndexInf
 		fmt.Fprintf(cli.Out(), "\nPlease login prior to %s:\n", cmdName)
 		indexServer := registry.GetAuthConfigKey(index)
 		isDefaultRegistry := indexServer == ElectAuthServer(context.Background(), cli)
-		authConfig, err := ConfigureAuth(cli, "", "", indexServer, isDefaultRegistry)
+		authConfig, err := GetDefaultAuthConfig(cli, true, indexServer, isDefaultRegistry)
+		if err != nil {
+			fmt.Fprintf(cli.Err(), "Unable to retrieve stored credentials for %s, error: %s.\n", indexServer, err)
+		}
+		err = ConfigureAuth(cli, "", "", authConfig, isDefaultRegistry)
 		if err != nil {
 			return "", err
 		}
-		return EncodeAuthToBase64(authConfig)
+		return EncodeAuthToBase64(*authConfig)
 	}
 }
 
@@ -73,20 +77,29 @@ func ResolveAuthConfig(ctx context.Context, cli Cli, index *registrytypes.IndexI
 	return a
 }
 
-// ConfigureAuth returns an AuthConfig from the specified user, password and server.
-func ConfigureAuth(cli Cli, flUser, flPassword, serverAddress string, isDefaultRegistry bool) (types.AuthConfig, error) {
-	// On Windows, force the use of the regular OS stdin stream. Fixes #14336/#14210
-	if runtime.GOOS == "windows" {
-		cli.SetIn(NewInStream(os.Stdin))
-	}
-
+// GetDefaultAuthConfig gets the default auth config given a serverAddress
+// If credentials for given serverAddress exists in the credential store, the configuration will be populated with values in it
+func GetDefaultAuthConfig(cli Cli, checkCredStore bool, serverAddress string, isDefaultRegistry bool) (*types.AuthConfig, error) {
 	if !isDefaultRegistry {
 		serverAddress = registry.ConvertToHostname(serverAddress)
 	}
+	var authconfig types.AuthConfig
+	var err error
+	if checkCredStore {
+		authconfig, err = cli.ConfigFile().GetAuthConfig(serverAddress)
+	} else {
+		authconfig = types.AuthConfig{}
+	}
+	authconfig.ServerAddress = serverAddress
+	authconfig.IdentityToken = ""
+	return &authconfig, err
+}
 
-	authconfig, err := cli.ConfigFile().GetAuthConfig(serverAddress)
-	if err != nil {
-		return authconfig, err
+// ConfigureAuth handles prompting of user's username and password if needed
+func ConfigureAuth(cli Cli, flUser, flPassword string, authconfig *types.AuthConfig, isDefaultRegistry bool) error {
+	// On Windows, force the use of the regular OS stdin stream. Fixes #14336/#14210
+	if runtime.GOOS == "windows" {
+		cli.SetIn(NewInStream(os.Stdin))
 	}
 
 	// Some links documenting this:
@@ -97,7 +110,7 @@ func ConfigureAuth(cli Cli, flUser, flPassword, serverAddress string, isDefaultR
 	// will hit this if you attempt docker login from mintty where stdin
 	// is a pipe, not a character based console.
 	if flPassword == "" && !cli.In().IsTerminal() {
-		return authconfig, errors.Errorf("Error: Cannot perform an interactive login from a non TTY device")
+		return errors.Errorf("Error: Cannot perform an interactive login from a non TTY device")
 	}
 
 	authconfig.Username = strings.TrimSpace(authconfig.Username)
@@ -115,12 +128,12 @@ func ConfigureAuth(cli Cli, flUser, flPassword, serverAddress string, isDefaultR
 		}
 	}
 	if flUser == "" {
-		return authconfig, errors.Errorf("Error: Non-null Username Required")
+		return errors.Errorf("Error: Non-null Username Required")
 	}
 	if flPassword == "" {
 		oldState, err := term.SaveState(cli.In().FD())
 		if err != nil {
-			return authconfig, err
+			return err
 		}
 		fmt.Fprintf(cli.Out(), "Password: ")
 		term.DisableEcho(cli.In().FD(), oldState)
@@ -130,16 +143,14 @@ func ConfigureAuth(cli Cli, flUser, flPassword, serverAddress string, isDefaultR
 
 		term.RestoreTerminal(cli.In().FD(), oldState)
 		if flPassword == "" {
-			return authconfig, errors.Errorf("Error: Password Required")
+			return errors.Errorf("Error: Password Required")
 		}
 	}
 
 	authconfig.Username = flUser
 	authconfig.Password = flPassword
-	authconfig.ServerAddress = serverAddress
-	authconfig.IdentityToken = ""
 
-	return authconfig, nil
+	return nil
 }
 
 func readInput(in io.Reader, out io.Writer) string {

cli/command/registry/login.go

@@ -9,6 +9,9 @@ import (
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
+	"github.com/docker/docker/api/types"
+	registrytypes "github.com/docker/docker/api/types/registry"
+	"github.com/docker/docker/client"
 	"github.com/docker/docker/registry"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -47,10 +50,7 @@ func NewLoginCommand(dockerCli command.Cli) *cobra.Command {
 	return cmd
 }
 
-func runLogin(dockerCli command.Cli, opts loginOptions) error {
-	ctx := context.Background()
-	clnt := dockerCli.Client()
-
+func verifyloginOptions(dockerCli command.Cli, opts *loginOptions) error {
 	if opts.password != "" {
 		fmt.Fprintln(dockerCli.Err(), "WARNING! Using --password via the CLI is insecure. Use --password-stdin.")
 		if opts.passwordStdin {
@@ -71,7 +71,15 @@ func runLogin(dockerCli command.Cli, opts loginOptions) error {
 		opts.password = strings.TrimSuffix(string(contents), "\n")
 		opts.password = strings.TrimSuffix(opts.password, "\r")
 	}
+	return nil
+}
 
+func runLogin(dockerCli command.Cli, opts loginOptions) error {
+	ctx := context.Background()
+	clnt := dockerCli.Client()
+	if err := verifyloginOptions(dockerCli, &opts); err != nil {
+		return err
+	}
 	var (
 		serverAddress string
 		authServer    = command.ElectAuthServer(ctx, dockerCli)
@@ -82,21 +90,30 @@ func runLogin(dockerCli command.Cli, opts loginOptions) error {
 		serverAddress = authServer
 	}
 
+	var err error
+	var authConfig *types.AuthConfig
+	var response registrytypes.AuthenticateOKBody
 	isDefaultRegistry := serverAddress == authServer
-
-	authConfig, err := command.ConfigureAuth(dockerCli, opts.user, opts.password, serverAddress, isDefaultRegistry)
-	if err != nil {
-		return err
+	authConfig, err = command.GetDefaultAuthConfig(dockerCli, opts.user == "" && opts.password == "", serverAddress, isDefaultRegistry)
+	if err == nil && authConfig.Username != "" && authConfig.Password != "" {
+		response, err = loginWithCredStoreCreds(ctx, dockerCli, authConfig)
 	}
-	response, err := clnt.RegistryLogin(ctx, authConfig)
-	if err != nil {
-		return err
+	if err != nil || authConfig.Username == "" || authConfig.Password == "" {
+		err = command.ConfigureAuth(dockerCli, opts.user, opts.password, authConfig, isDefaultRegistry)
+		if err != nil {
+			return err
+		}
+
+		response, err = clnt.RegistryLogin(ctx, *authConfig)
+		if err != nil {
+			return err
+		}
 	}
 	if response.IdentityToken != "" {
 		authConfig.Password = ""
 		authConfig.IdentityToken = response.IdentityToken
 	}
-	if err := dockerCli.ConfigFile().GetCredentialsStore(serverAddress).Store(authConfig); err != nil {
+	if err := dockerCli.ConfigFile().GetCredentialsStore(serverAddress).Store(*authConfig); err != nil {
 		return errors.Errorf("Error saving credentials: %v", err)
 	}
 
@@ -105,3 +122,17 @@ func runLogin(dockerCli command.Cli, opts loginOptions) error {
 	}
 	return nil
 }
+
+func loginWithCredStoreCreds(ctx context.Context, dockerCli command.Cli, authConfig *types.AuthConfig) (registrytypes.AuthenticateOKBody, error) {
+	fmt.Fprintf(dockerCli.Out(), "Authenticating with existing credentials...\n")
+	cliClient := dockerCli.Client()
+	response, err := cliClient.RegistryLogin(ctx, *authConfig)
+	if err != nil {
+		if client.IsErrUnauthorized(err) {
+			fmt.Fprintf(dockerCli.Err(), "Stored credentials invalid or expired\n")
+		} else {
+			fmt.Fprintf(dockerCli.Err(), "Login did not succeed, error: %s\n", err)
+		}
+	}
+	return response, err
+}

cli/command/registry/login_test.go

@@ -0,0 +1,151 @@
+package registry
+
+import (
+	"bytes"
+	"fmt"
+	"testing"
+
+	"golang.org/x/net/context"
+
+	"github.com/docker/cli/internal/test"
+	"github.com/docker/docker/api/types"
+	registrytypes "github.com/docker/docker/api/types/registry"
+	"github.com/docker/docker/client"
+	"github.com/stretchr/testify/assert"
+)
+
+const userErr = "userunknownError"
+const testAuthErrMsg = "UNKNOWN_ERR"
+
+var testAuthErrors = map[string]error{
+	userErr: fmt.Errorf(testAuthErrMsg),
+}
+
+var expiredPassword = "I_M_EXPIRED"
+
+type fakeClient struct {
+	client.Client
+}
+
+// nolint: unparam
+func (c fakeClient) RegistryLogin(ctx context.Context, auth types.AuthConfig) (registrytypes.AuthenticateOKBody, error) {
+	if auth.Password == expiredPassword {
+		return registrytypes.AuthenticateOKBody{}, fmt.Errorf("Invalid Username or Password")
+	}
+	err := testAuthErrors[auth.Username]
+	return registrytypes.AuthenticateOKBody{}, err
+}
+
+func TestLoginWithCredStoreCreds(t *testing.T) {
+	testCases := []struct {
+		inputAuthConfig types.AuthConfig
+		expectedMsg     string
+		expectedErr     string
+	}{
+		{
+			inputAuthConfig: types.AuthConfig{},
+			expectedMsg:     "Authenticating with existing credentials...\n",
+		},
+		{
+			inputAuthConfig: types.AuthConfig{
+				Username: userErr,
+			},
+			expectedMsg: "Authenticating with existing credentials...\n",
+			expectedErr: fmt.Sprintf("Login did not succeed, error: %s\n", testAuthErrMsg),
+		},
+		// can't easily test the 401 case because client.IsErrUnauthorized(err) involving
+		// creating an error of a private type
+	}
+	ctx := context.Background()
+	for _, tc := range testCases {
+		cli := (*test.FakeCli)(test.NewFakeCli(&fakeClient{}))
+		errBuf := new(bytes.Buffer)
+		cli.SetErr(errBuf)
+		loginWithCredStoreCreds(ctx, cli, &tc.inputAuthConfig)
+		outputString := cli.OutBuffer().String()
+		assert.Equal(t, tc.expectedMsg, outputString)
+		errorString := errBuf.String()
+		assert.Equal(t, tc.expectedErr, errorString)
+	}
+}
+
+func TestRunLogin(t *testing.T) {
+	const storedServerAddress = "reg1"
+	const validUsername = "u1"
+	const validPassword = "p1"
+	const validPassword2 = "p2"
+
+	validAuthConfig := types.AuthConfig{
+		ServerAddress: storedServerAddress,
+		Username:      validUsername,
+		Password:      validPassword,
+	}
+	expiredAuthConfig := types.AuthConfig{
+		ServerAddress: storedServerAddress,
+		Username:      validUsername,
+		Password:      expiredPassword,
+	}
+	testCases := []struct {
+		inputLoginOption  loginOptions
+		inputStoredCred   *types.AuthConfig
+		expectedErr       string
+		expectedSavedCred types.AuthConfig
+	}{
+		{
+			inputLoginOption: loginOptions{
+				serverAddress: storedServerAddress,
+			},
+			inputStoredCred:   &validAuthConfig,
+			expectedErr:       "",
+			expectedSavedCred: validAuthConfig,
+		},
+		{
+			inputLoginOption: loginOptions{
+				serverAddress: storedServerAddress,
+			},
+			inputStoredCred: &expiredAuthConfig,
+			expectedErr:     "Error: Cannot perform an interactive login from a non TTY device",
+		},
+		{
+			inputLoginOption: loginOptions{
+				serverAddress: storedServerAddress,
+				user:          validUsername,
+				password:      validPassword2,
+			},
+			inputStoredCred: &validAuthConfig,
+			expectedErr:     "",
+			expectedSavedCred: types.AuthConfig{
+				ServerAddress: storedServerAddress,
+				Username:      validUsername,
+				Password:      validPassword2,
+			},
+		},
+		{
+			inputLoginOption: loginOptions{
+				serverAddress: storedServerAddress,
+				user:          userErr,
+				password:      validPassword,
+			},
+			inputStoredCred: &validAuthConfig,
+			expectedErr:     testAuthErrMsg,
+		},
+	}
+	for _, tc := range testCases {
+		cli := test.NewFakeCli(&fakeClient{})
+		errBuf := new(bytes.Buffer)
+		cli.SetErr(errBuf)
+		if tc.inputStoredCred != nil {
+			cred := *tc.inputStoredCred
+			cli.ConfigFile().GetCredentialsStore(cred.ServerAddress).Store(cred)
+		}
+		loginErr := runLogin(cli, tc.inputLoginOption)
+		if tc.expectedErr != "" {
+			assert.Equal(t, tc.expectedErr, loginErr.Error())
+		} else {
+			assert.Nil(t, loginErr)
+			savedCred, credStoreErr := cli.ConfigFile().GetCredentialsStore(tc.inputStoredCred.ServerAddress).Get(tc.inputStoredCred.ServerAddress)
+			assert.Nil(t, credStoreErr)
+			assert.Equal(t, tc.expectedSavedCred, savedCred)
+		}
+	}
+}

cli/command/registry_test.go

@@ -1,6 +1,8 @@
 package command_test
 
 import (
+	"bytes"
+	"fmt"
 	"testing"
 
 	"github.com/pkg/errors"
@@ -8,6 +10,7 @@ import (
 	"golang.org/x/net/context"
 
 	// Prevents a circular import with "github.com/docker/cli/internal/test"
+
 	. "github.com/docker/cli/cli/command"
 	"github.com/docker/cli/internal/test"
 	"github.com/docker/docker/api/types"
@@ -19,6 +22,19 @@ type fakeClient struct {
 	infoFunc func() (types.Info, error)
 }
 
+var testAuthConfigs = []types.AuthConfig{
+	{
+		ServerAddress: "https://index.docker.io/v1/",
+		Username:      "u0",
+		Password:      "p0",
+	},
+	{
+		ServerAddress: "server1.io",
+		Username:      "u1",
+		Password:      "p1",
+	},
+}
+
 func (cli *fakeClient) Info(_ context.Context) (types.Info, error) {
 	if cli.infoFunc != nil {
 		return cli.infoFunc()
@@ -73,3 +89,58 @@ func TestElectAuthServer(t *testing.T) {
 		}
 	}
 }
+
+func TestGetDefaultAuthConfig(t *testing.T) {
+	testCases := []struct {
+		checkCredStore     bool
+		inputServerAddress string
+		expectedErr        string
+		expectedAuthConfig types.AuthConfig
+	}{
+		{
+			checkCredStore:     false,
+			inputServerAddress: "",
+			expectedErr:        "",
+			expectedAuthConfig: types.AuthConfig{
+				ServerAddress: "",
+				Username:      "",
+				Password:      "",
+			},
+		},
+		{
+			checkCredStore:     true,
+			inputServerAddress: testAuthConfigs[0].ServerAddress,
+			expectedErr:        "",
+			expectedAuthConfig: testAuthConfigs[0],
+		},
+		{
+			checkCredStore:     true,
+			inputServerAddress: testAuthConfigs[1].ServerAddress,
+			expectedErr:        "",
+			expectedAuthConfig: testAuthConfigs[1],
+		},
+		{
+			checkCredStore:     true,
+			inputServerAddress: fmt.Sprintf("https://%s", testAuthConfigs[1].ServerAddress),
+			expectedErr:        "",
+			expectedAuthConfig: testAuthConfigs[1],
+		},
+	}
+	cli := test.NewFakeCli(&fakeClient{})
+	errBuf := new(bytes.Buffer)
+	cli.SetErr(errBuf)
+	for _, authconfig := range testAuthConfigs {
+		cli.ConfigFile().GetCredentialsStore(authconfig.ServerAddress).Store(authconfig)
+	}
+	for _, tc := range testCases {
+		serverAddress := tc.inputServerAddress
+		authconfig, err := GetDefaultAuthConfig(cli, tc.checkCredStore, serverAddress, serverAddress == "https://index.docker.io/v1/")
+		if tc.expectedErr != "" {
+			assert.NotNil(t, err)
+			assert.Equal(t, tc.expectedErr, err.Error())
+		} else {
+			assert.Nil(t, err)
+			assert.Equal(t, tc.expectedAuthConfig, *authconfig)
+		}
+	}
+}

internal/test/store.go

@@ -5,8 +5,8 @@ import (
 	"github.com/docker/docker/api/types"
 )
 
-// fake store implements a credentials.Store that only acts as an in memory map
-type fakeStore struct {
+// FakeStore implements a credentials.Store that only acts as an in memory map
+type FakeStore struct {
 	store      map[string]types.AuthConfig
 	eraseFunc  func(serverAddress string) error
 	getFunc    func(serverAddress string) (types.AuthConfig, error)
@@ -16,31 +16,36 @@ type fakeStore struct {
 
 // NewFakeStore creates a new file credentials store.
 func NewFakeStore() credentials.Store {
-	return &fakeStore{store: map[string]types.AuthConfig{}}
+	return &FakeStore{store: map[string]types.AuthConfig{}}
 }
 
-func (c *fakeStore) SetStore(store map[string]types.AuthConfig) {
+// SetStore is used to overrides Set function
+func (c *FakeStore) SetStore(store map[string]types.AuthConfig) {
 	c.store = store
 }
 
-func (c *fakeStore) SetEraseFunc(eraseFunc func(string) error) {
+// SetEraseFunc is used to overrides Erase function
+func (c *FakeStore) SetEraseFunc(eraseFunc func(string) error) {
 	c.eraseFunc = eraseFunc
 }
 
-func (c *fakeStore) SetGetFunc(getFunc func(string) (types.AuthConfig, error)) {
+// SetGetFunc is used to overrides Get function
+func (c *FakeStore) SetGetFunc(getFunc func(string) (types.AuthConfig, error)) {
 	c.getFunc = getFunc
 }
 
-func (c *fakeStore) SetGetAllFunc(getAllFunc func() (map[string]types.AuthConfig, error)) {
+// SetGetAllFunc is used to  overrides GetAll function
+func (c *FakeStore) SetGetAllFunc(getAllFunc func() (map[string]types.AuthConfig, error)) {
 	c.getAllFunc = getAllFunc
 }
 
-func (c *fakeStore) SetStoreFunc(storeFunc func(types.AuthConfig) error) {
+// SetStoreFunc is used to override Store function
+func (c *FakeStore) SetStoreFunc(storeFunc func(types.AuthConfig) error) {
 	c.storeFunc = storeFunc
 }
 
 // Erase removes the given credentials from the map store
-func (c *fakeStore) Erase(serverAddress string) error {
+func (c *FakeStore) Erase(serverAddress string) error {
 	if c.eraseFunc != nil {
 		return c.eraseFunc(serverAddress)
 	}
@@ -49,14 +54,15 @@ func (c *fakeStore) Erase(serverAddress string) error {
 }
 
 // Get retrieves credentials for a specific server from the map store.
-func (c *fakeStore) Get(serverAddress string) (types.AuthConfig, error) {
+func (c *FakeStore) Get(serverAddress string) (types.AuthConfig, error) {
 	if c.getFunc != nil {
 		return c.getFunc(serverAddress)
 	}
 	return c.store[serverAddress], nil
 }
 
-func (c *fakeStore) GetAll() (map[string]types.AuthConfig, error) {
+// GetAll returns the key value pairs of ServerAddress => Username
+func (c *FakeStore) GetAll() (map[string]types.AuthConfig, error) {
 	if c.getAllFunc != nil {
 		return c.getAllFunc()
 	}
@@ -64,7 +70,7 @@ func (c *fakeStore) GetAll() (map[string]types.AuthConfig, error) {
 }
 
 // Store saves the given credentials in the map store.
-func (c *fakeStore) Store(authConfig types.AuthConfig) error {
+func (c *FakeStore) Store(authConfig types.AuthConfig) error {
 	if c.storeFunc != nil {
 		return c.storeFunc(authConfig)
 	}