Skip to content

auth: keep keyring backend on probe timeout during login#5210

Draft
simonfaltum wants to merge 3 commits intomainfrom
simonfaltum/keyring-probe-non-fatal
Draft

auth: keep keyring backend on probe timeout during login#5210
simonfaltum wants to merge 3 commits intomainfrom
simonfaltum/keyring-probe-non-fatal

Conversation

@simonfaltum
Copy link
Copy Markdown
Member

@simonfaltum simonfaltum commented May 7, 2026

Why

When databricks auth login runs 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.
  • Any other error: keyring is genuinely unavailable. Existing behavior unchanged (silent plaintext fallback when not explicit, error when explicit).

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

  • New unit tests in cache_test.go cover the TimeoutError path: bare and wrapped, default and explicit secure (4 sub cases)
  • Existing probe-fail tests still pass (plain errors still fall back / error as before)
  • task checks clean
  • task lint-q clean
  • Manual on Ubuntu: lock the default secret service collection, run databricks auth login with secure storage, deliberately wait >10s before entering the keyring password, confirm the token lands in keyring (not in the plaintext file cache)
  • Manual on Ubuntu: same setup but cancel the dialog after the probe times out, confirm login fails after a second 3 second wait at the end of OAuth (the documented trade-off)

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 simonfaltum temporarily deployed to test-trigger-is May 7, 2026 16:40 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 7, 2026 16:40 — with 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 simonfaltum temporarily deployed to test-trigger-is May 7, 2026 16:56 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 7, 2026 16:56 — with 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 simonfaltum temporarily deployed to test-trigger-is May 7, 2026 17:00 — with GitHub Actions Inactive
@simonfaltum simonfaltum temporarily deployed to test-trigger-is May 7, 2026 17:00 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant