From 13f0d464114b5a4715e70eaf8b5b1ece64663431 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 3 Dec 2024 11:31:09 +0100 Subject: [PATCH 1/6] cli/command/registry: don't return creds on error Be more explicit on not returning a response if there was an error; change some non-exported functions to return a pointer, and return nil instead. Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index ec509a1aea8e..06b8a6ea955e 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -200,7 +200,7 @@ func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, op return nil, err } - return &response, nil + return response, nil } func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (*registrytypes.AuthenticateOKBody, error) { @@ -219,7 +219,7 @@ func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (*regis return nil, err } - return &response, nil + return response, nil } func storeCredentials(dockerCli command.Cli, authConfig registrytypes.AuthConfig) error { @@ -231,30 +231,32 @@ func storeCredentials(dockerCli command.Cli, authConfig registrytypes.AuthConfig return nil } -func loginWithRegistry(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) { +func loginWithRegistry(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) { response, err := dockerCli.Client().RegistryLogin(ctx, authConfig) - if err != nil && client.IsErrConnectionFailed(err) { - // If the server isn't responding (yet) attempt to login purely client side - response, err = loginClientSide(ctx, authConfig) - } - // If we (still) have an error, give up if err != nil { - return registrytypes.AuthenticateOKBody{}, err + if client.IsErrConnectionFailed(err) { + // daemon isn't responding; attempt to login client side. + return loginClientSide(ctx, authConfig) + } + return nil, err } - return response, nil + return &response, nil } -func loginClientSide(ctx context.Context, auth registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) { +func loginClientSide(ctx context.Context, auth registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) { svc, err := registry.NewService(registry.ServiceOptions{}) if err != nil { - return registrytypes.AuthenticateOKBody{}, err + return nil, err } status, token, err := svc.Auth(ctx, &auth, command.UserAgent()) + if err != nil { + return nil, err + } - return registrytypes.AuthenticateOKBody{ + return ®istrytypes.AuthenticateOKBody{ Status: status, IdentityToken: token, - }, err + }, nil } From b5a00d0b0f950bb51b30eb6ecce3d58733457d5c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 3 Dec 2024 12:50:12 +0100 Subject: [PATCH 2/6] cli/command/registry: loginWithRegistry: use shallower interface This function only needs the API client, not all of the CLI. Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index 06b8a6ea955e..145922dc985b 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -187,7 +187,7 @@ func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, op return nil, err } - response, err := loginWithRegistry(ctx, dockerCli, authConfig) + response, err := loginWithRegistry(ctx, dockerCli.Client(), authConfig) if err != nil { return nil, err } @@ -210,7 +210,7 @@ func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (*regis return nil, err } - response, err := loginWithRegistry(ctx, dockerCli, registrytypes.AuthConfig(*authConfig)) + response, err := loginWithRegistry(ctx, dockerCli.Client(), registrytypes.AuthConfig(*authConfig)) if err != nil { return nil, err } @@ -231,8 +231,8 @@ func storeCredentials(dockerCli command.Cli, authConfig registrytypes.AuthConfig return nil } -func loginWithRegistry(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) { - response, err := dockerCli.Client().RegistryLogin(ctx, authConfig) +func loginWithRegistry(ctx context.Context, apiClient client.SystemAPIClient, authConfig registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) { + response, err := apiClient.RegistryLogin(ctx, authConfig) if err != nil { if client.IsErrConnectionFailed(err) { // daemon isn't responding; attempt to login client side. From cf89afb32baaf091389c1518adce7ca4004ec140 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 3 Dec 2024 13:00:59 +0100 Subject: [PATCH 3/6] cli/command/registry: storeCredentials: accept configfile as arg This function only needs access to the configfile Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index 145922dc985b..8efeed85fe84 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -11,6 +11,7 @@ import ( "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" + "github.com/docker/cli/cli/config/configfile" configtypes "github.com/docker/cli/cli/config/types" "github.com/docker/cli/cli/internal/oauth/manager" registrytypes "github.com/docker/docker/api/types/registry" @@ -134,7 +135,7 @@ func loginWithStoredCredentials(ctx context.Context, dockerCli command.Cli, auth authConfig.IdentityToken = response.IdentityToken } - if err := storeCredentials(dockerCli, authConfig); err != nil { + if err := storeCredentials(dockerCli.ConfigFile(), authConfig); err != nil { return nil, err } @@ -196,7 +197,7 @@ func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, op authConfig.Password = "" authConfig.IdentityToken = response.IdentityToken } - if err = storeCredentials(dockerCli, authConfig); err != nil { + if err = storeCredentials(dockerCli.ConfigFile(), authConfig); err != nil { return nil, err } @@ -215,15 +216,15 @@ func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (*regis return nil, err } - if err = storeCredentials(dockerCli, registrytypes.AuthConfig(*authConfig)); err != nil { + if err = storeCredentials(dockerCli.ConfigFile(), registrytypes.AuthConfig(*authConfig)); err != nil { return nil, err } return response, nil } -func storeCredentials(dockerCli command.Cli, authConfig registrytypes.AuthConfig) error { - creds := dockerCli.ConfigFile().GetCredentialsStore(authConfig.ServerAddress) +func storeCredentials(cfg *configfile.ConfigFile, authConfig registrytypes.AuthConfig) error { + creds := cfg.GetCredentialsStore(authConfig.ServerAddress) if err := creds.Store(configtypes.AuthConfig(authConfig)); err != nil { return errors.Errorf("Error saving credentials: %v", err) } From 575e373669e4cc72fbe9b9ebcedbd7328bb4ae1d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 3 Dec 2024 18:37:35 +0100 Subject: [PATCH 4/6] cli/command/registry: rename some vars that collided with imports Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index 0dbfaa65706d..cf6b126380a7 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -349,16 +349,16 @@ func TestLoginNonInteractive(t *testing.T) { // "" meaning default registry registries := []string{"", "my-registry.com"} - for _, registry := range registries { + 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() cli := test.NewFakeCli(&fakeClient{}) - configfile := cli.ConfigFile() - configfile.Filename = tmpFile.Path() + cfg := cli.ConfigFile() + cfg.Filename = tmpFile.Path() options := loginOptions{ - serverAddress: registry, + serverAddress: registryAddr, } if tc.username { options.user = "my-username" @@ -412,26 +412,26 @@ func TestLoginNonInteractive(t *testing.T) { // "" meaning default registry registries := []string{"", "my-registry.com"} - for _, registry := range registries { + 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() cli := test.NewFakeCli(&fakeClient{}) - configfile := cli.ConfigFile() - configfile.Filename = tmpFile.Path() - serverAddress := registry + cfg := cli.ConfigFile() + cfg.Filename = tmpFile.Path() + serverAddress := registryAddr if serverAddress == "" { serverAddress = "https://index.docker.io/v1/" } - assert.NilError(t, configfile.GetCredentialsStore(serverAddress).Store(configtypes.AuthConfig{ + assert.NilError(t, cfg.GetCredentialsStore(serverAddress).Store(configtypes.AuthConfig{ Username: "my-username", Password: "my-password", ServerAddress: serverAddress, })) options := loginOptions{ - serverAddress: registry, + serverAddress: registryAddr, } if tc.username { options.user = "my-username" From 297afb2a2b717f51a761baeb1517726f8739928e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 3 Dec 2024 18:38:27 +0100 Subject: [PATCH 5/6] cli/command/registry: TestLoginWithCredStoreCreds slight refactor - also check errors that were previously not handled - use the fakeCLI's buffer instead of creating one manually Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login_test.go | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index cf6b126380a7..a8af6acf8175 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -1,10 +1,10 @@ package registry import ( - "bytes" "context" "errors" "fmt" + "path/filepath" "testing" "time" @@ -55,8 +55,9 @@ func (c *fakeClient) RegistryLogin(_ context.Context, auth registrytypes.AuthCon func TestLoginWithCredStoreCreds(t *testing.T) { testCases := []struct { inputAuthConfig registrytypes.AuthConfig - expectedMsg string expectedErr string + expectedMsg string + expectedErrMsg string }{ { inputAuthConfig: registrytypes.AuthConfig{}, @@ -66,20 +67,25 @@ func TestLoginWithCredStoreCreds(t *testing.T) { inputAuthConfig: registrytypes.AuthConfig{ Username: unknownUser, }, - expectedMsg: "Authenticating with existing credentials...\n", - expectedErr: fmt.Sprintf("Login did not succeed, error: %s\n", errUnknownUser), + expectedErr: errUnknownUser, + expectedMsg: "Authenticating with existing credentials...\n", + expectedErrMsg: fmt.Sprintf("Login did not succeed, error: %s\n", errUnknownUser), }, } ctx := context.Background() + cli := test.NewFakeCli(&fakeClient{}) + cli.ConfigFile().Filename = filepath.Join(t.TempDir(), "config.json") for _, tc := range testCases { - cli := test.NewFakeCli(&fakeClient{}) - errBuf := new(bytes.Buffer) - cli.SetErr(streams.NewOut(errBuf)) - loginWithStoredCredentials(ctx, cli, tc.inputAuthConfig) - outputString := cli.OutBuffer().String() - assert.Check(t, is.Equal(tc.expectedMsg, outputString)) - errorString := errBuf.String() - assert.Check(t, is.Equal(tc.expectedErr, errorString)) + _, err := loginWithStoredCredentials(ctx, cli, tc.inputAuthConfig) + if tc.expectedErrMsg != "" { + assert.Check(t, is.Error(err, tc.expectedErr)) + } else { + assert.NilError(t, err) + } + assert.Check(t, is.Equal(tc.expectedMsg, cli.OutBuffer().String())) + assert.Check(t, is.Equal(tc.expectedErrMsg, cli.ErrBuffer().String())) + cli.ErrBuffer().Reset() + cli.OutBuffer().Reset() } } From e398d16c04818032ab23c7145419f85a35bfd498 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 3 Dec 2024 19:04:24 +0100 Subject: [PATCH 6/6] cli/command/registry: return status only instead of whole response Various functions were passing through the API response as a whole, but effectively only needed it for a status message (if any). Reduce what's returned to only the message. Signed-off-by: Sebastiaan van Stijn --- cli/command/registry/login.go | 45 ++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index 8efeed85fe84..3facb75ccc7e 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -89,7 +89,7 @@ func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) err } var ( serverAddress string - response *registrytypes.AuthenticateOKBody + msg string ) if opts.serverAddress != "" && opts.serverAddress != registry.DefaultNamespace { serverAddress = opts.serverAddress @@ -101,25 +101,25 @@ func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) err // attempt login with current (stored) credentials authConfig, err := command.GetDefaultAuthConfig(dockerCli.ConfigFile(), opts.user == "" && opts.password == "", serverAddress, isDefaultRegistry) if err == nil && authConfig.Username != "" && authConfig.Password != "" { - response, err = loginWithStoredCredentials(ctx, dockerCli, authConfig) + msg, err = loginWithStoredCredentials(ctx, dockerCli, authConfig) } // if we failed to authenticate with stored credentials (or didn't have stored credentials), // prompt the user for new credentials if err != nil || authConfig.Username == "" || authConfig.Password == "" { - response, err = loginUser(ctx, dockerCli, opts, authConfig.Username, authConfig.ServerAddress) + msg, err = loginUser(ctx, dockerCli, opts, authConfig.Username, authConfig.ServerAddress) if err != nil { return err } } - if response != nil && response.Status != "" { - _, _ = fmt.Fprintln(dockerCli.Out(), response.Status) + if msg != "" { + _, _ = fmt.Fprintln(dockerCli.Out(), msg) } return nil } -func loginWithStoredCredentials(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) { +func loginWithStoredCredentials(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (msg string, _ error) { _, _ = fmt.Fprintf(dockerCli.Out(), "Authenticating with existing credentials...\n") response, err := dockerCli.Client().RegistryLogin(ctx, authConfig) if err != nil { @@ -136,10 +136,10 @@ func loginWithStoredCredentials(ctx context.Context, dockerCli command.Cli, auth } if err := storeCredentials(dockerCli.ConfigFile(), authConfig); err != nil { - return nil, err + return "", err } - return &response, err + return response.Status, err } const OauthLoginEscapeHatchEnvVar = "DOCKER_CLI_DISABLE_OAUTH_LOGIN" @@ -155,7 +155,7 @@ func isOauthLoginDisabled() bool { return false } -func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (*registrytypes.AuthenticateOKBody, error) { +func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (msg string, _ error) { // Some links documenting this: // - https://code.google.com/archive/p/mintty/issues/56 // - https://github.com/docker/docker/issues/15272 @@ -164,16 +164,17 @@ func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, de // will hit this if you attempt docker login from mintty where stdin // is a pipe, not a character based console. if (opts.user == "" || opts.password == "") && !dockerCli.In().IsTerminal() { - return nil, 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") } // If we're logging into the index server and the user didn't provide a username or password, use the device flow if serverAddress == registry.IndexServer && opts.user == "" && opts.password == "" && !isOauthLoginDisabled() { - response, err := loginWithDeviceCodeFlow(ctx, dockerCli) + var err error + msg, err = loginWithDeviceCodeFlow(ctx, dockerCli) // if the error represents a failure to initiate the device-code flow, // then we fallback to regular cli credentials login if !errors.Is(err, manager.ErrDeviceLoginStartFail) { - return response, err + return msg, err } _, _ = fmt.Fprint(dockerCli.Err(), "Failed to start web-based login - falling back to command line login...\n\n") } @@ -181,16 +182,16 @@ func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, de return loginWithUsernameAndPassword(ctx, dockerCli, opts, defaultUsername, serverAddress) } -func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (*registrytypes.AuthenticateOKBody, error) { +func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (msg string, _ error) { // Prompt user for credentials authConfig, err := command.PromptUserForCredentials(ctx, dockerCli, opts.user, opts.password, defaultUsername, serverAddress) if err != nil { - return nil, err + return "", err } response, err := loginWithRegistry(ctx, dockerCli.Client(), authConfig) if err != nil { - return nil, err + return "", err } if response.IdentityToken != "" { @@ -198,29 +199,29 @@ func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, op authConfig.IdentityToken = response.IdentityToken } if err = storeCredentials(dockerCli.ConfigFile(), authConfig); err != nil { - return nil, err + return "", err } - return response, nil + return response.Status, nil } -func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (*registrytypes.AuthenticateOKBody, error) { +func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (msg string, _ error) { store := dockerCli.ConfigFile().GetCredentialsStore(registry.IndexServer) authConfig, err := manager.NewManager(store).LoginDevice(ctx, dockerCli.Err()) if err != nil { - return nil, err + return "", err } response, err := loginWithRegistry(ctx, dockerCli.Client(), registrytypes.AuthConfig(*authConfig)) if err != nil { - return nil, err + return "", err } if err = storeCredentials(dockerCli.ConfigFile(), registrytypes.AuthConfig(*authConfig)); err != nil { - return nil, err + return "", err } - return response, nil + return response.Status, nil } func storeCredentials(cfg *configfile.ConfigFile, authConfig registrytypes.AuthConfig) error {