Skip to content

Commit 6cc96a5

Browse files
committed
auth: keep keyring backend on probe timeout during login
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.
1 parent f0edaa4 commit 6cc96a5

5 files changed

Lines changed: 81 additions & 12 deletions

File tree

NEXT_CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
### CLI
66

7+
* `auth login` no longer falls back to plaintext when the OS keyring is reachable but locked. The unlock prompt shown by the probe now runs in parallel with the OAuth flow, and the token is stored in the keyring once the user has typed their password.
8+
79
### Bundles
810

911
* Fixed `--force-pull` on `bundle summary` and `bundle open` so the flag bypasses the local state cache and reads state from the workspace.

cmd/auth/login.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -146,10 +146,12 @@ a new profile is created.
146146
ctx := cmd.Context()
147147
profileName := cmd.Flag("profile").Value.String()
148148

149-
// Resolve the cache before the browser step so a missing/locked keyring
150-
// surfaces here rather than after the user completes OAuth. When secure
151-
// is selected but the keyring is unreachable, this silently falls back
152-
// to plaintext and persists auth_storage = plaintext for next time.
149+
// Resolve the cache before the browser step. The probe also acts as
150+
// a side-channel that triggers the OS keyring unlock prompt early,
151+
// so the user can answer it while OAuth is running. A genuinely
152+
// unavailable keyring still surfaces here rather than after OAuth;
153+
// a locked keyring (probe times out) is non-fatal and login stays
154+
// on the keyring backend.
153155
tokenCache, mode, err := storage.ResolveCacheForLogin(ctx, "")
154156
if err != nil {
155157
return err

libs/auth/storage/cache.go

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package storage
22

33
import (
44
"context"
5+
"errors"
56
"fmt"
67

78
"github.com/databricks/cli/libs/databrickscfg"
@@ -54,8 +55,13 @@ func ResolveCache(ctx context.Context, override StorageMode) (cache.TokenCache,
5455
// 2. When the user explicitly asked for secure (override, env var, or
5556
// config) but the keyring is unreachable, return an error. An explicit
5657
// "I want secure" is honored strictly: never silently downgrade.
58+
// 3. When the probe times out, the keyring is reachable but locked — the
59+
// OS unlock prompt is on screen and the user is mid-typing. Stay on
60+
// keyring regardless of explicit. The unlock continues in parallel
61+
// with the OAuth flow, and by the time the final Store runs the
62+
// keyring will be unlocked.
5763
//
58-
// Both rules are dormant today: the resolver default is plaintext, so
64+
// Rules 1 and 2 are dormant today: the resolver default is plaintext, so
5965
// (mode=Secure, explicit=false) is unreachable. They activate when the
6066
// default flips to secure (MS4 / cli-ga). Wiring lands now so MS4 is a
6167
// single-line default flip plus pin-on-success additions.
@@ -133,6 +139,18 @@ func applyLoginFallback(ctx context.Context, mode StorageMode, explicit bool, f
133139
return c, mode, nil
134140
case StorageModeSecure:
135141
if probeErr := f.probeKeyring(); probeErr != nil {
142+
// A timeout means the keyring is reachable but locked: the OS
143+
// unlock prompt is on screen and the user is mid-typing. Stay on
144+
// keyring regardless of explicit; by the time OAuth finishes the
145+
// prompt has been answered and the final Store will succeed
146+
// against an unlocked keyring. Mirrors gh CLI's accidentally-
147+
// similar flow where AuthTokenWriteable's early keyring.Get
148+
// triggers the same dialog without committing to plaintext.
149+
var timeoutErr *TimeoutError
150+
if errors.As(probeErr, &timeoutErr) {
151+
log.Debugf(ctx, "keyring probe timed out (%v); user is likely unlocking, staying on keyring", probeErr)
152+
return f.newKeyring(), mode, nil
153+
}
136154
if explicit {
137155
return nil, "", fmt.Errorf("secure storage was requested but the OS keyring is not reachable: %w", probeErr)
138156
}

libs/auth/storage/cache_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package storage
33
import (
44
"context"
55
"errors"
6+
"fmt"
67
"os"
78
"path/filepath"
89
"testing"
@@ -232,6 +233,45 @@ func TestApplyLoginFallback_ExplicitSecure_ProbeFail_Errors(t *testing.T) {
232233
assert.Equal(t, "", persisted, "explicit-secure error must not write config")
233234
}
234235

236+
// A locked keyring with a slow user surfaces as TimeoutError. We want login
237+
// to stay on the keyring so the final Store lands there once the user has
238+
// finished unlocking, regardless of whether secure was explicit. Cover both
239+
// the bare TimeoutError (in case probe wraps thinner in the future) and the
240+
// real wrapped form returned by probeWithBackend.
241+
func TestApplyLoginFallback_ProbeTimeout_StaysOnKeyring(t *testing.T) {
242+
cases := []struct {
243+
name string
244+
explicit bool
245+
probeErr error
246+
}{
247+
{"default-secure, bare TimeoutError", false, &TimeoutError{Op: "set"}},
248+
{"default-secure, wrapped TimeoutError", false, fmt.Errorf("write: %w", &TimeoutError{Op: "set"})},
249+
{"explicit-secure, bare TimeoutError", true, &TimeoutError{Op: "set"}},
250+
{"explicit-secure, wrapped TimeoutError", true, fmt.Errorf("write: %w", &TimeoutError{Op: "set"})},
251+
}
252+
253+
for _, tc := range cases {
254+
t.Run(tc.name, func(t *testing.T) {
255+
hermetic(t)
256+
ctx := t.Context()
257+
configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE")
258+
259+
f := fakeFactories(t)
260+
f.probeKeyring = func() error { return tc.probeErr }
261+
262+
got, mode, err := applyLoginFallback(ctx, StorageModeSecure, tc.explicit, f)
263+
264+
require.NoError(t, err)
265+
assert.Equal(t, StorageModeSecure, mode)
266+
assert.Equal(t, "keyring", got.(stubCache).source)
267+
268+
persisted, gerr := databrickscfg.GetConfiguredAuthStorage(ctx, configPath)
269+
require.NoError(t, gerr)
270+
assert.Equal(t, "", persisted, "probe timeout must not persist plaintext fallback")
271+
})
272+
}
273+
}
274+
235275
func TestWrapForOAuthArgument(t *testing.T) {
236276
const (
237277
host = "https://example.com"

libs/auth/storage/keyring.go

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -88,14 +88,21 @@ func NewKeyringCache() cache.TokenCache {
8888
}
8989
}
9090

91-
// ProbeKeyring returns nil if the OS keyring is reachable and accepts a
92-
// write+delete cycle within the standard timeout. A non-nil error means the
93-
// keyring cannot be used in this environment (no backend, headless Linux
94-
// session waiting on a UI prompt, locked keychain refusing access, etc.).
91+
// ProbeKeyring returns nil if a write+delete cycle completed within the
92+
// standard timeout. Callers distinguish two non-nil shapes via errors.As:
9593
//
96-
// Used by databricks auth login to decide whether to silently fall back to
97-
// plaintext storage before opening the browser, so the user does not
98-
// complete an OAuth flow only to fail at the final Store call.
94+
// - *TimeoutError: the keyring is reachable but locked. On Linux this
95+
// usually means the GUI unlock prompt is up and the user has not
96+
// finished typing yet. The login path treats this as "stay on keyring"
97+
// because the prompt continues in parallel with OAuth and the final
98+
// Store will succeed against the by-then-unlocked keyring.
99+
// - any other error: the keyring is genuinely unavailable (no daemon,
100+
// headless session with no secret service, ...). Login falls back to
101+
// plaintext now rather than failing after OAuth.
102+
//
103+
// Probing also has a useful side effect: triggering the unlock prompt up
104+
// front, before the browser step. The user can answer it while OAuth is in
105+
// flight instead of after.
99106
func ProbeKeyring() error {
100107
return probeWithBackend(zalandoBackend{}, defaultKeyringTimeout)
101108
}

0 commit comments

Comments
 (0)