Add support for device-code oauth login #5244

laurazard · 2024-07-08T12:43:15Z

Signed-off-by: Laura Brehm [email protected]

- What I did

This PR adds support for the oauth device-code login flow when authenticating against the official registry.

- How I did it

Added cli/internal/oauth, which contains code to manage interacting with the Docker OAuth tenant (login.docker.com), including launching the device-code flow, refreshing access using the refresh-token, and logging out.

In order to manage storage and retrieval of the oauth credentials, as well as ensuring that the access token retrieved from the credentials store is valid when accessed, this PR also introduces a new credential store wrapper, OAuthStore, which defers storage to an underlying store (such as the file or native/keychain/pass/wincred store) and gets a new access token using the refresh token when accessed.

Also included an (imo overdue 😅) refactor of cli/command/registry/login.go.

- How to verify it

Try to login, do things that require authentication, and logout!

- Description for the changelog

Added support for device-code flow login when authenticating to the official registry.

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2024-07-08T14:32:41Z

Codecov Report

Attention: Patch coverage is 70.61995% with 109 lines in your changes missing coverage. Please review.

Project coverage is 61.65%. Comparing base (0052d2c) to head (0bac5a7).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5244      +/-   ##
==========================================
+ Coverage   61.45%   61.65%   +0.19%     
==========================================
  Files         299      304       +5     
  Lines       20857    21180     +323     
==========================================
+ Hits        12818    13058     +240     
- Misses       7124     7192      +68     
- Partials      915      930      +15

laurazard · 2024-07-09T10:17:40Z

Screen.Recording.2024-07-09.at.11.16.43.mov

cli/internal/oauth/util/util.go

cli/config/credentials/oauth_store.go

cli/internal/oauth/util/client.go

cli/internal/oauth/manager/manager.go

cli/internal/oauth/api/api.go

cli/internal/oauth/manager/manager.go

cli/internal/oauth/util/util.go

laurazard · 2024-07-31T13:24:16Z

Just tested that vendoring the changes into buildkit/buildx without any additional changes allows pulling images from private repos while building.

thaJeztah

Left some blurbs; also playing with some options in those areas.

Overall (from a user perspective) it all works pretty neat. Some notes I had while playing;

I think we may be missing some normalization of registry-names still (likely not something new); for example;

docker login registry-1.docker.io
Username:

But docker.io works;

docker login docker.io

You will be signed in using a web-based login.
To sign in with credentials on the command line, use 'docker login -u <username>'
...

Probably not problematic, but I noticed that a mix of "logout" using the old CLI, and login using the "new" CLI showed this;

# this is the old CLI:
docker logout
Removing login credentials for https://index.docker.io/v1/

# login with the new CLI:
./build/docker login
Authenticating with existing credentials...

You will be signed in using a web-based login.
To sign in with credentials on the command line, use 'docker login -u <username>'
...

Notice that it prints Authenticating with existing credentials...; I think what happens is that the old CLI only removes credentials it knows about, but then leaves the new ones. The new CLI (probably) sees that there's credentials, but then they don't work, so it continues the new path to interactively authenticate. Definitely not problematic AFAICS, just thinking if we can detect that situation.

Also had a try what happens if hub is not accessible when logging out / in (making sure we don't fully hang);

docker login docker.io

# You will be signed in using a web-based login.
# ...

# block login.docker.com
echo '127.0.0.1 login.docker.com' >> /etc/hosts

docker logout
Removing login credentials for https://index.docker.io/v1/

docker login
login failed: Post "https://login.docker.com/oauth/device/code": dial tcp 127.0.0.1:443: connect: connection refused

Slightly wondering if we should print a message if logout failed; we continue removing the credentials from the store, but RevokeToken would've failed; is that something important to log (i.e., if the token was found by someone, it could still be valid?).

thaJeztah · 2024-07-31T13:10:12Z

cli/config/configfile/file.go

-	return credentials.NewFileStore(configFile)
+	return credentials.NewOAuthStore(credsStore, manager.NewManager())


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.

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.

cli/command/cli.go

cli/command/registry/login.go

thaJeztah · 2024-07-31T15:41:36Z

Oh! Erm, this one I forgot to post; something I didn't consider in my changes I guess; we store multiple times now, and each of them prints a warning 🙈

docker login

You will be signed in using a web-based login.
To sign in with credentials on the command line, use 'docker login -u <username>'

Your one-time device confirmation code is: XXXXXX

Press ENTER to open the browser.
Or open the URL manually: https://login.docker.com/activate

Waiting for authentication in the browser...

WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/


WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/

Login Succeeded

This one was slightly more surprising; after Having signed in, running docker login does succeed (Authenticating with existing credentials...), but also prints three warnings.

docker login

WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/


WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/

Authenticating with existing credentials...

WARNING! Your credentials are stored unencrypted in '/root/.docker/config.json'.
Configure a credential helper to remove this warning. See
https://docs.docker.com/go/credential-store/

Login Succeeded

laurazard · 2024-08-01T09:10:47Z

Re: the warning now printing multiple times, I'm wondering how to address that. We store twice (for both keys, now) and on login, since we refresh the token and store the new one.

laurazard · 2024-08-01T09:31:00Z

Slightly wondering if we should print a message if logout failed; we continue removing the credentials from the store, but RevokeToken would've failed; is that something important to log (i.e., if the token was found by someone, it could still be valid?).

This is a good question – it kind of sucks in this case since we'd be removing the token, effectively making it so that the user no longer has the token to revoke it. Definitely should log a message in this case.

laurazard · 2024-08-01T09:52:14Z

cli/config/credentials/oauth_store.go

+		err = o.manager.Logout(ctx, refreshTokenAuth.Password)
+		if err != nil {
+			// todo(laurazard): actual message here
+			fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.")


Ideas for a better message here?

@dvdksn thoughts on this one?

What is the user action that leads here? docker logout myregistry.example.com?

This only applies to logging out from the official registry, so either docker logout or docker logout index.docker.io, docker logout registry-1.docker.io

laurazard · 2024-08-05T11:09:01Z

@thaJeztah also wondering about how we'll fix the multiple file_store warnings, now that the warnings live in the store 😅 Ideas?

I did a hacky solution in be4ecc3, but open to alternatives.

cli/internal/oauth/manager/manager.go

cli/config/credentials/oauth_store.go

vvoland · 2024-08-05T15:09:33Z

cli/config/credentials/oauth_store.go

+		err = o.manager.Logout(ctx, refreshTokenAuth.Password)
+		if err != nil {
+			// todo(laurazard): actual message here
+			fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.")


nit:

Suggested change

fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.")

_, _ = fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.")

(to be consistent with other Fprints)

What about making this a warning? I don't recall if we have a helper function for styling, but something like:

Suggested change

fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.")

_, _ = fmt.Fprint(os.Stderr, "Warning: You have been logged out successfully, but there was a failure to revoke the OAuth refresh token with the tenant.")

The only problem with the above is that it doesn't really say anything about manually addressing that failure. A more complete, but unattractively longer, version could be:

Suggested change

fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.")

_, _ = fmt.Fprint(os.Stderr, "Warning: You have been logged out successfully, but there was a failure to revoke the OAuth refresh token with the tenant. You may want to manually revoke the token through your OAuth provider's settings.")

I don't think the You have been logged out successfully works super well with the messages we have here and here.

In those, we don't say "logging out", but rather could not erase credentials or Removing login credentials. @dvdksn

Wasn't aware of the prior art 👍🏻 In that case s/You have been logged out successfully/Credentials erased successfully/ for consistency.

cli/internal/oauth/manager/manager.go

cli/internal/oauth/manager/manager_test.go

This commit adds support for the oauth [device-code](https://auth0.com/docs/get-started/authentication-and-authorization-flow/device-authorization-flow) login flow when authenticating against the official registry. This is achieved by adding `cli/internal/oauth`, which contains code to manage interacting with the Docker OAuth tenant (`login.docker.com`), including launching the device-code flow, refreshing access using the refresh-token, and logging out. The `OAuthManager` introduced here is also made available through the `command.Cli` interface method `OAuthManager()`. In order to manage storage and retrieval of the oauth credentials, as well as ensuring that the access token retrieved from the credentials store is valid when accessed, this commit also introduces a new credential store wrapper, `OAuthStore`, which defers storage to an underlying store (such as the file or native/keychain/pass/wincred store) and gets a new access token using the refresh token when accessed. Signed-off-by: Laura Brehm <[email protected]>

Signed-off-by: Laura Brehm <[email protected]>

laurazard · 2024-08-13T14:30:56Z

Closed in favor of #5344

laurazard changed the title ~~🚧 Add support for device-code oauth login 🚧~~ 🚧 Add support for device-code oauth login (alternative 1) 🚧 Jul 8, 2024

This was referenced Jul 8, 2024

🚧 Add support for device-code flow login (alternative 2) 🚧 #5245

Closed

🚧 Add support for OAuth device-code login 🚧 #5221

Closed

laurazard force-pushed the auth-device-flow-2 branch 4 times, most recently from 5368212 to 7a1da8e Compare July 8, 2024 14:29

laurazard force-pushed the auth-device-flow-2 branch 9 times, most recently from dd7d670 to 4674044 Compare July 9, 2024 09:07

laurazard added the area/authentication label Jul 9, 2024

laurazard force-pushed the auth-device-flow-2 branch 2 times, most recently from b392301 to d720959 Compare July 9, 2024 10:00

laurazard self-assigned this Jul 9, 2024

laurazard marked this pull request as ready for review July 9, 2024 10:19

laurazard changed the title ~~🚧 Add support for device-code oauth login (alternative 1) 🚧~~ Add support for device-code oauth login (alternative 1) Jul 9, 2024

Benehiko reviewed Jul 10, 2024

View reviewed changes

cli/internal/oauth/manager/manager.go Outdated Show resolved Hide resolved

cli/internal/oauth/util/util.go Outdated Show resolved Hide resolved

jpbriend reviewed Jul 15, 2024

View reviewed changes

cli/internal/oauth/util/util.go Outdated Show resolved Hide resolved

laurazard force-pushed the auth-device-flow-2 branch 3 times, most recently from 0be6989 to f937eeb Compare July 17, 2024 08:30

laurazard requested a review from vvoland July 29, 2024 10:16

laurazard force-pushed the auth-device-flow-2 branch 3 times, most recently from 0d948ee to 34ee692 Compare July 31, 2024 11:03

thaJeztah reviewed Jul 31, 2024

View reviewed changes

laurazard force-pushed the auth-device-flow-2 branch 2 times, most recently from 9ef2396 to 78a84c1 Compare August 1, 2024 09:09

laurazard commented Aug 1, 2024

View reviewed changes

laurazard requested a review from thaJeztah August 1, 2024 13:05

dvdksn reviewed Aug 5, 2024

View reviewed changes

cli/internal/oauth/manager/manager.go Outdated Show resolved Hide resolved

laurazard force-pushed the auth-device-flow-2 branch from be4ecc3 to 6a838e2 Compare August 5, 2024 13:34

vvoland reviewed Aug 5, 2024

View reviewed changes

laurazard force-pushed the auth-device-flow-2 branch 3 times, most recently from b884d32 to 1b798cf Compare August 8, 2024 11:34

laurazard added 7 commits August 8, 2024 12:39

Refactor cli/command/registry

e7dd302

Signed-off-by: Laura Brehm <[email protected]>

oauth store: use different keys for oauth credentials

fe1a817

Signed-off-by: Laura Brehm <[email protected]>

oauth-store: print warning when we fail to revoke token

a7bf65b

Signed-off-by: Laura Brehm <[email protected]>

login: normalize registry-1.docker.io

4ed0c6c

Signed-off-by: Laura Brehm <[email protected]>

tests: fix flaky TestLoginDevice test

bdf5e4a

Signed-off-by: Laura Brehm <[email protected]>

filestore: don't print warning multiple times

0bac5a7

Signed-off-by: Laura Brehm <[email protected]>

laurazard force-pushed the auth-device-flow-2 branch from 1b798cf to 0bac5a7 Compare August 8, 2024 11:39

laurazard closed this Aug 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for device-code oauth login #5244

Add support for device-code oauth login #5244

laurazard commented Jul 8, 2024 •

edited

Loading

codecov-commenter commented Jul 8, 2024 •

edited

Loading

laurazard commented Jul 9, 2024

laurazard commented Jul 31, 2024

thaJeztah left a comment

thaJeztah Jul 31, 2024

laurazard Jul 31, 2024

thaJeztah commented Jul 31, 2024

laurazard commented Aug 1, 2024

laurazard commented Aug 1, 2024

laurazard Aug 1, 2024

laurazard Aug 5, 2024

dvdksn Aug 5, 2024

laurazard Aug 5, 2024 •

edited

Loading

laurazard commented Aug 5, 2024 •

edited

Loading

vvoland Aug 5, 2024 •

edited

Loading

dvdksn Aug 6, 2024

laurazard Aug 8, 2024

dvdksn Aug 8, 2024

laurazard commented Aug 13, 2024

		return credentials.NewFileStore(configFile)
		return credentials.NewOAuthStore(credsStore, manager.NewManager())

	fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.")
	_, _ = fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.")

	fmt.Fprint(os.Stderr, "Failed to revoke refresh token with tenant.. Credentials will still be erased.")
	_, _ = fmt.Fprint(os.Stderr, "Warning: You have been logged out successfully, but there was a failure to revoke the OAuth refresh token with the tenant.")

Add support for device-code oauth login #5244

Add support for device-code oauth login #5244

Conversation

laurazard commented Jul 8, 2024 • edited Loading

codecov-commenter commented Jul 8, 2024 • edited Loading

Codecov Report

laurazard commented Jul 9, 2024

laurazard commented Jul 31, 2024

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jul 31, 2024

laurazard commented Aug 1, 2024

laurazard commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurazard Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

laurazard commented Aug 5, 2024 • edited Loading

vvoland Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurazard commented Aug 13, 2024

laurazard commented Jul 8, 2024 •

edited

Loading

codecov-commenter commented Jul 8, 2024 •

edited

Loading

laurazard Aug 5, 2024 •

edited

Loading

laurazard commented Aug 5, 2024 •

edited

Loading

vvoland Aug 5, 2024 •

edited

Loading