From b3923017140d117a2b7804434a2c20904c4cc673 Mon Sep 17 00:00:00 2001 From: Laura Brehm Date: Mon, 8 Jul 2024 23:48:13 +0100 Subject: [PATCH] Refactor `cli/command/registry` Signed-off-by: Laura Brehm --- cli/command/registry.go | 41 +++-- cli/command/registry/login.go | 171 +++++++++++++-------- cli/command/registry/login_test.go | 2 +- cli/command/registry_test.go | 5 +- cli/config/credentials/oauth_store.go | 16 +- cli/config/credentials/oauth_store_test.go | 10 +- cli/internal/oauth/util/util.go | 13 -- cli/oauth/jwt.go | 16 ++ 8 files changed, 164 insertions(+), 110 deletions(-) diff --git a/cli/command/registry.go b/cli/command/registry.go index 89fa0cca5953..db205a2165b7 100644 --- a/cli/command/registry.go +++ b/cli/command/registry.go @@ -29,8 +29,7 @@ func RegistryAuthenticationPrivilegedFunc(cli Cli, index *registrytypes.IndexInf return func(ctx context.Context) (string, error) { 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) + authConfig, err := GetDefaultAuthConfig(cli.ConfigFile(), true, indexServer) if err != nil { fmt.Fprintf(cli.Err(), "Unable to retrieve stored credentials for %s, error: %s.\n", indexServer, err) } @@ -41,7 +40,7 @@ func RegistryAuthenticationPrivilegedFunc(cli Cli, index *registrytypes.IndexInf default: } - err = ConfigureAuth(ctx, cli, "", "", &authConfig, isDefaultRegistry) + authConfig, err = ConfigureAuth(ctx, cli, "", "", authConfig.Username, indexServer) if err != nil { return "", err } @@ -67,8 +66,8 @@ func ResolveAuthConfig(cfg *configfile.ConfigFile, index *registrytypes.IndexInf // 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(cfg *configfile.ConfigFile, checkCredStore bool, serverAddress string, isDefaultRegistry bool) (registrytypes.AuthConfig, error) { - if !isDefaultRegistry { +func GetDefaultAuthConfig(cfg *configfile.ConfigFile, checkCredStore bool, serverAddress string) (registrytypes.AuthConfig, error) { + if serverAddress != registry.IndexServer { serverAddress = credentials.ConvertToHostname(serverAddress) } authconfig := configtypes.AuthConfig{} @@ -87,7 +86,7 @@ func GetDefaultAuthConfig(cfg *configfile.ConfigFile, checkCredStore bool, serve } // ConfigureAuth handles prompting of user's username and password if needed -func ConfigureAuth(ctx context.Context, cli Cli, flUser, flPassword string, authconfig *registrytypes.AuthConfig, isDefaultRegistry bool) error { +func ConfigureAuth(ctx context.Context, cli Cli, flUser, flPassword, defaultUsername, serverAddress string) (authConfig registrytypes.AuthConfig, err error) { // On Windows, force the use of the regular OS stdin stream. // // See: @@ -108,10 +107,11 @@ func ConfigureAuth(ctx context.Context, cli Cli, flUser, flPassword string, auth // 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 errors.Errorf("Error: Cannot perform an interactive login from a non TTY device") + return authConfig, errors.Errorf("Error: Cannot perform an interactive login from a non TTY device") } - authconfig.Username = strings.TrimSpace(authconfig.Username) + isDefaultRegistry := serverAddress == registry.IndexServer + defaultUsername = strings.TrimSpace(defaultUsername) if flUser = strings.TrimSpace(flUser); flUser == "" { if isDefaultRegistry { @@ -124,44 +124,43 @@ func ConfigureAuth(ctx context.Context, cli Cli, flUser, flPassword string, auth } var prompt string - if authconfig.Username == "" { + if defaultUsername == "" { prompt = "Username: " } else { - prompt = fmt.Sprintf("Username (%s): ", authconfig.Username) + prompt = fmt.Sprintf("Username (%s): ", defaultUsername) } - var err error flUser, err = PromptForInput(ctx, cli.In(), cli.Out(), prompt) if err != nil { - return err + return authConfig, err } if flUser == "" { - flUser = authconfig.Username + flUser = defaultUsername } } if flUser == "" { - return errors.Errorf("Error: Non-null Username Required") + return authConfig, errors.Errorf("Error: Non-null Username Required") } if flPassword == "" { restoreInput, err := DisableInputEcho(cli.In()) if err != nil { - return err + return authConfig, err } defer restoreInput() flPassword, err = PromptForInput(ctx, cli.In(), cli.Out(), "Password: ") if err != nil { - return err + return authConfig, err } fmt.Fprint(cli.Out(), "\n") if flPassword == "" { - return errors.Errorf("Error: Password Required") + return authConfig, errors.Errorf("Error: Password Required") } } - authconfig.Username = flUser - authconfig.Password = flPassword - - return nil + authConfig.Username = flUser + authConfig.Password = flPassword + authConfig.ServerAddress = serverAddress + return authConfig, nil } // RetrieveAuthTokenFromImage retrieves an encoded auth token given a complete diff --git a/cli/command/registry/login.go b/cli/command/registry/login.go index 068777604bab..3140bd25af24 100644 --- a/cli/command/registry/login.go +++ b/cli/command/registry/login.go @@ -10,6 +10,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/completion" configtypes "github.com/docker/cli/cli/config/types" + "github.com/docker/cli/cli/oauth" registrytypes "github.com/docker/docker/api/types/registry" "github.com/docker/docker/client" "github.com/docker/docker/errdefs" @@ -100,14 +101,13 @@ func verifyloginOptions(dockerCli command.Cli, opts *loginOptions) error { return nil } -func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) error { //nolint:gocyclo - clnt := dockerCli.Client() +func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) error { if err := verifyloginOptions(dockerCli, &opts); err != nil { return err } var ( serverAddress string - response registrytypes.AuthenticateOKBody + response *registrytypes.AuthenticateOKBody ) if opts.serverAddress != "" && opts.serverAddress != registry.DefaultNamespace { serverAddress = opts.serverAddress @@ -115,68 +115,122 @@ func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) err serverAddress = registry.IndexServer } - isDefaultRegistry := serverAddress == registry.IndexServer - authConfig, err := command.GetDefaultAuthConfig(dockerCli.ConfigFile(), opts.user == "" && opts.password == "", serverAddress, isDefaultRegistry) + // attempt login with current (stored) credentials + authConfig, err := command.GetDefaultAuthConfig(dockerCli.ConfigFile(), opts.user == "" && opts.password == "", serverAddress) if err == nil && authConfig.Username != "" && authConfig.Password != "" { - response, err = loginWithCredStoreCreds(ctx, dockerCli, &authConfig) + response, 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 == "" { - if isDefaultRegistry && opts.user == "" && opts.password == "" { - // todo(laurazard: clean this up - tokenRes, err := dockerCli.OAuthManager().LoginDevice(ctx, dockerCli.Err()) - if err != nil { - return err - } - authConfig.Username = tokenRes.Claims.Domain.Username - authConfig.Password = tokenRes.AccessToken - authConfig.Email = tokenRes.Claims.Domain.Email - authConfig.ServerAddress = serverAddress - - response, err = clnt.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 err - } + response, err = loginUser(ctx, dockerCli, opts, authConfig.Username, serverAddress) + if err != nil { + return err + } + } - authConfig.Password = authConfig.Password + ".." + tokenRes.RefreshToken + if response.Status != "" { + _, _ = fmt.Fprintln(dockerCli.Out(), response.Status) + } + return nil +} - creds := dockerCli.ConfigFile().GetCredentialsStore(serverAddress) - if err := creds.Store(configtypes.AuthConfig(authConfig)); err != nil { - return errors.Errorf("Error saving credentials: %v", err) - } - return nil +func loginWithStoredCredentials(ctx context.Context, dockerCli command.Cli, authConfig registrytypes.AuthConfig) (*registrytypes.AuthenticateOKBody, error) { + _, _ = fmt.Fprintf(dockerCli.Out(), "Authenticating with existing credentials...\n") + response, err := dockerCli.Client().RegistryLogin(ctx, authConfig) + if err != nil { + if errdefs.IsUnauthorized(err) { + _, _ = fmt.Fprintf(dockerCli.Err(), "Stored credentials invalid or expired\n") } else { - err = command.ConfigureAuth(ctx, dockerCli, opts.user, opts.password, &authConfig, isDefaultRegistry) - if err != nil { - return err - } + _, _ = fmt.Fprintf(dockerCli.Err(), "Login did not succeed, error: %s\n", err) } + } - response, err = clnt.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 err - } + if response.IdentityToken != "" { + authConfig.Password = "" + authConfig.IdentityToken = response.IdentityToken + } + + if err := storeCredentials(dockerCli, authConfig); err != nil { + return nil, err + } + + return &response, err +} + +func loginUser(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (*registrytypes.AuthenticateOKBody, error) { + // 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 == "" { + return loginWithDeviceCodeFlow(ctx, dockerCli) + } else { + return loginWithUsernameAndPassword(ctx, dockerCli, opts, defaultUsername, serverAddress) } +} + +func loginWithUsernameAndPassword(ctx context.Context, dockerCli command.Cli, opts loginOptions, defaultUsername, serverAddress string) (*registrytypes.AuthenticateOKBody, error) { + // Prompt user for credentials + authConfig, err := command.ConfigureAuth(ctx, dockerCli, opts.user, opts.password, defaultUsername, serverAddress) + if err != nil { + return nil, err + } + + response, err := loginWithRegistry(ctx, dockerCli, authConfig) + if err != nil { + return nil, err + } + if response.IdentityToken != "" { authConfig.Password = "" authConfig.IdentityToken = response.IdentityToken } + if err = storeCredentials(dockerCli, authConfig); err != nil { + return nil, err + } + + return &response, nil +} + +func loginWithDeviceCodeFlow(ctx context.Context, dockerCli command.Cli) (*registrytypes.AuthenticateOKBody, error) { + authConfig, refreshToken, err := getOAuthCredentials(ctx, dockerCli) + if err != nil { + return nil, err + } + + response, err := loginWithRegistry(ctx, dockerCli, authConfig) + if err != nil { + return nil, err + } + + authConfig.Password = oauth.ConcatTokens(authConfig.Password, refreshToken) + if err = storeCredentials(dockerCli, authConfig); err != nil { + return nil, err + } + + return &response, nil +} + +func getOAuthCredentials(ctx context.Context, dockerCli command.Cli) (authConfig registrytypes.AuthConfig, refreshToken string, err error) { + tokenRes, err := dockerCli.OAuthManager().LoginDevice(ctx, dockerCli.Err()) + if err != nil { + return authConfig, "", err + } + + return registrytypes.AuthConfig{ + Username: tokenRes.Claims.Domain.Username, + Password: tokenRes.AccessToken, + Email: tokenRes.Claims.Domain.Email, + ServerAddress: registry.IndexServer, + }, tokenRes.RefreshToken, nil +} - creds := dockerCli.ConfigFile().GetCredentialsStore(serverAddress) +func storeCredentials(dockerCli command.Cli, authConfig registrytypes.AuthConfig) error { + creds := dockerCli.ConfigFile().GetCredentialsStore(authConfig.ServerAddress) // todo(laurazard): this will no longer trigger even when the store is a file store store, isDefault := creds.(isFileStore) // Display a warning if we're storing the users password (not a token) if isDefault && authConfig.Password != "" { - err = displayUnencryptedWarning(dockerCli, store.GetFilename()) + err := displayUnencryptedWarning(dockerCli, store.GetFilename()) if err != nil { return err } @@ -186,24 +240,21 @@ func runLogin(ctx context.Context, dockerCli command.Cli, opts loginOptions) err return errors.Errorf("Error saving credentials: %v", err) } - if response.Status != "" { - fmt.Fprintln(dockerCli.Out(), response.Status) - } return nil } -func loginWithCredStoreCreds(ctx context.Context, dockerCli command.Cli, authConfig *registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) { - fmt.Fprintf(dockerCli.Out(), "Authenticating with existing credentials...\n") - cliClient := dockerCli.Client() - response, err := cliClient.RegistryLogin(ctx, *authConfig) +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 { - if errdefs.IsUnauthorized(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 registrytypes.AuthenticateOKBody{}, err } - return response, err + + return response, nil } func loginClientSide(ctx context.Context, auth registrytypes.AuthConfig) (registrytypes.AuthenticateOKBody, error) { diff --git a/cli/command/registry/login_test.go b/cli/command/registry/login_test.go index a28191994bb6..4c3003cf4b14 100644 --- a/cli/command/registry/login_test.go +++ b/cli/command/registry/login_test.go @@ -74,7 +74,7 @@ func TestLoginWithCredStoreCreds(t *testing.T) { cli := test.NewFakeCli(&fakeClient{}) errBuf := new(bytes.Buffer) cli.SetErr(streams.NewOut(errBuf)) - loginWithCredStoreCreds(ctx, cli, &tc.inputAuthConfig) + loginWithStoredCredentials(ctx, cli, tc.inputAuthConfig) outputString := cli.OutBuffer().String() assert.Check(t, is.Equal(tc.expectedMsg, outputString)) errorString := errBuf.String() diff --git a/cli/command/registry_test.go b/cli/command/registry_test.go index 65acd069e2bb..46b260573464 100644 --- a/cli/command/registry_test.go +++ b/cli/command/registry_test.go @@ -61,7 +61,7 @@ func TestGetDefaultAuthConfig(t *testing.T) { } for _, tc := range testCases { serverAddress := tc.inputServerAddress - authconfig, err := command.GetDefaultAuthConfig(cfg, tc.checkCredStore, serverAddress, serverAddress == "https://index.docker.io/v1/") + authconfig, err := command.GetDefaultAuthConfig(cfg, tc.checkCredStore, serverAddress) assert.NilError(t, err) assert.Check(t, is.DeepEqual(tc.expectedAuthConfig, authconfig)) } @@ -75,8 +75,7 @@ func TestGetDefaultAuthConfig_HelperError(t *testing.T) { expectedAuthConfig := registry.AuthConfig{ ServerAddress: serverAddress, } - const isDefaultRegistry = false // registry is not "https://index.docker.io/v1/" - authconfig, err := command.GetDefaultAuthConfig(cfg, true, serverAddress, isDefaultRegistry) + authconfig, err := command.GetDefaultAuthConfig(cfg, true, serverAddress) assert.Check(t, is.DeepEqual(expectedAuthConfig, authconfig)) assert.Check(t, is.ErrorContains(err, "docker-credential-fake-does-not-exist")) } diff --git a/cli/config/credentials/oauth_store.go b/cli/config/credentials/oauth_store.go index 8e68cc9b7b3a..d85424139fcd 100644 --- a/cli/config/credentials/oauth_store.go +++ b/cli/config/credentials/oauth_store.go @@ -3,7 +3,6 @@ package credentials import ( "context" "errors" - "strings" "time" "github.com/docker/cli/cli/config/types" @@ -49,6 +48,7 @@ func (c *oauthStore) Get(serverAddress string) (types.AuthConfig, error) { tokenRes, err := c.parseToken(auth.Password) // if the password is not a token, return the auth config as is if err != nil { + //nolint:nilerr return auth, nil } @@ -111,13 +111,11 @@ func (c *oauthStore) Store(auth types.AuthConfig) error { } func (c *oauthStore) parseToken(password string) (oauth.TokenResult, error) { - parts := strings.Split(password, "..") - if len(parts) != 2 { + accessToken, refreshToken, err := oauth.SplitTokens(password) + if err != nil { return oauth.TokenResult{}, errors.New("failed to parse token") } - accessToken := parts[0] - refreshToken := parts[1] - claims, err := oauth.GetClaims(parts[0]) + claims, err := oauth.GetClaims(accessToken) if err != nil { return oauth.TokenResult{}, err } @@ -131,13 +129,9 @@ func (c *oauthStore) parseToken(password string) (oauth.TokenResult, error) { func (c *oauthStore) storeInBackingStore(tokenRes oauth.TokenResult) error { auth := types.AuthConfig{ Username: tokenRes.Claims.Domain.Username, - Password: c.concat(tokenRes.AccessToken, tokenRes.RefreshToken), + Password: oauth.ConcatTokens(tokenRes.AccessToken, tokenRes.RefreshToken), Email: tokenRes.Claims.Domain.Email, ServerAddress: registry.IndexServer, } return c.backingStore.Store(auth) } - -func (c *oauthStore) concat(accessToken, refreshToken string) string { - return accessToken + ".." + refreshToken -} diff --git a/cli/config/credentials/oauth_store_test.go b/cli/config/credentials/oauth_store_test.go index 3fbc79aac3c6..19287f8edc9b 100644 --- a/cli/config/credentials/oauth_store_test.go +++ b/cli/config/credentials/oauth_store_test.go @@ -337,6 +337,7 @@ func TestStore(t *testing.T) { } err := s.Store(auth) assert.NilError(t, err) + assert.Check(t, is.Len(f.GetAuthConfigs(), 1)) }) @@ -353,7 +354,14 @@ func TestStore(t *testing.T) { } err := s.Store(auth) assert.NilError(t, err) - assert.Check(t, is.Len(f.GetAuthConfigs(), 0)) + + assert.Check(t, is.Len(f.GetAuthConfigs(), 1)) + assert.DeepEqual(t, f.GetAuthConfigs()[registry.IndexServer], types.AuthConfig{ + Username: "foo", + Password: validNotExpiredToken + "..refresh-token", + Email: "foo@example.com", + ServerAddress: registry.IndexServer, + }) }) }) diff --git a/cli/internal/oauth/util/util.go b/cli/internal/oauth/util/util.go index c170e77a4371..ac18953af008 100644 --- a/cli/internal/oauth/util/util.go +++ b/cli/internal/oauth/util/util.go @@ -4,21 +4,8 @@ import ( "errors" "os/exec" "runtime" - "time" - - "github.com/docker/cli/cli/oauth" - "github.com/go-jose/go-jose/v3/jwt" ) -// IsExpired returns whether the claims are expired or not. -func IsExpired(claims oauth.Claims) bool { - err := claims.Validate(jwt.Expected{ - Time: time.Now().UTC(), - }) - - return errors.Is(err, jwt.ErrExpired) -} - // OpenBrowser opens the specified URL in a browser based on OS. func OpenBrowser(url string) (err error) { switch runtime.GOOS { diff --git a/cli/oauth/jwt.go b/cli/oauth/jwt.go index a94eff9da4f3..17b079cd9977 100644 --- a/cli/oauth/jwt.go +++ b/cli/oauth/jwt.go @@ -1,6 +1,9 @@ package oauth import ( + "errors" + "strings" + "github.com/go-jose/go-jose/v3/jwt" ) @@ -97,3 +100,16 @@ func GetClaims(accessToken string) (claims Claims, err error) { return } + +func ConcatTokens(accessToken, refreshToken string) string { + return accessToken + ".." + refreshToken +} + +func SplitTokens(token string) (accessToken, refreshToken string, err error) { + parts := strings.Split(token, "..") + if len(parts) != 2 { + return "", "", errors.New("failed to parse token") + } + + return parts[0], parts[1], nil +}