auth: keep keyring backend on probe timeout during login#5210
Draft
simonfaltum wants to merge 3 commits intomainfrom
Draft
auth: keep keyring backend on probe timeout during login#5210simonfaltum wants to merge 3 commits intomainfrom
simonfaltum wants to merge 3 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.
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'.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)