Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for device-code oauth login #5244

Closed
wants to merge 7 commits into from
Closed

Add support for device-code oauth login #5244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0bfb1e5
auth: add support for oauth device-code login
laurazard Jul 8, 2024
e7dd302
Refactor `cli/command/registry`
laurazard Jul 8, 2024
fe1a817
oauth store: use different keys for oauth credentials
laurazard Jul 23, 2024
a7bf65b
oauth-store: print warning when we fail to revoke token
laurazard Aug 1, 2024
4ed0c6c
login: normalize `registry-1.docker.io`
laurazard Aug 1, 2024
bdf5e4a
tests: fix flaky `TestLoginDevice` test
laurazard Aug 1, 2024
0bac5a7
filestore: don't print warning multiple times
laurazard Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 20 additions & 21 deletions cli/command/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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
}
Expand All @@ -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{}
Expand All @@ -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:
Expand All @@ -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 {
Expand All @@ -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
Expand Down
144 changes: 114 additions & 30 deletions cli/command/registry/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ 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/internal/oauth/manager"
"github.com/docker/cli/cli/oauth"
registrytypes "github.com/docker/docker/api/types/registry"
"github.com/docker/docker/client"
"github.com/docker/docker/errdefs"
Expand Down Expand Up @@ -79,70 +81,152 @@ 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 {
if opts.serverAddress != "" &&
opts.serverAddress != registry.DefaultNamespace &&
opts.serverAddress != registry.DefaultRegistryHost {
serverAddress = opts.serverAddress
} else {
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 == "" {
err = command.ConfigureAuth(ctx, dockerCli, opts.user, opts.password, &authConfig, isDefaultRegistry)
response, err = loginUser(ctx, dockerCli, opts, authConfig.Username, serverAddress)
if err != nil {
return 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 != nil && response.Status != "" {
_, _ = fmt.Fprintln(dockerCli.Out(), response.Status)
}
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 {
_, _ = fmt.Fprintf(dockerCli.Err(), "Login did not succeed, error: %s\n", err)
}
}

if response.IdentityToken != "" {
authConfig.Password = ""
authConfig.IdentityToken = response.IdentityToken
}

creds := dockerCli.ConfigFile().GetCredentialsStore(serverAddress)
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 := manager.NewManager().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
}

func storeCredentials(dockerCli command.Cli, authConfig registrytypes.AuthConfig) error {
creds := dockerCli.ConfigFile().GetCredentialsStore(authConfig.ServerAddress)
if err := creds.Store(configtypes.AuthConfig(authConfig)); err != nil {
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) {
Expand Down
6 changes: 4 additions & 2 deletions cli/command/registry/login_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -213,7 +213,9 @@ func TestLoginTermination(t *testing.T) {

runErr := make(chan error)
go func() {
runErr <- runLogin(ctx, cli, loginOptions{})
runErr <- runLogin(ctx, cli, loginOptions{
user: "test-user",
})
}()

// Let the prompt get canceled by the context
Expand Down
5 changes: 2 additions & 3 deletions cli/command/registry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand All @@ -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"))
}
8 changes: 6 additions & 2 deletions cli/config/configfile/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/docker/cli/cli/config/credentials"
"github.com/docker/cli/cli/config/types"
"github.com/docker/cli/cli/internal/oauth/manager"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
)
Expand Down Expand Up @@ -254,10 +255,13 @@ func decodeAuth(authStr string) (string, string, error) {
// GetCredentialsStore returns a new credentials store from the settings in the
// configuration file
func (configFile *ConfigFile) GetCredentialsStore(registryHostname string) credentials.Store {
var credsStore credentials.Store
if helper := getConfiguredCredentialStore(configFile, registryHostname); helper != "" {
return newNativeStore(configFile, helper)
credsStore = newNativeStore(configFile, helper)
} else {
credsStore = credentials.NewFileStore(configFile)
}
return credentials.NewFileStore(configFile)
return credentials.NewOAuthStore(credsStore, manager.NewManager())
Comment on lines -260 to +264
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking now; instead of oauthStore (oauthStore.Get() etc) checking for if serverAddress != defaultRegistry {, should that be handled here instead; i.e. along these lines;

if serverAddress == defaultRegistry {
	credsStore = credentials.NewOAuthStore(credsStore, manager.NewManager())
}

return credsStore

Alternatively, we could have a way to set some helper to do this; that way we can keep the implementation out of here, and have some way to set "how to construct a helper")

if configFile.credsHelperProvider != nil {
	return configFile.credsHelperProvider(registryHostname, credsStore)
}

I wish we hadn't conflated the ConfigFile type (for serialising) with the ConfigFile functionality (saving, loading, manipulating) that much though. We need to look if we can get out of that at some point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I find it easier to reason about if we have a single point managing both. Otherwise, if someone does GetCredentialsStore("some-other-registry").GetAll() they're going to get the access-token and refresh-token specific keys that we shouldn't leak out/callers shouldn't concern themselves with. There might be other things that the wrapper does that should be abstracted away by the wrapper.

}
Comment on lines 257 to 265
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not something new, but I think your PR makes it more apparent, and ... maybe I'm rambling; I need to have a closer look; I'm somewhat starting to consider if constructing these stores should happen here, or if they should be constructed when initialising the configfile.

Basically; I need to look where this code is used; it may be imported elsewhere, which can be in code that's "non-interactive" (say k8s); for those, getting credentials through the OAuth flow isn't an option.

So I'm thinking if whether or not to use the interactive flow is something that must be set when constructing all of this 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for those, getting credentials through the OAuth flow isn't an option

True, but as you said, it's not different/new here. With the "old" flow, if there are no credentials, we also can't prompt the user to input them if running non-interactive. And if the user does have "oauth" credentials stored, even in a non-interactive environment, we want the oauth codepath to be executed so that the CLI can use the refresh token to fetch a fresh access token before returning them to whoever is calling it, right?

Basically, what setting this when initializing let us do/decide that we can't already?


// var for unit testing.
Expand Down
5 changes: 4 additions & 1 deletion cli/config/credentials/file_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/
`

var alreadyPrinted bool

// Store saves the given credentials in the file store.
func (c *fileStore) Store(authConfig types.AuthConfig) error {
authConfigs := c.file.GetAuthConfigs()
Expand All @@ -73,11 +75,12 @@ func (c *fileStore) Store(authConfig types.AuthConfig) error {
return err
}

if authConfig.Password != "" {
if !alreadyPrinted && authConfig.Password != "" {
// Display a warning if we're storing the users password (not a token).
//
// FIXME(thaJeztah): make output configurable instead of hardcoding to os.Stderr
_, _ = fmt.Fprintln(os.Stderr, fmt.Sprintf(unencryptedWarning, c.file.GetFilename()))
alreadyPrinted = true
}

return nil
Expand Down
Loading
Loading