auth: keep keyring backend on probe timeout during login#5210
auth: keep keyring backend on probe timeout during login#5210simonfaltum wants to merge 6 commits intomainfrom
Conversation
A locked-but-reachable OS keyring surfaces as a probe TimeoutError. Today that's treated the same as a genuinely unavailable keyring: login commits to plaintext, even though the user is mid-typing in the unlock dialog and the keyring is about to be unlocked. Distinguish *TimeoutError from other probe errors. On timeout, stay on keyring regardless of whether secure was explicit; the unlock continues in parallel with the OAuth flow and the final Store completes against an unlocked keyring. Other probe errors keep current behavior.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
The wrapper produces TimeoutError for any operation that misses the deadline, not only locked-but-reachable keyrings. The login path's optimistic stay-on-keyring is still right (cost of guessing wrong is one wasted OAuth ceremony, not a silently-plaintext token), but the docstring and inline comment overclaimed by labeling timeout as 'reachable but locked'.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
The doc and inline comments narrated the trade-off in too much detail. Comments should explain WHY when non-obvious, not walk through every branch. The decision and its trade-off are clear in the code; the comment just needs to flag that the timeout case is intentional and not a silent downgrade.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Approval status: pending
|
| // the final Store rather than silently downgrading to plaintext. | ||
| // | ||
| // Both rules are dormant today: the resolver default is plaintext, so | ||
| // Rules 1 and 2 are dormant today: the resolver default is plaintext, so |
There was a problem hiding this comment.
Meta point, why is this called cache.go and ResolveCache if it is a storage backend?
Or is this a cache layer on top of it?
There was a problem hiding this comment.
Yeah, this is a remnant from earlier iterations. It started as cmd/auth/cache.go with a newAuthCache() helper that literally just returned a cache.TokenCache (the SDK type at databricks-sdk-go/credentials/u2m/cache). Back then the name matched what it did.
To answer the second part directly: no, it's not a cache layer on top of a storage backend. The SDK's cache.TokenCache interface is the storage backend abstraction, that naming is just bleeding through. Since then this file has grown probe logic, login fallback, and the dual-write wrapper, so it's really a storage backend resolver now.
If we want to do a rename for clarity, I would suggest a follow-up PR (cache.go -> resolve.go, ResolveCache -> ResolveStorage). ~5 call sites, small.
| // Rules 1 and 2 are dormant today: the resolver default is plaintext, so | ||
| // (mode=Secure, explicit=false) is unreachable. They activate when the | ||
| // default flips to secure (MS4 / cli-ga). Wiring lands now so MS4 is a | ||
| // single-line default flip plus pin-on-success additions. |
There was a problem hiding this comment.
I don't understand the above.
There was a problem hiding this comment.
Tried to clarify it a bit. Reads clearer now?
| if errors.As(probeErr, &timeoutErr) { | ||
| log.Debugf(ctx, "keyring probe timed out (%v); staying on keyring", probeErr) | ||
| return f.newKeyring(), mode, nil | ||
| } |
There was a problem hiding this comment.
Under which conditions does a timeout happen?
There was a problem hiding this comment.
One example is if the keyring is locked, we can't know if the user is typing the password or not. If we wait for the keyring to become available, we could be blocking forever with potentially no way for the user to take action. So then the timeout would trigger
There was a problem hiding this comment.
To add some precision: the common case is a locked OS keyring on Linux. The Secret Service blocks the Set call while waiting for the user to type their password into a GUI unlock dialog. The keyring API gives us no progress signal and no cancel, so we wrap the call in a 3s timeout (matches GitHub CLI's pattern, see defaultKeyringTimeout in keyring.go). If the user takes longer than 3s to enter the password (common), the wrapper returns a *TimeoutError. The dialog itself stays up, which is what this PR exploits: keep the keyring backend, let OAuth proceed in parallel, and the user finishes unlocking before the SDK's final Store call.
Drops internal milestone jargon (MS4 / cli-ga) from doc comments and aligns wording with the package doc in mode.go (which already says "at GA"). No behavior change. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
2 participants
Why
When
databricks auth loginruns with secure storage and the OS keyring is reachable but locked, the probe triggers the GUI unlock prompt and immediately races a 3 second timeout. If the user takes longer than 3 seconds to type their password, the probe returns a TimeoutError and the entire login commits to plaintext, even though the user goes on to successfully unlock the keyring while OAuth is running in the browser.End result: user types keyring password, finishes OAuth, token still lands in plaintext. Confusing and easy to miss.
Changes
Before: any probe error (timeout or otherwise) caused secure-default logins to silently fall back to plaintext, and explicit-secure logins to error.
Now: probe errors split into two cases.
*TimeoutError: keyring is reachable but locked. Login stays on keyring regardless of whether secure was explicit. The unlock prompt continues in parallel with OAuth, and by the time the SDK calls Store at the end the keyring has been unlocked.The probe still triggers the unlock prompt up front, so the prompt is visible while the user is also doing OAuth. That overlap is the whole point: it gives the user time to unlock without the CLI giving up.
Trade-off
If the user does not unlock the keyring within the probe's 3 second window, login proceeds with the keyring backend on the assumption that the user will type their password during the OAuth ceremony. If instead they cancel the dialog, ignore it, or otherwise never enter the password, the SDK's final Store at the end of login triggers a second unlock attempt and times out after 3 seconds, failing the login.
In other words: cooperative users who actually want to use the keyring see one prompt and succeed. Users who chose not to use the keyring after seeing it see a second prompt and a 3 second wait at the end. We accept this compromise on the assumption that users who hit the dialog at all are lawful-good users working with the CLI, not against it.
Test plan
cache_test.gocover the TimeoutError path: bare and wrapped, default and explicit secure (4 sub cases)task checkscleantask lint-qcleandatabricks auth loginwith secure storage, deliberately wait >10s before entering the keyring password, confirm the token lands in keyring (not in the plaintext file cache)