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

laurazard · 2024-07-03T10:28:48Z

- What I did

WIP PR for discussion that adds support for the device-code flow OAuth login, when authenticating against the official registry.

I'll follow up with another PR to explore implementing this at the credential store layer/using a new credential store.

- How I did it

Added cli/internal/oauth, which includes a new OAuthManager that is capable of fulfilling a device-code flow against the official registry.

Fetching credentials remains the same, as the returned access token can be transparently used in place of the password when authenticating.

Login and logout flows now defer to OAuthManager when running against the official registry.

- How to verify it

Run docker login, and check your credentials store to find the stored access tokens. Attempt to do an operation which requires authentication (such as pushing an image to your repo) and verify it works.

- Description for the changelog

Added support for the device-code flow OAuth login, when authenticating against the official registry.

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

codecov-commenter · 2024-07-03T11:46:06Z

Codecov Report

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

Project coverage is 61.39%. Comparing base (6abed4e) to head (4029dbc).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5221      +/-   ##
==========================================
- Coverage   61.48%   61.39%   -0.09%     
==========================================
  Files         298      302       +4     
  Lines       20813    21037     +224     
==========================================
+ Hits        12797    12916     +119     
- Misses       7104     7192      +88     
- Partials      912      929      +17

cli/internal/oauth/manager/manager.go

laurazard · 2024-07-03T14:20:40Z

Recording with the UX/copy changes:

Screen.Recording.2024-07-03.at.15.12.35.mov

dvdksn · 2024-07-03T16:06:11Z

you're probably doing this already... but can we hide the "Authenticating with existing credentials" (and error) bit if that isn't happening?

dvdksn · 2024-07-03T16:06:28Z

misclick 😓

laurazard · 2024-07-03T16:20:26Z

you're probably doing this already... but can we hide the "Authenticating with existing credentials" (and error) bit if that isn't happening?

Yep! That message should only show up when we're actually logging in with existing credentials, what happened in that video was misleading because in truth we already had credentials, the "first" login (with those credentials) just failed for different reasons. I'll update the recording.

thaJeztah

🙈 also gave it a quick try what it would look like without that host.Info() call;

quick try without host.Info() laurazard/cli#2

thaJeztah · 2024-07-04T09:08:49Z

cli/internal/oauth/manager/util.go

+)
+
+func NewManager(store credentials.Store) (*OAuthManager, error) {
+	hostinfo, err := host.Info()


I was, erm, curious where all the extra dependencies originated from 🙈;

github.com/tklauser/go-sysconf

github.com/tklauser/go-sysconf (number of online CPUs?)

github.com/shoenig/go-m1cpu (CPU info for m1 CPUs)

github.com/power-devops/perfstat (handling of libperfstat ??

github.com/go-ole/go-ole (loading of Windows OLE objects)

github.com/yusufpapurcu/wmi (WQL interface to Windows Management Instrumentation (WMI))

And those all look to be related to this single host.Info(); gopsutil/host.Info() is doing a lot of things; https://github.com/shirou/gopsutil/blob/865b8c3f58d6ee4c47dab074fbf3c4b037abfd96/host/host.go#L65-L117

Getting information about the distro-version, which;

on Linux involves trying to parse /etc/os-release and more; https://github.com/shirou/gopsutil/blob/865b8c3f58d6ee4c47dab074fbf3c4b037abfd96/host/host_linux.go#L173-L321

on macOS involves shelling out to sw_vers -productVersion https://github.com/shirou/gopsutil/blob/865b8c3f58d6ee4c47dab074fbf3c4b037abfd96/host/host_darwin.go#L98-L124

and on Windows invoking the infamous tricks to try to get a version, because there is no official API for this https://github.com/shirou/gopsutil/blob/865b8c3f58d6ee4c47dab074fbf3c4b037abfd96/host/host_windows.go#L146-L234

^^ on Windows that's invoked twice because it uses PlatformVersion as KernelVersion

Getting the kernel architecture, which on Linux/Darwin is just unix.Utsname, but on Windows involves parsing processor architectures; https://github.com/shirou/gopsutil/blob/865b8c3f58d6ee4c47dab074fbf3c4b037abfd96/host/host_windows.go#L251-L280

Trying to detect Virtualization, which is only implemented on Linux, but there it's a lot, including (again) parsing /etc/os-release; enough for the library author to implement caching, but that won't do much for a short-lived process like the CLI; https://github.com/shirou/gopsutil/blob/865b8c3f58d6ee4c47dab074fbf3c4b037abfd96/internal/common/common_linux.go#L168-L346

It also tries to get BootTime, and UpTime, which again do a lot, including shelling out to various binaries, and it even has special cases for docker; caching was implemented there as well, but won't help us here; https://github.com/shirou/gopsutil/blob/865b8c3f58d6ee4c47dab074fbf3c4b037abfd96/internal/common/common_linux.go#L63-L119

It tries to obtain the number of running processes on the machine, which on Linux means listing all files in /proc, and for each of them trying to see if it can convert them to an Integer (in which case it's a process-id); https://github.com/shirou/gopsutil/blob/865b8c3f58d6ee4c47dab074fbf3c4b037abfd96/internal/common/common_linux.go#L41-L61

☝️ which makes me want to sit in a corner and cry

It tries to obtain a HostID (sys/kernel/random/boot_id, on Linux, but shelling out to ioreg -rd1 -c IOPlatformExpertDevice on Darwin, and using the registry on Windows)

cli/internal/oauth/manager/util.go

vvoland · 2024-07-04T09:59:31Z

cli/internal/oauth/manager/manager.go

+const (
+	registryAuthKey = "https://index.docker.io/v1/"
+	accessTokenKey  = "access-token"
+	refreshTokenKey = "refresh-token"
+)
+
+func (m *OAuthManager) fetchTokensFromStore() (access, refresh string, err error) {
+	accessAuth, err := m.credStore.Get(registryAuthKey + accessTokenKey)
+	if err != nil {
+		return access, refresh, fmt.Errorf("failed to fetch access token: %w", err)
+	}
+	access = accessAuth.Password
+
+	refreshAuth, err := m.credStore.Get(registryAuthKey + refreshTokenKey)
+	if err != nil {
+		return access, refresh, fmt.Errorf("failed to fetch refresh token: %w", err)
+	}
+	refresh = refreshAuth.Password
+
+	return
+}


We may need some other way to construct the access and refresh token keys (prepend as a subdomain?), or make sure that there's no code that takes the registry hostname into account - like #3599 + #5195.
Otherwise, the access/refresh token could end up being used as an actual auth token.

I haven't investigated it in detail, so it might not be an actual issue in this case, but thought it's worth bringing up 😅

Hmm, that code only does that for non-official registries, so it should be fine (and indeed, that works). We can make sure it continues to work by adding some e2e tests, but maybe we can think of some other way to store these.

I still also want to try to implement some of these at the credstore level, so I'll see how that looks too/if this still matters.

Yeah while it might not be a problem for the CLI code itself, I'm curious if other credential helpers might also do something like this.

Implementing these extra tokens on the credential store level would be nice indeed.

We can check! What I was going to do was still at the CLI level, but as a wrapper to the CLI "credential store" implementations.

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

laurazard · 2024-07-08T13:07:00Z

Superseded by #5244 or #5245

laurazard force-pushed the auth-device-flow branch 2 times, most recently from a26accb to 819df9f Compare July 3, 2024 11:42

laurazard force-pushed the auth-device-flow branch from 819df9f to 44c31c3 Compare July 3, 2024 12:25

dvdksn reviewed Jul 3, 2024

View reviewed changes

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

laurazard force-pushed the auth-device-flow branch 5 times, most recently from dec275e to 8205723 Compare July 3, 2024 14:19

laurazard force-pushed the auth-device-flow branch 2 times, most recently from d490cf3 to 86ccab5 Compare July 3, 2024 16:01

dvdksn closed this Jul 3, 2024

dvdksn reopened this Jul 3, 2024

laurazard self-assigned this Jul 3, 2024

thaJeztah mentioned this pull request Jul 4, 2024

quick try without host.Info() laurazard/cli#2

Draft

thaJeztah reviewed Jul 4, 2024

View reviewed changes

laurazard force-pushed the auth-device-flow branch 2 times, most recently from 49ba2bf to ed4e811 Compare July 4, 2024 09:35

vvoland reviewed Jul 4, 2024

View reviewed changes

WIP

4029dbc

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

laurazard force-pushed the auth-device-flow branch from ed4e811 to 4029dbc Compare July 4, 2024 10:13

laurazard mentioned this pull request Jul 8, 2024

Add support for device-code oauth login #5244

Closed

laurazard closed this Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

laurazard commented Jul 3, 2024

codecov-commenter commented Jul 3, 2024 •

edited

Loading

laurazard commented Jul 3, 2024

dvdksn commented Jul 3, 2024

dvdksn commented Jul 3, 2024

laurazard commented Jul 3, 2024 •

edited

Loading

thaJeztah left a comment •

edited

Loading

thaJeztah Jul 4, 2024

vvoland Jul 4, 2024 •

edited

Loading

laurazard Jul 4, 2024

vvoland Jul 4, 2024

laurazard Jul 4, 2024

laurazard commented Jul 8, 2024

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

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

Conversation

laurazard commented Jul 3, 2024

codecov-commenter commented Jul 3, 2024 • edited Loading

Codecov Report

laurazard commented Jul 3, 2024

dvdksn commented Jul 3, 2024

dvdksn commented Jul 3, 2024

laurazard commented Jul 3, 2024 • edited Loading

thaJeztah left a comment • edited Loading

Choose a reason for hiding this comment

thaJeztah Jul 4, 2024

Choose a reason for hiding this comment

vvoland Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

laurazard Jul 4, 2024

Choose a reason for hiding this comment

vvoland Jul 4, 2024

Choose a reason for hiding this comment

laurazard Jul 4, 2024

Choose a reason for hiding this comment

laurazard commented Jul 8, 2024

codecov-commenter commented Jul 3, 2024 •

edited

Loading

laurazard commented Jul 3, 2024 •

edited

Loading

thaJeztah left a comment •

edited

Loading

vvoland Jul 4, 2024 •

edited

Loading