From 6ad7eaf11c0c908c7f13752a4e3ff770d2026a7b Mon Sep 17 00:00:00 2001 From: simon Date: Tue, 5 May 2026 11:11:43 +0200 Subject: [PATCH 1/4] auth: silently fall back to plaintext when keyring is unreachable on login When secure storage is selected but the OS keyring is unreachable (no D-Bus on Linux, headless SSH session, locked keychain that hangs), databricks auth login now silently falls back to plaintext storage and writes auth_storage = plaintext to .databrickscfg so the choice is sticky for later commands. Mirrors the behavior of GitHub CLI. The probe runs before the browser step so users do not complete OAuth just to fail at the final Store call. Login is the only command that falls back: read paths (auth token, bundle commands) keep the original keyring error so they do not silently mint plaintext copies of tokens that live in the keyring on another machine. Co-authored-by: Isaac --- cmd/auth/login.go | 6 ++- libs/auth/storage/cache.go | 65 +++++++++++++++++++++++-- libs/auth/storage/cache_test.go | 79 +++++++++++++++++++++++++++++-- libs/auth/storage/keyring.go | 33 +++++++++++++ libs/auth/storage/keyring_test.go | 40 ++++++++++++++++ libs/databrickscfg/ops.go | 23 +++++++++ libs/databrickscfg/ops_test.go | 54 +++++++++++++++++++++ 7 files changed, 291 insertions(+), 9 deletions(-) diff --git a/cmd/auth/login.go b/cmd/auth/login.go index b7700e1cde2..643e547c146 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -147,7 +147,11 @@ a new profile is created. ctx := cmd.Context() profileName := cmd.Flag("profile").Value.String() - tokenCache, mode, err := storage.ResolveCache(ctx, "") + // Resolve the cache before the browser step so a missing/locked keyring + // surfaces here rather than after the user completes OAuth. When secure + // is selected but the keyring is unreachable, this silently falls back + // to plaintext and persists auth_storage = plaintext for next time. + tokenCache, mode, err := storage.ResolveCacheForLogin(ctx, "") if err != nil { return err } diff --git a/libs/auth/storage/cache.go b/libs/auth/storage/cache.go index 151646081d3..60f77a706e1 100644 --- a/libs/auth/storage/cache.go +++ b/libs/auth/storage/cache.go @@ -4,6 +4,9 @@ import ( "context" "fmt" + "github.com/databricks/cli/libs/databrickscfg" + "github.com/databricks/cli/libs/env" + "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go/credentials/u2m" "github.com/databricks/databricks-sdk-go/credentials/u2m/cache" ) @@ -12,15 +15,17 @@ import ( // so unit tests can inject stubs without hitting the real OS keyring or // filesystem. Production code uses defaultCacheFactories(). type cacheFactories struct { - newFile func(context.Context) (cache.TokenCache, error) - newKeyring func() cache.TokenCache + newFile func(context.Context) (cache.TokenCache, error) + newKeyring func() cache.TokenCache + probeKeyring func() error } // defaultCacheFactories returns the production factory set. func defaultCacheFactories() cacheFactories { return cacheFactories{ - newFile: func(ctx context.Context) (cache.TokenCache, error) { return NewFileTokenCache(ctx) }, - newKeyring: NewKeyringCache, + newFile: func(ctx context.Context) (cache.TokenCache, error) { return NewFileTokenCache(ctx) }, + newKeyring: NewKeyringCache, + probeKeyring: ProbeKeyring, } } @@ -38,6 +43,18 @@ func ResolveCache(ctx context.Context, override StorageMode) (cache.TokenCache, return resolveCacheWith(ctx, override, defaultCacheFactories()) } +// ResolveCacheForLogin resolves the cache like ResolveCache, but if the +// resolved mode is secure and the OS keyring is unreachable it silently +// falls back to plaintext and persists auth_storage = plaintext to +// .databrickscfg (only if no value is set there yet). +// +// Login-specific. Read paths (auth token, bundle commands) keep the original +// keyring error so they don't silently mint plaintext copies of tokens that +// were stored in the keyring on another machine. +func ResolveCacheForLogin(ctx context.Context, override StorageMode) (cache.TokenCache, StorageMode, error) { + return resolveCacheForLoginWith(ctx, override, defaultCacheFactories()) +} + // WrapForOAuthArgument wraps tokenCache so SDK-side writes (Challenge, refresh) // dual-write to the legacy host-based cache key when mode is plaintext. Other // modes return tokenCache unchanged: secure mode never writes a host-key entry, @@ -73,3 +90,43 @@ func resolveCacheWith(ctx context.Context, override StorageMode, f cacheFactorie return nil, "", fmt.Errorf("unsupported storage mode %q", string(mode)) } } + +// resolveCacheForLoginWith is the pure form of ResolveCacheForLogin. It takes +// the factory set as a parameter so tests can inject stubs. +func resolveCacheForLoginWith(ctx context.Context, override StorageMode, f cacheFactories) (cache.TokenCache, StorageMode, error) { + tokenCache, mode, err := resolveCacheWith(ctx, override, f) + if err != nil { + return nil, "", err + } + if mode != StorageModeSecure { + return tokenCache, mode, nil + } + if probeErr := f.probeKeyring(); probeErr != nil { + log.Debugf(ctx, "secure storage unavailable (%v), falling back to plaintext", probeErr) + fileCache, fileErr := f.newFile(ctx) + if fileErr != nil { + return nil, "", fmt.Errorf("open file token cache: %w", fileErr) + } + if err := persistPlaintextFallback(ctx); err != nil { + log.Debugf(ctx, "persisting auth_storage fallback failed: %v", err) + } + return fileCache, StorageModePlaintext, nil + } + return tokenCache, mode, nil +} + +// persistPlaintextFallback writes auth_storage = plaintext to [__settings__] +// in .databrickscfg, but only if the key is not already set. This makes the +// silent fallback sticky across commands: future invocations resolve directly +// to plaintext and skip the keyring probe. +func persistPlaintextFallback(ctx context.Context) error { + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") + existing, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + if err != nil { + return err + } + if existing != "" { + return nil + } + return databrickscfg.SetConfiguredAuthStorage(ctx, string(StorageModePlaintext), configPath) +} diff --git a/libs/auth/storage/cache_test.go b/libs/auth/storage/cache_test.go index b84c1ef3ba0..54a54d3cbe9 100644 --- a/libs/auth/storage/cache_test.go +++ b/libs/auth/storage/cache_test.go @@ -3,9 +3,11 @@ package storage import ( "context" "errors" + "os" "path/filepath" "testing" + "github.com/databricks/cli/libs/databrickscfg" "github.com/databricks/cli/libs/env" "github.com/databricks/databricks-sdk-go/credentials/u2m" "github.com/databricks/databricks-sdk-go/credentials/u2m/cache" @@ -24,8 +26,9 @@ func (stubCache) Lookup(string) (*oauth2.Token, error) { return nil, cache.ErrNo func fakeFactories(t *testing.T) cacheFactories { t.Helper() return cacheFactories{ - newFile: func(context.Context) (cache.TokenCache, error) { return stubCache{source: "file"}, nil }, - newKeyring: func() cache.TokenCache { return stubCache{source: "keyring"} }, + newFile: func(context.Context) (cache.TokenCache, error) { return stubCache{source: "file"}, nil }, + newKeyring: func() cache.TokenCache { return stubCache{source: "keyring"} }, + probeKeyring: func() error { return nil }, } } @@ -106,8 +109,9 @@ func TestResolveCache_FileFactoryErrorPropagates(t *testing.T) { ctx := t.Context() boom := errors.New("disk full") factories := cacheFactories{ - newFile: func(context.Context) (cache.TokenCache, error) { return nil, boom }, - newKeyring: func() cache.TokenCache { return stubCache{source: "keyring"} }, + newFile: func(context.Context) (cache.TokenCache, error) { return nil, boom }, + newKeyring: func() cache.TokenCache { return stubCache{source: "keyring"} }, + probeKeyring: func() error { return nil }, } _, _, err := resolveCacheWith(ctx, StorageModePlaintext, factories) @@ -116,6 +120,73 @@ func TestResolveCache_FileFactoryErrorPropagates(t *testing.T) { assert.ErrorIs(t, err, boom) } +func TestResolveCacheForLogin_PlaintextSkipsProbe(t *testing.T) { + hermetic(t) + ctx := t.Context() + probed := false + f := fakeFactories(t) + f.probeKeyring = func() error { + probed = true + return nil + } + + got, mode, err := resolveCacheForLoginWith(ctx, StorageModePlaintext, f) + + require.NoError(t, err) + assert.Equal(t, StorageModePlaintext, mode) + assert.Equal(t, "file", got.(stubCache).source) + assert.False(t, probed, "probe must not run when mode is already plaintext") +} + +func TestResolveCacheForLogin_SecureProbeOK(t *testing.T) { + hermetic(t) + ctx := env.Set(t.Context(), EnvVar, "secure") + + got, mode, err := resolveCacheForLoginWith(ctx, "", fakeFactories(t)) + + require.NoError(t, err) + assert.Equal(t, StorageModeSecure, mode) + assert.Equal(t, "keyring", got.(stubCache).source) +} + +func TestResolveCacheForLogin_FallsBackWhenProbeFails(t *testing.T) { + hermetic(t) + ctx := env.Set(t.Context(), EnvVar, "secure") + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") + + f := fakeFactories(t) + f.probeKeyring = func() error { return errors.New("no keyring") } + + got, mode, err := resolveCacheForLoginWith(ctx, "", f) + + require.NoError(t, err) + assert.Equal(t, StorageModePlaintext, mode) + assert.Equal(t, "file", got.(stubCache).source) + + persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, err) + assert.Equal(t, "plaintext", persisted, "auth_storage must be persisted on first fallback") +} + +func TestResolveCacheForLogin_DoesNotOverwriteExistingAuthStorage(t *testing.T) { + hermetic(t) + ctx := env.Set(t.Context(), EnvVar, "secure") + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") + require.NoError(t, os.WriteFile(configPath, []byte("[__settings__]\nauth_storage = secure\n"), 0o600)) + + f := fakeFactories(t) + f.probeKeyring = func() error { return errors.New("no keyring") } + + _, mode, err := resolveCacheForLoginWith(ctx, "", f) + + require.NoError(t, err) + assert.Equal(t, StorageModePlaintext, mode) + + persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, err) + assert.Equal(t, "secure", persisted, "existing auth_storage must not be overwritten by fallback") +} + func TestWrapForOAuthArgument(t *testing.T) { const ( host = "https://example.com" diff --git a/libs/auth/storage/keyring.go b/libs/auth/storage/keyring.go index e9fc7d13dfa..d16428a57b0 100644 --- a/libs/auth/storage/keyring.go +++ b/libs/auth/storage/keyring.go @@ -17,6 +17,11 @@ import ( // cache key the SDK passes through TokenCache.Store / Lookup. const keyringServiceName = "databricks-cli" +// keyringProbeAccount is the account name ProbeKeyring writes and deletes +// to verify the keyring is reachable. Distinct from any real cache key so a +// concurrent probe cannot collide with an actual OAuth token entry. +const keyringProbeAccount = "__probe__" + // defaultKeyringTimeout is how long a single keyring operation is allowed // to run before the wrapper returns a TimeoutError. Matches the value used // by GitHub CLI. @@ -79,6 +84,34 @@ func NewKeyringCache() cache.TokenCache { } } +// ProbeKeyring returns nil if the OS keyring is reachable and accepts a +// write+delete cycle within the standard timeout. A non-nil error means the +// keyring cannot be used in this environment (no backend, headless Linux +// session waiting on a UI prompt, locked keychain refusing access, etc.). +// +// Used by databricks auth login to decide whether to silently fall back to +// plaintext storage before opening the browser, so the user does not +// complete an OAuth flow only to fail at the final Store call. +func ProbeKeyring() error { + return probeWithBackend(zalandoBackend{}, defaultKeyringTimeout) +} + +func probeWithBackend(backend keyringBackend, timeout time.Duration) error { + c := &keyringCache{ + backend: backend, + timeout: timeout, + keyringSvcName: keyringServiceName, + } + tok := &oauth2.Token{AccessToken: "probe"} + if err := c.Store(keyringProbeAccount, tok); err != nil { + return fmt.Errorf("write: %w", err) + } + if err := c.Store(keyringProbeAccount, nil); err != nil { + return fmt.Errorf("delete: %w", err) + } + return nil +} + // Store stores t under key. Nil t deletes the entry; deleting a missing // entry is not an error. func (k *keyringCache) Store(key string, t *oauth2.Token) error { diff --git a/libs/auth/storage/keyring_test.go b/libs/auth/storage/keyring_test.go index 74ea3c0c63f..fbfa4713daa 100644 --- a/libs/auth/storage/keyring_test.go +++ b/libs/auth/storage/keyring_test.go @@ -217,3 +217,43 @@ func TestKeyringCache_StoreNil_TimesOut(t *testing.T) { var timeoutErr *TimeoutError assert.ErrorAs(t, err, &timeoutErr, "expected TimeoutError, got %T: %v", err, err) } + +func TestProbeKeyring_SuccessLeavesNoEntry(t *testing.T) { + backend := newFakeBackend() + + err := probeWithBackend(backend, 100*time.Millisecond) + require.NoError(t, err) + + _, ok := backend.items[itemKey(keyringServiceName, keyringProbeAccount)] + assert.False(t, ok, "probe must clean up after itself") +} + +func TestProbeKeyring_SetErrorPropagates(t *testing.T) { + boom := errors.New("no backend") + backend := newFakeBackend() + backend.setErr = boom + + err := probeWithBackend(backend, 100*time.Millisecond) + require.Error(t, err) + assert.ErrorIs(t, err, boom) +} + +func TestProbeKeyring_SetTimesOut(t *testing.T) { + backend := newFakeBackend() + backend.setBlock = true + + err := probeWithBackend(backend, 50*time.Millisecond) + require.Error(t, err) + var timeoutErr *TimeoutError + assert.ErrorAs(t, err, &timeoutErr) +} + +func TestProbeKeyring_DeleteErrorPropagates(t *testing.T) { + boom := errors.New("delete failed") + backend := newFakeBackend() + backend.deleteErr = boom + + err := probeWithBackend(backend, 100*time.Millisecond) + require.Error(t, err) + assert.ErrorIs(t, err, boom) +} diff --git a/libs/databrickscfg/ops.go b/libs/databrickscfg/ops.go index c4d0f1cc797..43b3fc70005 100644 --- a/libs/databrickscfg/ops.go +++ b/libs/databrickscfg/ops.go @@ -196,6 +196,29 @@ func SetDefaultProfile(ctx context.Context, profileName, configFilePath string) return writeConfigFile(ctx, configFile) } +// SetConfiguredAuthStorage writes the auth_storage key to the [__settings__] +// section. Used by auth login to persist a plaintext fallback when the OS +// keyring is unreachable, so subsequent commands skip the keyring probe and +// route directly to the file cache. +func SetConfiguredAuthStorage(ctx context.Context, value, configFilePath string) error { + configFile, err := loadOrCreateConfigFile(ctx, configFilePath) + if err != nil { + return err + } + + section, err := configFile.GetSection(databricksSettingsSection) + if err != nil { + section, err = configFile.NewSection(databricksSettingsSection) + if err != nil { + return fmt.Errorf("cannot create %s section: %w", databricksSettingsSection, err) + } + } + + section.Key(authStorageKey).SetValue(value) + + return writeConfigFile(ctx, configFile) +} + // ClearDefaultProfile removes the default_profile key from the [__settings__] // section if the current default matches the given profile name. func ClearDefaultProfile(ctx context.Context, profileName, configFilePath string) error { diff --git a/libs/databrickscfg/ops_test.go b/libs/databrickscfg/ops_test.go index 0555a8171f6..a8ef811e751 100644 --- a/libs/databrickscfg/ops_test.go +++ b/libs/databrickscfg/ops_test.go @@ -709,3 +709,57 @@ func TestGetConfiguredAuthStorage_MissingFile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "", got) } + +func TestSetConfiguredAuthStorage(t *testing.T) { + cases := []struct { + name string + contents string + }{ + { + name: "missing file is created", + contents: "", + }, + { + name: "missing settings section is created", + contents: "[my-ws]\nhost = https://example.cloud.databricks.com\n", + }, + { + name: "settings section without auth_storage gets the key added", + contents: "[__settings__]\ndefault_profile = my-ws\n", + }, + { + name: "existing auth_storage value is overwritten", + contents: "[__settings__]\nauth_storage = secure\n", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), ".databrickscfg") + if tc.contents != "" { + require.NoError(t, os.WriteFile(path, []byte(tc.contents), 0o600)) + } + + require.NoError(t, SetConfiguredAuthStorage(t.Context(), "plaintext", path)) + + got, err := GetConfiguredAuthStorage(t.Context(), path) + require.NoError(t, err) + assert.Equal(t, "plaintext", got) + }) + } +} + +func TestSetConfiguredAuthStorage_PreservesOtherSettings(t *testing.T) { + path := filepath.Join(t.TempDir(), ".databrickscfg") + require.NoError(t, os.WriteFile(path, []byte("[__settings__]\ndefault_profile = dev\n\n[dev]\nhost = https://example.cloud.databricks.com\n"), 0o600)) + + require.NoError(t, SetConfiguredAuthStorage(t.Context(), "plaintext", path)) + + defaultProfile, err := GetConfiguredDefaultProfile(t.Context(), path) + require.NoError(t, err) + assert.Equal(t, "dev", defaultProfile) + + authStorage, err := GetConfiguredAuthStorage(t.Context(), path) + require.NoError(t, err) + assert.Equal(t, "plaintext", authStorage) +} From d56ec6f4e3a90884fa482917b9abe4936da2ae18 Mon Sep 17 00:00:00 2001 From: simon Date: Tue, 5 May 2026 13:27:55 +0200 Subject: [PATCH 2/4] auth: error on explicit secure when keyring unreachable Differentiate "I asked for secure" from "the default happens to be secure": when the user sets DATABRICKS_AUTH_STORAGE=secure or auth_storage = secure in .databrickscfg, honor it strictly. If the keyring fails the probe, return a clear error rather than silently downgrading to plaintext. The silent fallback now applies only when secure was the implicit default (no override flag, no env var, no config). In that case login still falls back and persists auth_storage = plaintext so subsequent commands route to the file cache without re-probing. Adds ResolveStorageModeWithSource so callers can detect explicit user choice. Splits the login-time policy into applyLoginFallback so it can be unit-tested across the (mode, explicit) input space directly. Co-authored-by: Isaac --- libs/auth/storage/cache.go | 53 ++++++++++++++++-------- libs/auth/storage/cache_test.go | 71 +++++++++++++++++++++++++++------ libs/auth/storage/mode.go | 23 ++++++++--- libs/auth/storage/mode_test.go | 51 +++++++++++++++++++++++ 4 files changed, 163 insertions(+), 35 deletions(-) diff --git a/libs/auth/storage/cache.go b/libs/auth/storage/cache.go index 60f77a706e1..c2c99962441 100644 --- a/libs/auth/storage/cache.go +++ b/libs/auth/storage/cache.go @@ -43,10 +43,16 @@ func ResolveCache(ctx context.Context, override StorageMode) (cache.TokenCache, return resolveCacheWith(ctx, override, defaultCacheFactories()) } -// ResolveCacheForLogin resolves the cache like ResolveCache, but if the -// resolved mode is secure and the OS keyring is unreachable it silently -// falls back to plaintext and persists auth_storage = plaintext to -// .databrickscfg (only if no value is set there yet). +// ResolveCacheForLogin resolves the cache like ResolveCache with extra rules +// for the auth login path: +// +// 1. When the resolved mode is secure and the user did not explicitly ask for +// it (no override flag, no env var, no config), and the OS keyring is +// unreachable, fall back silently to plaintext and persist +// auth_storage = plaintext so subsequent commands skip the probe. +// 2. When the user explicitly asked for secure (override, env var, or config) +// but the keyring is unreachable, return an error. An explicit "I want +// secure" is honored strictly: never silently downgrade. // // Login-specific. Read paths (auth token, bundle commands) keep the original // keyring error so they don't silently mint plaintext copies of tokens that @@ -94,14 +100,34 @@ func resolveCacheWith(ctx context.Context, override StorageMode, f cacheFactorie // resolveCacheForLoginWith is the pure form of ResolveCacheForLogin. It takes // the factory set as a parameter so tests can inject stubs. func resolveCacheForLoginWith(ctx context.Context, override StorageMode, f cacheFactories) (cache.TokenCache, StorageMode, error) { - tokenCache, mode, err := resolveCacheWith(ctx, override, f) + mode, explicit, err := ResolveStorageModeWithSource(ctx, override) if err != nil { return nil, "", err } + return applyLoginFallback(ctx, mode, explicit, f) +} + +// applyLoginFallback realizes the login-time fallback rules given an already- +// resolved mode and whether the user explicitly asked for it. Split out so +// tests can drive the (mode, explicit) input space directly without depending +// on whatever the resolver's default mode happens to be at any point in time. +func applyLoginFallback(ctx context.Context, mode StorageMode, explicit bool, f cacheFactories) (cache.TokenCache, StorageMode, error) { if mode != StorageModeSecure { - return tokenCache, mode, nil + switch mode { + case StorageModePlaintext: + c, err := f.newFile(ctx) + if err != nil { + return nil, "", fmt.Errorf("open file token cache: %w", err) + } + return c, mode, nil + default: + return nil, "", fmt.Errorf("unsupported storage mode %q", string(mode)) + } } if probeErr := f.probeKeyring(); probeErr != nil { + if explicit { + return nil, "", fmt.Errorf("secure storage was requested but the OS keyring is not reachable: %w", probeErr) + } log.Debugf(ctx, "secure storage unavailable (%v), falling back to plaintext", probeErr) fileCache, fileErr := f.newFile(ctx) if fileErr != nil { @@ -112,21 +138,14 @@ func resolveCacheForLoginWith(ctx context.Context, override StorageMode, f cache } return fileCache, StorageModePlaintext, nil } - return tokenCache, mode, nil + return f.newKeyring(), StorageModeSecure, nil } // persistPlaintextFallback writes auth_storage = plaintext to [__settings__] -// in .databrickscfg, but only if the key is not already set. This makes the -// silent fallback sticky across commands: future invocations resolve directly -// to plaintext and skip the keyring probe. +// in .databrickscfg. Only called when the user did not explicitly choose +// secure, so overwriting any existing value would only happen if a previous +// fallback already wrote the same value: a no-op write. func persistPlaintextFallback(ctx context.Context) error { configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") - existing, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) - if err != nil { - return err - } - if existing != "" { - return nil - } return databrickscfg.SetConfiguredAuthStorage(ctx, string(StorageModePlaintext), configPath) } diff --git a/libs/auth/storage/cache_test.go b/libs/auth/storage/cache_test.go index 54a54d3cbe9..34d3f38c131 100644 --- a/libs/auth/storage/cache_test.go +++ b/libs/auth/storage/cache_test.go @@ -149,7 +149,7 @@ func TestResolveCacheForLogin_SecureProbeOK(t *testing.T) { assert.Equal(t, "keyring", got.(stubCache).source) } -func TestResolveCacheForLogin_FallsBackWhenProbeFails(t *testing.T) { +func TestResolveCacheForLogin_ExplicitEnvSecure_ProbeFail_Errors(t *testing.T) { hermetic(t) ctx := env.Set(t.Context(), EnvVar, "secure") configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") @@ -157,34 +157,79 @@ func TestResolveCacheForLogin_FallsBackWhenProbeFails(t *testing.T) { f := fakeFactories(t) f.probeKeyring = func() error { return errors.New("no keyring") } - got, mode, err := resolveCacheForLoginWith(ctx, "", f) - - require.NoError(t, err) - assert.Equal(t, StorageModePlaintext, mode) - assert.Equal(t, "file", got.(stubCache).source) + _, _, err := resolveCacheForLoginWith(ctx, "", f) + require.Error(t, err) + assert.ErrorContains(t, err, "secure storage was requested") - persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) - require.NoError(t, err) - assert.Equal(t, "plaintext", persisted, "auth_storage must be persisted on first fallback") + persisted, gerr := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, gerr) + assert.Equal(t, "", persisted, "env-set secure must not be persisted as plaintext") } -func TestResolveCacheForLogin_DoesNotOverwriteExistingAuthStorage(t *testing.T) { +func TestResolveCacheForLogin_ExplicitConfigSecure_ProbeFail_Errors(t *testing.T) { hermetic(t) - ctx := env.Set(t.Context(), EnvVar, "secure") + ctx := t.Context() configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") require.NoError(t, os.WriteFile(configPath, []byte("[__settings__]\nauth_storage = secure\n"), 0o600)) f := fakeFactories(t) f.probeKeyring = func() error { return errors.New("no keyring") } - _, mode, err := resolveCacheForLoginWith(ctx, "", f) + _, _, err := resolveCacheForLoginWith(ctx, "", f) + require.Error(t, err) + assert.ErrorContains(t, err, "secure storage was requested") + + persisted, gerr := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, gerr) + assert.Equal(t, "secure", persisted, "config-set secure must not be silently rewritten") +} + +func TestResolveCacheForLogin_ExplicitOverrideSecure_ProbeFail_Errors(t *testing.T) { + hermetic(t) + ctx := t.Context() + + f := fakeFactories(t) + f.probeKeyring = func() error { return errors.New("no keyring") } + + _, _, err := resolveCacheForLoginWith(ctx, StorageModeSecure, f) + require.Error(t, err) + assert.ErrorContains(t, err, "secure storage was requested") +} + +func TestApplyLoginFallback_DefaultSecure_ProbeFail_FallsBackAndPersists(t *testing.T) { + hermetic(t) + ctx := t.Context() + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") + + f := fakeFactories(t) + f.probeKeyring = func() error { return errors.New("no keyring") } + + got, mode, err := applyLoginFallback(ctx, StorageModeSecure, false, f) require.NoError(t, err) assert.Equal(t, StorageModePlaintext, mode) + assert.Equal(t, "file", got.(stubCache).source) persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) require.NoError(t, err) - assert.Equal(t, "secure", persisted, "existing auth_storage must not be overwritten by fallback") + assert.Equal(t, "plaintext", persisted, "default-mode fallback must persist auth_storage = plaintext") +} + +func TestApplyLoginFallback_ExplicitSecure_ProbeFail_Errors(t *testing.T) { + hermetic(t) + ctx := t.Context() + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") + + f := fakeFactories(t) + f.probeKeyring = func() error { return errors.New("no keyring") } + + _, _, err := applyLoginFallback(ctx, StorageModeSecure, true, f) + require.Error(t, err) + assert.ErrorContains(t, err, "secure storage was requested") + + persisted, gerr := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, gerr) + assert.Equal(t, "", persisted, "explicit-secure error must not write config") } func TestWrapForOAuthArgument(t *testing.T) { diff --git a/libs/auth/storage/mode.go b/libs/auth/storage/mode.go index b3dc846536b..2caace171f3 100644 --- a/libs/auth/storage/mode.go +++ b/libs/auth/storage/mode.go @@ -65,24 +65,37 @@ func ParseMode(raw string) StorageMode { // unrecognized env or config value is reported as an error wrapped with // the source name. func ResolveStorageMode(ctx context.Context, override StorageMode) (StorageMode, error) { + mode, _, err := ResolveStorageModeWithSource(ctx, override) + return mode, err +} + +// ResolveStorageModeWithSource is like ResolveStorageMode but also reports +// whether the resolved mode came from an explicit user choice (override flag, +// env var, or config) versus the built-in default. Callers use this to honor +// "I want secure" strictly: when the user explicitly asked for secure storage +// but it cannot be provided, the right move is to error out, not to silently +// downgrade. +func ResolveStorageModeWithSource(ctx context.Context, override StorageMode) (StorageMode, bool, error) { if override != StorageModeUnknown { - return override, nil + return override, true, nil } if raw := env.Get(ctx, EnvVar); raw != "" { - return parseFromSource(raw, EnvVar) + mode, err := parseFromSource(raw, EnvVar) + return mode, true, err } configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") raw, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) if err != nil { - return "", fmt.Errorf("read auth_storage setting: %w", err) + return "", false, fmt.Errorf("read auth_storage setting: %w", err) } if raw != "" { - return parseFromSource(raw, "auth_storage") + mode, err := parseFromSource(raw, "auth_storage") + return mode, true, err } - return StorageModePlaintext, nil + return StorageModePlaintext, false, nil } func parseFromSource(raw, source string) (StorageMode, error) { diff --git a/libs/auth/storage/mode_test.go b/libs/auth/storage/mode_test.go index d932d2253a2..d42eceabbce 100644 --- a/libs/auth/storage/mode_test.go +++ b/libs/auth/storage/mode_test.go @@ -128,3 +128,54 @@ func TestResolveStorageMode_SkipsConfigReadWhenOverrideOrEnvSet(t *testing.T) { assert.Equal(t, StorageModeSecure, got) }) } + +func TestResolveStorageModeWithSource(t *testing.T) { + cases := []struct { + name string + override StorageMode + envValue string + configBody string + wantMode StorageMode + wantExplicit bool + }{ + { + name: "default is not explicit", + wantMode: StorageModePlaintext, + wantExplicit: false, + }, + { + name: "override is explicit", + override: StorageModeSecure, + wantMode: StorageModeSecure, + wantExplicit: true, + }, + { + name: "env is explicit", + envValue: "secure", + wantMode: StorageModeSecure, + wantExplicit: true, + }, + { + name: "config is explicit", + configBody: "[__settings__]\nauth_storage = secure\n", + wantMode: StorageModeSecure, + wantExplicit: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfgPath := filepath.Join(t.TempDir(), ".databrickscfg") + if tc.configBody != "" { + require.NoError(t, os.WriteFile(cfgPath, []byte(tc.configBody), 0o600)) + } + t.Setenv("DATABRICKS_CONFIG_FILE", cfgPath) + t.Setenv(EnvVar, tc.envValue) + + mode, explicit, err := ResolveStorageModeWithSource(t.Context(), tc.override) + require.NoError(t, err) + assert.Equal(t, tc.wantMode, mode) + assert.Equal(t, tc.wantExplicit, explicit) + }) + } +} From 983cd1bcfd4c07780f57dd788c0fb9d394323214 Mon Sep 17 00:00:00 2001 From: simon Date: Wed, 6 May 2026 10:01:47 +0200 Subject: [PATCH 3/4] auth: address review feedback on probe + persist asymmetry - Collapse the four TestProbeKeyring_* tests into a single table. - Add error-case rows to TestResolveStorageModeWithSource (invalid env, invalid config). - Expand the persistPlaintextFallback comment to explain why we don't persist on the success paths (default + probe ok would lock the current default into the user's config; explicit secure already has the value set somewhere by definition). Co-authored-by: Isaac --- libs/auth/storage/cache.go | 15 ++++- libs/auth/storage/keyring_test.go | 95 +++++++++++++++++++------------ libs/auth/storage/mode_test.go | 16 ++++++ 3 files changed, 86 insertions(+), 40 deletions(-) diff --git a/libs/auth/storage/cache.go b/libs/auth/storage/cache.go index c2c99962441..69b5f6e7076 100644 --- a/libs/auth/storage/cache.go +++ b/libs/auth/storage/cache.go @@ -142,9 +142,18 @@ func applyLoginFallback(ctx context.Context, mode StorageMode, explicit bool, f } // persistPlaintextFallback writes auth_storage = plaintext to [__settings__] -// in .databrickscfg. Only called when the user did not explicitly choose -// secure, so overwriting any existing value would only happen if a previous -// fallback already wrote the same value: a no-op write. +// in .databrickscfg so subsequent commands skip the (slow/blocking) keyring +// probe and route straight to the file cache. +// +// We deliberately persist only on the default-mode + probe-fail path, never +// on the success paths: +// - default + probe ok: writing the runtime mode would lock the current +// default into the user's config and prevent a future change to the +// default from reaching them. +// - explicit secure (override, env, config): the value is already set +// somewhere by definition, so a write would be redundant. +// +// The fallback is the only path where persisting changes future behavior. func persistPlaintextFallback(ctx context.Context) error { configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") return databrickscfg.SetConfiguredAuthStorage(ctx, string(StorageModePlaintext), configPath) diff --git a/libs/auth/storage/keyring_test.go b/libs/auth/storage/keyring_test.go index fbfa4713daa..1057c75d20a 100644 --- a/libs/auth/storage/keyring_test.go +++ b/libs/auth/storage/keyring_test.go @@ -218,42 +218,63 @@ func TestKeyringCache_StoreNil_TimesOut(t *testing.T) { assert.ErrorAs(t, err, &timeoutErr, "expected TimeoutError, got %T: %v", err, err) } -func TestProbeKeyring_SuccessLeavesNoEntry(t *testing.T) { - backend := newFakeBackend() - - err := probeWithBackend(backend, 100*time.Millisecond) - require.NoError(t, err) - - _, ok := backend.items[itemKey(keyringServiceName, keyringProbeAccount)] - assert.False(t, ok, "probe must clean up after itself") -} - -func TestProbeKeyring_SetErrorPropagates(t *testing.T) { - boom := errors.New("no backend") - backend := newFakeBackend() - backend.setErr = boom - - err := probeWithBackend(backend, 100*time.Millisecond) - require.Error(t, err) - assert.ErrorIs(t, err, boom) -} - -func TestProbeKeyring_SetTimesOut(t *testing.T) { - backend := newFakeBackend() - backend.setBlock = true - - err := probeWithBackend(backend, 50*time.Millisecond) - require.Error(t, err) - var timeoutErr *TimeoutError - assert.ErrorAs(t, err, &timeoutErr) -} - -func TestProbeKeyring_DeleteErrorPropagates(t *testing.T) { - boom := errors.New("delete failed") - backend := newFakeBackend() - backend.deleteErr = boom +func TestProbeKeyring(t *testing.T) { + boom := errors.New("backend boom") + cases := []struct { + name string + setErr error + deleteErr error + setBlock bool + timeout time.Duration + wantErr error + wantTimeout bool + }{ + { + name: "success leaves no entry", + timeout: 100 * time.Millisecond, + }, + { + name: "set error propagates", + setErr: boom, + timeout: 100 * time.Millisecond, + wantErr: boom, + }, + { + name: "set times out", + setBlock: true, + timeout: 50 * time.Millisecond, + wantTimeout: true, + }, + { + name: "delete error propagates", + deleteErr: boom, + timeout: 100 * time.Millisecond, + wantErr: boom, + }, + } - err := probeWithBackend(backend, 100*time.Millisecond) - require.Error(t, err) - assert.ErrorIs(t, err, boom) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + backend := newFakeBackend() + backend.setErr = tc.setErr + backend.deleteErr = tc.deleteErr + backend.setBlock = tc.setBlock + + err := probeWithBackend(backend, tc.timeout) + + switch { + case tc.wantErr != nil: + require.Error(t, err) + assert.ErrorIs(t, err, tc.wantErr) + case tc.wantTimeout: + require.Error(t, err) + var timeoutErr *TimeoutError + assert.ErrorAs(t, err, &timeoutErr) + default: + require.NoError(t, err) + _, ok := backend.items[itemKey(keyringServiceName, keyringProbeAccount)] + assert.False(t, ok, "probe must clean up after itself") + } + }) + } } diff --git a/libs/auth/storage/mode_test.go b/libs/auth/storage/mode_test.go index d42eceabbce..bec3e571eb7 100644 --- a/libs/auth/storage/mode_test.go +++ b/libs/auth/storage/mode_test.go @@ -137,6 +137,7 @@ func TestResolveStorageModeWithSource(t *testing.T) { configBody string wantMode StorageMode wantExplicit bool + wantErrSub string }{ { name: "default is not explicit", @@ -161,6 +162,16 @@ func TestResolveStorageModeWithSource(t *testing.T) { wantMode: StorageModeSecure, wantExplicit: true, }, + { + name: "invalid env is rejected", + envValue: "bogus", + wantErrSub: "DATABRICKS_AUTH_STORAGE", + }, + { + name: "invalid config value is rejected", + configBody: "[__settings__]\nauth_storage = bogus\n", + wantErrSub: "auth_storage", + }, } for _, tc := range cases { @@ -173,6 +184,11 @@ func TestResolveStorageModeWithSource(t *testing.T) { t.Setenv(EnvVar, tc.envValue) mode, explicit, err := ResolveStorageModeWithSource(t.Context(), tc.override) + if tc.wantErrSub != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErrSub) + return + } require.NoError(t, err) assert.Equal(t, tc.wantMode, mode) assert.Equal(t, tc.wantExplicit, explicit) From f2295382c62e0b4304619e8b3b87550f8040dd0a Mon Sep 17 00:00:00 2001 From: simon Date: Wed, 6 May 2026 11:27:23 +0200 Subject: [PATCH 4/4] auth: expand persistPlaintextFallback comment on forward-compat Spell out the second reason we persist only on the fallback path: it pins those users to plaintext explicitly, so future changes to this logic can't accidentally regress them. They're already using plaintext implicitly (the keyring is unreachable), and the persisted setting just makes that choice stable across CLI versions. Co-authored-by: Isaac --- libs/auth/storage/cache.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libs/auth/storage/cache.go b/libs/auth/storage/cache.go index 69b5f6e7076..2801c8a6b82 100644 --- a/libs/auth/storage/cache.go +++ b/libs/auth/storage/cache.go @@ -154,6 +154,10 @@ func applyLoginFallback(ctx context.Context, mode StorageMode, explicit bool, f // somewhere by definition, so a write would be redundant. // // The fallback is the only path where persisting changes future behavior. +// It also pins these users to plaintext explicitly, so any future changes to +// this logic don't accidentally disrupt them: they're already using plaintext +// implicitly (the keyring is unreachable), and the persisted setting makes +// that choice stable across CLI versions. func persistPlaintextFallback(ctx context.Context) error { configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") return databrickscfg.SetConfiguredAuthStorage(ctx, string(StorageModePlaintext), configPath)