auth: add support for oauth device-code login #5344
Conversation
laurazard
force-pushed
the
auth-device-flow-pat
branch
from
2cb2bd2
to
18493da
Compare
| accessTokenKey = registry.IndexServer + "access-token" | ||
| refreshTokenKey = registry.IndexServer + "refresh-token" |
There was a problem hiding this comment.
using these for now so that Desktop can pick them up
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5344 +/- ##
==========================================
+ Coverage 61.45% 61.51% +0.05%
==========================================
Files 299 303 +4
Lines 20857 21130 +273
==========================================
+ Hits 12818 12998 +180
- Misses 7124 7203 +79
- Partials 915 929 +14 |
laurazard
force-pushed
the
auth-device-flow-pat
branch
from
01a9b43
to
6aedf2a
Compare
|
New login flow: Screen.Recording.2024-08-12.at.11.07.45.mov |
laurazard
force-pushed
the
auth-device-flow-pat
branch
2 times, most recently
from
569af38
to
204c974
Compare
laurazard
force-pushed
the
auth-device-flow-pat
branch
from
204c974
to
3b2e2e5
Compare
| @@ -53,6 +54,13 @@ func runLogout(_ context.Context, dockerCli command.Cli, serverAddress string) e | |||
| regsToLogout = append(regsToLogout, hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress) | |||
| } | |||
|
|
|||
| if isDefaultRegistry { | |||
There was a problem hiding this comment.
Don't think this is new, but we're now using it twice; wondering if we're missing a new check for isDefaultRegistry after the above branch does a credentials.ConvertToHostname(serverAddress)
(unrelated: I think the intent of ConvertToHostname is effectively "normalise storage key" - we need to look at some of that at some point - naming is confusing, and not always properly describes intent)
There was a problem hiding this comment.
I don't think so? But we could dig into that after this. Much like command/registry/login.go, this could use a bit of a cleanup.
There was a problem hiding this comment.
On another look, I guess a user could try to logout by doing docker logout index.docker.io and we should make that work.
There was a problem hiding this comment.
On another look, I guess a user could try to logout by doing docker logout index.docker.io and we should make that work.
Yes, that's roughly the scenario I had in mind. Technically it could go even further (log out for [http(s)://]registry-1.docker.io etc), but it looks like the current code only assumes "no registry" passed to mean docker hub. For the old code that made no real difference, as in that case it would at most do too many logouts (hostnameAddress, "http://"+hostnameAddress, "https://"+hostnameAddress), but for the new flow, it may be relevant that we would miss the manager.NewManager(store).Logout() call.
There was a problem hiding this comment.
So I started looking into this, and the current situation is – the web-based flow only triggers when the user runs either:
docker login, ordocker login https://index.docker.io/v1/
the following do not work:docker login index.docker.io- `docker login index.docker.io/v1/
docker login https://index.docker.io
For logout, the following trigger an oauth logout:
docker logout, ordocker logout https://index.docker.io/v1/
This seems consistent to me, but WDYT @thaJeztah?
laurazard
force-pushed
the
auth-device-flow-pat
branch
from
3b2e2e5
to
1ba762d
Compare
laurazard
force-pushed
the
auth-device-flow-pat
branch
2 times, most recently
from
2bb97a6
to
5dc3138
Compare
laurazard
force-pushed
the
auth-device-flow-pat
branch
from
5dc3138
to
58bf2cc
Compare
laurazard
force-pushed
the
auth-device-flow-pat
branch
from
67b69bd
to
69f8157
Compare
|
With the last commit laurazard@cd0023d:
|
There was a problem hiding this comment.
overall LGTM
- Can you double-check the comment about the
logout <registry name that is actually default registry>case - The "saving insecure" "once" message is probably fine w.r.t. concurrency, but not 100% sure if (e.g.) Piñata or other codebases may be executing this code; (maybe not too problematic?)
- w.r.t. the signature changes; yeah, on the fence; we could try to be on the safe side (keep the signature for now, but start making a pass at a more rigorous cleanup)
- ☝️ as part of cleanup, we really need to centralise some of the parsing/normalizing bits it's so much spread in way too many places 😞
laurazard
force-pushed
the
auth-device-flow-pat
branch
5 times, most recently
from
c2dec74
to
26a2860
Compare
|
JFYI I have a small improvement I want to do if possible so please ping me before merging. |
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 maintain compatibility with any clients manually accessing the credentials through `~/.docker/config.json` or via credential helpers, the added `OAuthManager` uses the retrieved access token to automatically generate a PAT with Hub, and store that in the credentials. Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
Signed-off-by: Laura Brehm <[email protected]>
laurazard
force-pushed
the
auth-device-flow-pat
branch
from
26a2860
to
c3fe7bc
Compare
|
Done! I wanted to prevent these errors: /usr/bin/xdg-open: 882: seamonkey: not found
/usr/bin/xdg-open: 882: mozilla: not found
/usr/bin/xdg-open: 882: epiphany: not found
/usr/bin/xdg-open: 882: konqueror: not found
/usr/bin/xdg-open: 882: chromium: not found
/usr/bin/xdg-open: 882: chromium-browser: not found
/usr/bin/xdg-open: 882: google-chrome: not found
/usr/bin/xdg-open: 882: www-browser: not found
/usr/bin/xdg-open: 882: links2: not found
/usr/bin/xdg-open: 882: elinks: not found
/usr/bin/xdg-open: 882: links: not found
/usr/bin/xdg-open: 882: lynx: not found
/usr/bin/xdg-open: 882: w3m: not found
xdg-open: no method available for opening ...from showing up if there are no such binaries (I run into this in my headless pet dev box). We already print the URL for the user, this just clutters up the output. Fun fact, the Github CLI runs into this as welll:
|
|
@thaJeztah, want to take a final look? |
- What I did
This commit adds support for the oauth device-code login flow when authenticating against the official registry.
Also included an (imo overdue 😅) refactor of
cli/command/registry/login.go.- How I did it
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.In order to preserve compatibility with other clients directly accessing credential helpers or
~/.docker/config.json, theOAuthManageralso generates a Hub PAT and saves it in the expected place.This is the first part of a body of work to revamp CLI/engine authentication. More work will follow up, adding the ability to refresh tokens to credential helpers, at which point we plan to stop using the PAT for authentication and instead rely on the credential helpers to refresh the token on access before returning a token that can be used for authentication to the caller.
- How to verify it
Try to login, do things that require authentication, and logout!
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)