From 6ad7eaf11c0c908c7f13752a4e3ff770d2026a7b Mon Sep 17 00:00:00 2001 From: simon Date: Tue, 5 May 2026 11:11:43 +0200 Subject: [PATCH 1/7] 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/7] 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/7] 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/7] 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) From 7cbfed81343d55b827dc72cdc01b6d3fcee4243a Mon Sep 17 00:00:00 2001 From: simon Date: Thu, 7 May 2026 11:24:17 +0200 Subject: [PATCH 5/7] auth: pin auth_storage on first successful login regardless of mode Per review feedback: persisting only on the default-secure + probe-fail path is asymmetric. Pinning the resolved mode on every successful login locks in the first working behavior, smooths out keyring flakiness, and keeps subsequent invocations stable across CLI versions. The pin is idempotent: it skips the write when auth_storage is already set so a one-off override flag (--auth-storage plaintext) does not silently overwrite an existing user preference. Error paths still do not persist. Co-authored-by: Isaac --- .../configure-serverless/out.databrickscfg | 3 + .../login/configure-serverless/output.txt | 3 + .../auth/login/discovery/out.databrickscfg | 7 +- .../login/host-from-profile/out.databrickscfg | 3 + .../auth/login/host-from-profile/output.txt | 3 + .../cmd/auth/login/nominal/out.databrickscfg | 7 +- .../cmd/auth/login/preserve-fields/output.txt | 3 + .../auth/login/with-scopes/out.databrickscfg | 7 +- cmd/auth/auth_test.go | 19 +++- libs/auth/storage/cache.go | 106 ++++++++++-------- libs/auth/storage/cache_test.go | 55 +++++++++ libs/databrickscfg/ops.go | 6 +- 12 files changed, 160 insertions(+), 62 deletions(-) diff --git a/acceptance/cmd/auth/login/configure-serverless/out.databrickscfg b/acceptance/cmd/auth/login/configure-serverless/out.databrickscfg index b095b683c97..3a43f2c2bbd 100644 --- a/acceptance/cmd/auth/login/configure-serverless/out.databrickscfg +++ b/acceptance/cmd/auth/login/configure-serverless/out.databrickscfg @@ -3,3 +3,6 @@ host = [DATABRICKS_URL] serverless_compute_id = auto workspace_id = [NUMID] auth_type = databricks-cli + +[__settings__] +auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/configure-serverless/output.txt b/acceptance/cmd/auth/login/configure-serverless/output.txt index bb29a75f698..31a2e92dab3 100644 --- a/acceptance/cmd/auth/login/configure-serverless/output.txt +++ b/acceptance/cmd/auth/login/configure-serverless/output.txt @@ -14,3 +14,6 @@ host = [DATABRICKS_URL] serverless_compute_id = auto workspace_id = [NUMID] auth_type = databricks-cli + +[__settings__] +auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/discovery/out.databrickscfg b/acceptance/cmd/auth/login/discovery/out.databrickscfg index 56763df71ca..e9d29c5b629 100644 --- a/acceptance/cmd/auth/login/discovery/out.databrickscfg +++ b/acceptance/cmd/auth/login/discovery/out.databrickscfg @@ -1,11 +1,12 @@ ; The profile defined in the DEFAULT section is to be used as a fallback when no profile is explicitly specified. [DEFAULT] +[__settings__] +auth_storage = plaintext +default_profile = discovery-test + [discovery-test] host = [DATABRICKS_URL] account_id = test-account-123 workspace_id = [NUMID] auth_type = databricks-cli - -[__settings__] -default_profile = discovery-test diff --git a/acceptance/cmd/auth/login/host-from-profile/out.databrickscfg b/acceptance/cmd/auth/login/host-from-profile/out.databrickscfg index 0c13bde2570..72e358a3b1c 100644 --- a/acceptance/cmd/auth/login/host-from-profile/out.databrickscfg +++ b/acceptance/cmd/auth/login/host-from-profile/out.databrickscfg @@ -5,3 +5,6 @@ host = [DATABRICKS_URL] workspace_id = [NUMID] auth_type = databricks-cli + +[__settings__] +auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/host-from-profile/output.txt b/acceptance/cmd/auth/login/host-from-profile/output.txt index 6faae38ae5c..889840f8f3b 100644 --- a/acceptance/cmd/auth/login/host-from-profile/output.txt +++ b/acceptance/cmd/auth/login/host-from-profile/output.txt @@ -15,3 +15,6 @@ Profile existing-profile was successfully saved host = [DATABRICKS_URL] workspace_id = [NUMID] auth_type = databricks-cli + +[__settings__] +auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/nominal/out.databrickscfg b/acceptance/cmd/auth/login/nominal/out.databrickscfg index d94ee2221d5..c92ac51d7e1 100644 --- a/acceptance/cmd/auth/login/nominal/out.databrickscfg +++ b/acceptance/cmd/auth/login/nominal/out.databrickscfg @@ -1,10 +1,11 @@ ; The profile defined in the DEFAULT section is to be used as a fallback when no profile is explicitly specified. [DEFAULT] +[__settings__] +auth_storage = plaintext +default_profile = test + [test] host = [DATABRICKS_URL] workspace_id = [NUMID] auth_type = databricks-cli - -[__settings__] -default_profile = test diff --git a/acceptance/cmd/auth/login/preserve-fields/output.txt b/acceptance/cmd/auth/login/preserve-fields/output.txt index 28c0ed56608..3fce11501fc 100644 --- a/acceptance/cmd/auth/login/preserve-fields/output.txt +++ b/acceptance/cmd/auth/login/preserve-fields/output.txt @@ -20,3 +20,6 @@ azure_environment = USGOVERNMENT custom_key = my-custom-value workspace_id = [NUMID] auth_type = databricks-cli + +[__settings__] +auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/with-scopes/out.databrickscfg b/acceptance/cmd/auth/login/with-scopes/out.databrickscfg index c8af1832d70..b1cd5b03e0e 100644 --- a/acceptance/cmd/auth/login/with-scopes/out.databrickscfg +++ b/acceptance/cmd/auth/login/with-scopes/out.databrickscfg @@ -1,11 +1,12 @@ ; The profile defined in the DEFAULT section is to be used as a fallback when no profile is explicitly specified. [DEFAULT] +[__settings__] +auth_storage = plaintext +default_profile = scoped-test + [scoped-test] host = [DATABRICKS_URL] workspace_id = [NUMID] scopes = jobs,pipelines,clusters auth_type = databricks-cli - -[__settings__] -default_profile = scoped-test diff --git a/cmd/auth/auth_test.go b/cmd/auth/auth_test.go index c54d550f016..cce105bd3f3 100644 --- a/cmd/auth/auth_test.go +++ b/cmd/auth/auth_test.go @@ -1,6 +1,8 @@ package auth import ( + "os" + "path/filepath" "testing" "github.com/databricks/cli/cmd/root" @@ -10,6 +12,19 @@ import ( "github.com/stretchr/testify/require" ) +// copyTestCfgToTempDir copies the checked-in fixture to a temp dir and returns +// the temp path. Used by tests that drive the full login command end-to-end: +// auth login pins auth_storage to the config file on success, which would +// otherwise mutate the shared fixture. +func copyTestCfgToTempDir(t *testing.T) string { + t.Helper() + src, err := os.ReadFile("./testdata/.databrickscfg") + require.NoError(t, err) + dst := filepath.Join(t.TempDir(), ".databrickscfg") + require.NoError(t, os.WriteFile(dst, src, 0o600)) + return dst +} + func TestValidateProfileHostConflict(t *testing.T) { profiler := profile.InMemoryProfiler{ Profiles: profile.Profiles{ @@ -113,7 +128,9 @@ func TestProfileHostConflictTokenViaCobra(t *testing.T) { // pass the conflict check (the command will fail later for other reasons, but // NOT with a conflict error). func TestProfileHostCompatibleViaCobra(t *testing.T) { - t.Setenv("DATABRICKS_CONFIG_FILE", "./testdata/.databrickscfg") + // Use a temp copy: the compatible path runs through to RunE, where login + // pins auth_storage to the config file. + t.Setenv("DATABRICKS_CONFIG_FILE", copyTestCfgToTempDir(t)) ctx := cmdctx.GenerateExecId(t.Context()) cli := root.New(ctx) diff --git a/libs/auth/storage/cache.go b/libs/auth/storage/cache.go index 2801c8a6b82..9d4267937ce 100644 --- a/libs/auth/storage/cache.go +++ b/libs/auth/storage/cache.go @@ -46,13 +46,17 @@ func ResolveCache(ctx context.Context, override StorageMode) (cache.TokenCache, // 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. +// 1. After a successful resolution, pin the resolved mode by writing +// auth_storage to [__settings__] if the key is not already set. This +// locks in the first working behavior (secure or plaintext) so a +// subsequent invocation skips the keyring probe and doesn't oscillate +// between modes if the keyring becomes flaky. +// 2. 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. +// 3. 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 @@ -112,53 +116,57 @@ func resolveCacheForLoginWith(ctx context.Context, override StorageMode, f cache // 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 { - 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 { - return nil, "", fmt.Errorf("open file token cache: %w", fileErr) + switch mode { + case StorageModePlaintext: + c, err := f.newFile(ctx) + if err != nil { + return nil, "", fmt.Errorf("open file token cache: %w", err) } - if err := persistPlaintextFallback(ctx); err != nil { - log.Debugf(ctx, "persisting auth_storage fallback failed: %v", err) + pinResolvedMode(ctx, mode) + return c, mode, nil + case StorageModeSecure: + 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 { + return nil, "", fmt.Errorf("open file token cache: %w", fileErr) + } + pinResolvedMode(ctx, StorageModePlaintext) + return fileCache, StorageModePlaintext, nil } - return fileCache, StorageModePlaintext, nil + pinResolvedMode(ctx, mode) + return f.newKeyring(), mode, nil + default: + return nil, "", fmt.Errorf("unsupported storage mode %q", string(mode)) } - return f.newKeyring(), StorageModeSecure, nil } -// persistPlaintextFallback writes auth_storage = plaintext to [__settings__] -// in .databrickscfg so subsequent commands skip the (slow/blocking) keyring -// probe and route straight to the file cache. +// pinResolvedMode writes auth_storage = mode to [__settings__] in +// .databrickscfg only if the key is not already set. The first successful +// login pins whichever mode worked; later logins with a different transient +// source (override flag, env var) do not overwrite the user's pinned +// preference. Once pinned, ResolveStorageModeWithSource reads the value as +// "explicit" and the resolver routes straight to the chosen backend, which +// also makes the keyring probe redundant for subsequent secure logins. // -// 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. -// 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 { +// Best-effort: a write failure is logged at debug and not returned. Users +// have already authenticated successfully by the time we get here, and the +// only consequence of a missing pin is a redundant probe (or fallback) on +// the next login. +func pinResolvedMode(ctx context.Context, mode StorageMode) { configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") - return databrickscfg.SetConfiguredAuthStorage(ctx, string(StorageModePlaintext), configPath) + existing, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + if err != nil { + log.Debugf(ctx, "reading existing auth_storage failed: %v", err) + return + } + if existing != "" { + return + } + if err := databrickscfg.SetConfiguredAuthStorage(ctx, string(mode), configPath); err != nil { + log.Debugf(ctx, "persisting auth_storage = %s failed: %v", mode, err) + } } diff --git a/libs/auth/storage/cache_test.go b/libs/auth/storage/cache_test.go index 34d3f38c131..6c1cac90a95 100644 --- a/libs/auth/storage/cache_test.go +++ b/libs/auth/storage/cache_test.go @@ -123,6 +123,7 @@ func TestResolveCache_FileFactoryErrorPropagates(t *testing.T) { func TestResolveCacheForLogin_PlaintextSkipsProbe(t *testing.T) { hermetic(t) ctx := t.Context() + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") probed := false f := fakeFactories(t) f.probeKeyring = func() error { @@ -136,17 +137,26 @@ func TestResolveCacheForLogin_PlaintextSkipsProbe(t *testing.T) { 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") + + persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, err) + assert.Equal(t, "plaintext", persisted, "first login pins the resolved mode") } func TestResolveCacheForLogin_SecureProbeOK(t *testing.T) { hermetic(t) ctx := env.Set(t.Context(), EnvVar, "secure") + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") got, mode, err := resolveCacheForLoginWith(ctx, "", fakeFactories(t)) require.NoError(t, err) assert.Equal(t, StorageModeSecure, mode) assert.Equal(t, "keyring", got.(stubCache).source) + + persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, err) + assert.Equal(t, "secure", persisted, "first login pins the resolved mode") } func TestResolveCacheForLogin_ExplicitEnvSecure_ProbeFail_Errors(t *testing.T) { @@ -232,6 +242,51 @@ func TestApplyLoginFallback_ExplicitSecure_ProbeFail_Errors(t *testing.T) { assert.Equal(t, "", persisted, "explicit-secure error must not write config") } +func TestApplyLoginFallback_SecureProbeOK_PinsSecure(t *testing.T) { + hermetic(t) + ctx := t.Context() + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") + + _, mode, err := applyLoginFallback(ctx, StorageModeSecure, false, fakeFactories(t)) + require.NoError(t, err) + assert.Equal(t, StorageModeSecure, mode) + + persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, err) + assert.Equal(t, "secure", persisted) +} + +func TestApplyLoginFallback_PlaintextMode_PinsPlaintext(t *testing.T) { + hermetic(t) + ctx := t.Context() + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") + + _, mode, err := applyLoginFallback(ctx, StorageModePlaintext, false, fakeFactories(t)) + require.NoError(t, err) + assert.Equal(t, StorageModePlaintext, mode) + + persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, err) + assert.Equal(t, "plaintext", persisted) +} + +func TestApplyLoginFallback_AlreadyPinned_DoesNotOverwrite(t *testing.T) { + hermetic(t) + ctx := t.Context() + configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") + require.NoError(t, os.WriteFile(configPath, []byte("[__settings__]\nauth_storage = plaintext\n"), 0o600)) + + // Override forces secure for this run, but the existing pin must be preserved + // so a one-off override flag does not silently change a stable user preference. + _, mode, err := applyLoginFallback(ctx, StorageModeSecure, true, fakeFactories(t)) + require.NoError(t, err) + assert.Equal(t, StorageModeSecure, mode) + + persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) + require.NoError(t, err) + assert.Equal(t, "plaintext", persisted) +} + func TestWrapForOAuthArgument(t *testing.T) { const ( host = "https://example.com" diff --git a/libs/databrickscfg/ops.go b/libs/databrickscfg/ops.go index 43b3fc70005..b4045edc4ce 100644 --- a/libs/databrickscfg/ops.go +++ b/libs/databrickscfg/ops.go @@ -197,9 +197,9 @@ func SetDefaultProfile(ctx context.Context, profileName, configFilePath string) } // 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. +// section. Used by auth login to pin the resolved storage mode after a +// successful login so subsequent commands skip the keyring probe and route +// directly to the chosen backend. func SetConfiguredAuthStorage(ctx context.Context, value, configFilePath string) error { configFile, err := loadOrCreateConfigFile(ctx, configFilePath) if err != nil { From 2d06707292fafaa7a1c5b91ae2f45b760654170e Mon Sep 17 00:00:00 2001 From: simon Date: Thu, 7 May 2026 11:55:41 +0200 Subject: [PATCH 6/7] auth: defer pin-on-success to MS4 default flip Walks back the always-pin-on-success change. The resolver default is plaintext today, so pinning would freeze every user into plaintext and neutralize MS4's default flip to secure: after the flip, users with a pinned plaintext config would still resolve to plaintext. Keeps the silent-fallback persist (mode=Secure, explicit=false, probe fail -> plaintext). That branch is already gated naturally: it is unreachable today (default=plaintext means explicit=false only with mode=Plaintext) and lights up at MS4 alongside the default flip. Pin-on-success across modes is the correct end state but lands together with the default flip in MS4. Co-authored-by: Isaac --- .../configure-serverless/out.databrickscfg | 3 - .../login/configure-serverless/output.txt | 3 - .../auth/login/discovery/out.databrickscfg | 7 +-- .../login/host-from-profile/out.databrickscfg | 3 - .../auth/login/host-from-profile/output.txt | 3 - .../cmd/auth/login/nominal/out.databrickscfg | 7 +-- .../cmd/auth/login/preserve-fields/output.txt | 3 - .../auth/login/with-scopes/out.databrickscfg | 7 +-- cmd/auth/auth_test.go | 19 +----- libs/auth/storage/cache.go | 63 +++++++++---------- libs/auth/storage/cache_test.go | 55 ---------------- libs/databrickscfg/ops.go | 6 +- 12 files changed, 42 insertions(+), 137 deletions(-) diff --git a/acceptance/cmd/auth/login/configure-serverless/out.databrickscfg b/acceptance/cmd/auth/login/configure-serverless/out.databrickscfg index 3a43f2c2bbd..b095b683c97 100644 --- a/acceptance/cmd/auth/login/configure-serverless/out.databrickscfg +++ b/acceptance/cmd/auth/login/configure-serverless/out.databrickscfg @@ -3,6 +3,3 @@ host = [DATABRICKS_URL] serverless_compute_id = auto workspace_id = [NUMID] auth_type = databricks-cli - -[__settings__] -auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/configure-serverless/output.txt b/acceptance/cmd/auth/login/configure-serverless/output.txt index 31a2e92dab3..bb29a75f698 100644 --- a/acceptance/cmd/auth/login/configure-serverless/output.txt +++ b/acceptance/cmd/auth/login/configure-serverless/output.txt @@ -14,6 +14,3 @@ host = [DATABRICKS_URL] serverless_compute_id = auto workspace_id = [NUMID] auth_type = databricks-cli - -[__settings__] -auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/discovery/out.databrickscfg b/acceptance/cmd/auth/login/discovery/out.databrickscfg index e9d29c5b629..56763df71ca 100644 --- a/acceptance/cmd/auth/login/discovery/out.databrickscfg +++ b/acceptance/cmd/auth/login/discovery/out.databrickscfg @@ -1,12 +1,11 @@ ; The profile defined in the DEFAULT section is to be used as a fallback when no profile is explicitly specified. [DEFAULT] -[__settings__] -auth_storage = plaintext -default_profile = discovery-test - [discovery-test] host = [DATABRICKS_URL] account_id = test-account-123 workspace_id = [NUMID] auth_type = databricks-cli + +[__settings__] +default_profile = discovery-test diff --git a/acceptance/cmd/auth/login/host-from-profile/out.databrickscfg b/acceptance/cmd/auth/login/host-from-profile/out.databrickscfg index 72e358a3b1c..0c13bde2570 100644 --- a/acceptance/cmd/auth/login/host-from-profile/out.databrickscfg +++ b/acceptance/cmd/auth/login/host-from-profile/out.databrickscfg @@ -5,6 +5,3 @@ host = [DATABRICKS_URL] workspace_id = [NUMID] auth_type = databricks-cli - -[__settings__] -auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/host-from-profile/output.txt b/acceptance/cmd/auth/login/host-from-profile/output.txt index 889840f8f3b..6faae38ae5c 100644 --- a/acceptance/cmd/auth/login/host-from-profile/output.txt +++ b/acceptance/cmd/auth/login/host-from-profile/output.txt @@ -15,6 +15,3 @@ Profile existing-profile was successfully saved host = [DATABRICKS_URL] workspace_id = [NUMID] auth_type = databricks-cli - -[__settings__] -auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/nominal/out.databrickscfg b/acceptance/cmd/auth/login/nominal/out.databrickscfg index c92ac51d7e1..d94ee2221d5 100644 --- a/acceptance/cmd/auth/login/nominal/out.databrickscfg +++ b/acceptance/cmd/auth/login/nominal/out.databrickscfg @@ -1,11 +1,10 @@ ; The profile defined in the DEFAULT section is to be used as a fallback when no profile is explicitly specified. [DEFAULT] -[__settings__] -auth_storage = plaintext -default_profile = test - [test] host = [DATABRICKS_URL] workspace_id = [NUMID] auth_type = databricks-cli + +[__settings__] +default_profile = test diff --git a/acceptance/cmd/auth/login/preserve-fields/output.txt b/acceptance/cmd/auth/login/preserve-fields/output.txt index 3fce11501fc..28c0ed56608 100644 --- a/acceptance/cmd/auth/login/preserve-fields/output.txt +++ b/acceptance/cmd/auth/login/preserve-fields/output.txt @@ -20,6 +20,3 @@ azure_environment = USGOVERNMENT custom_key = my-custom-value workspace_id = [NUMID] auth_type = databricks-cli - -[__settings__] -auth_storage = plaintext diff --git a/acceptance/cmd/auth/login/with-scopes/out.databrickscfg b/acceptance/cmd/auth/login/with-scopes/out.databrickscfg index b1cd5b03e0e..c8af1832d70 100644 --- a/acceptance/cmd/auth/login/with-scopes/out.databrickscfg +++ b/acceptance/cmd/auth/login/with-scopes/out.databrickscfg @@ -1,12 +1,11 @@ ; The profile defined in the DEFAULT section is to be used as a fallback when no profile is explicitly specified. [DEFAULT] -[__settings__] -auth_storage = plaintext -default_profile = scoped-test - [scoped-test] host = [DATABRICKS_URL] workspace_id = [NUMID] scopes = jobs,pipelines,clusters auth_type = databricks-cli + +[__settings__] +default_profile = scoped-test diff --git a/cmd/auth/auth_test.go b/cmd/auth/auth_test.go index cce105bd3f3..c54d550f016 100644 --- a/cmd/auth/auth_test.go +++ b/cmd/auth/auth_test.go @@ -1,8 +1,6 @@ package auth import ( - "os" - "path/filepath" "testing" "github.com/databricks/cli/cmd/root" @@ -12,19 +10,6 @@ import ( "github.com/stretchr/testify/require" ) -// copyTestCfgToTempDir copies the checked-in fixture to a temp dir and returns -// the temp path. Used by tests that drive the full login command end-to-end: -// auth login pins auth_storage to the config file on success, which would -// otherwise mutate the shared fixture. -func copyTestCfgToTempDir(t *testing.T) string { - t.Helper() - src, err := os.ReadFile("./testdata/.databrickscfg") - require.NoError(t, err) - dst := filepath.Join(t.TempDir(), ".databrickscfg") - require.NoError(t, os.WriteFile(dst, src, 0o600)) - return dst -} - func TestValidateProfileHostConflict(t *testing.T) { profiler := profile.InMemoryProfiler{ Profiles: profile.Profiles{ @@ -128,9 +113,7 @@ func TestProfileHostConflictTokenViaCobra(t *testing.T) { // pass the conflict check (the command will fail later for other reasons, but // NOT with a conflict error). func TestProfileHostCompatibleViaCobra(t *testing.T) { - // Use a temp copy: the compatible path runs through to RunE, where login - // pins auth_storage to the config file. - t.Setenv("DATABRICKS_CONFIG_FILE", copyTestCfgToTempDir(t)) + t.Setenv("DATABRICKS_CONFIG_FILE", "./testdata/.databrickscfg") ctx := cmdctx.GenerateExecId(t.Context()) cli := root.New(ctx) diff --git a/libs/auth/storage/cache.go b/libs/auth/storage/cache.go index 9d4267937ce..8e8b779afe9 100644 --- a/libs/auth/storage/cache.go +++ b/libs/auth/storage/cache.go @@ -46,18 +46,20 @@ func ResolveCache(ctx context.Context, override StorageMode) (cache.TokenCache, // ResolveCacheForLogin resolves the cache like ResolveCache with extra rules // for the auth login path: // -// 1. After a successful resolution, pin the resolved mode by writing -// auth_storage to [__settings__] if the key is not already set. This -// locks in the first working behavior (secure or plaintext) so a -// subsequent invocation skips the keyring probe and doesn't oscillate -// between modes if the keyring becomes flaky. -// 2. When the resolved mode is secure and the user did not explicitly ask +// 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. -// 3. When the user explicitly asked for secure (override, env var, or +// is unreachable, fall back silently to plaintext and persist +// auth_storage = plaintext to [__settings__] so subsequent commands +// skip the (slow/blocking) probe and route directly to the file cache. +// 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. // +// Both rules 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. +// // 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. @@ -115,6 +117,12 @@ func resolveCacheForLoginWith(ctx context.Context, override StorageMode, f cache // 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. +// +// Pin-on-success across modes (locking in the first working behavior to +// insulate users from keyring flakiness) is intentionally not implemented +// here. It lands with MS4 alongside the default flip; pinning before the +// flip would freeze every default user into plaintext and make the flip a +// no-op for them. func applyLoginFallback(ctx context.Context, mode StorageMode, explicit bool, f cacheFactories) (cache.TokenCache, StorageMode, error) { switch mode { case StorageModePlaintext: @@ -122,7 +130,6 @@ func applyLoginFallback(ctx context.Context, mode StorageMode, explicit bool, f if err != nil { return nil, "", fmt.Errorf("open file token cache: %w", err) } - pinResolvedMode(ctx, mode) return c, mode, nil case StorageModeSecure: if probeErr := f.probeKeyring(); probeErr != nil { @@ -134,39 +141,27 @@ func applyLoginFallback(ctx context.Context, mode StorageMode, explicit bool, f if fileErr != nil { return nil, "", fmt.Errorf("open file token cache: %w", fileErr) } - pinResolvedMode(ctx, StorageModePlaintext) + if err := persistPlaintextFallback(ctx); err != nil { + log.Debugf(ctx, "persisting auth_storage fallback failed: %v", err) + } return fileCache, StorageModePlaintext, nil } - pinResolvedMode(ctx, mode) return f.newKeyring(), mode, nil default: return nil, "", fmt.Errorf("unsupported storage mode %q", string(mode)) } } -// pinResolvedMode writes auth_storage = mode to [__settings__] in -// .databrickscfg only if the key is not already set. The first successful -// login pins whichever mode worked; later logins with a different transient -// source (override flag, env var) do not overwrite the user's pinned -// preference. Once pinned, ResolveStorageModeWithSource reads the value as -// "explicit" and the resolver routes straight to the chosen backend, which -// also makes the keyring probe redundant for subsequent secure logins. +// persistPlaintextFallback writes auth_storage = plaintext to [__settings__] +// in .databrickscfg so subsequent commands skip the (slow/blocking) keyring +// probe and route straight to the file cache. // -// Best-effort: a write failure is logged at debug and not returned. Users -// have already authenticated successfully by the time we get here, and the -// only consequence of a missing pin is a redundant probe (or fallback) on -// the next login. -func pinResolvedMode(ctx context.Context, mode StorageMode) { +// Only called on the (mode=Secure, explicit=false) probe-failure branch. That +// branch is unreachable today (resolver default is plaintext), so this is +// dormant infrastructure: it activates when the default flips to secure +// (MS4) and lets default-on-broken-keyring users avoid a 3s probe on every +// command. +func persistPlaintextFallback(ctx context.Context) error { configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") - existing, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) - if err != nil { - log.Debugf(ctx, "reading existing auth_storage failed: %v", err) - return - } - if existing != "" { - return - } - if err := databrickscfg.SetConfiguredAuthStorage(ctx, string(mode), configPath); err != nil { - log.Debugf(ctx, "persisting auth_storage = %s failed: %v", mode, err) - } + return databrickscfg.SetConfiguredAuthStorage(ctx, string(StorageModePlaintext), configPath) } diff --git a/libs/auth/storage/cache_test.go b/libs/auth/storage/cache_test.go index 6c1cac90a95..34d3f38c131 100644 --- a/libs/auth/storage/cache_test.go +++ b/libs/auth/storage/cache_test.go @@ -123,7 +123,6 @@ func TestResolveCache_FileFactoryErrorPropagates(t *testing.T) { func TestResolveCacheForLogin_PlaintextSkipsProbe(t *testing.T) { hermetic(t) ctx := t.Context() - configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") probed := false f := fakeFactories(t) f.probeKeyring = func() error { @@ -137,26 +136,17 @@ func TestResolveCacheForLogin_PlaintextSkipsProbe(t *testing.T) { 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") - - persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) - require.NoError(t, err) - assert.Equal(t, "plaintext", persisted, "first login pins the resolved mode") } func TestResolveCacheForLogin_SecureProbeOK(t *testing.T) { hermetic(t) ctx := env.Set(t.Context(), EnvVar, "secure") - configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") got, mode, err := resolveCacheForLoginWith(ctx, "", fakeFactories(t)) require.NoError(t, err) assert.Equal(t, StorageModeSecure, mode) assert.Equal(t, "keyring", got.(stubCache).source) - - persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) - require.NoError(t, err) - assert.Equal(t, "secure", persisted, "first login pins the resolved mode") } func TestResolveCacheForLogin_ExplicitEnvSecure_ProbeFail_Errors(t *testing.T) { @@ -242,51 +232,6 @@ func TestApplyLoginFallback_ExplicitSecure_ProbeFail_Errors(t *testing.T) { assert.Equal(t, "", persisted, "explicit-secure error must not write config") } -func TestApplyLoginFallback_SecureProbeOK_PinsSecure(t *testing.T) { - hermetic(t) - ctx := t.Context() - configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") - - _, mode, err := applyLoginFallback(ctx, StorageModeSecure, false, fakeFactories(t)) - require.NoError(t, err) - assert.Equal(t, StorageModeSecure, mode) - - persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) - require.NoError(t, err) - assert.Equal(t, "secure", persisted) -} - -func TestApplyLoginFallback_PlaintextMode_PinsPlaintext(t *testing.T) { - hermetic(t) - ctx := t.Context() - configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") - - _, mode, err := applyLoginFallback(ctx, StorageModePlaintext, false, fakeFactories(t)) - require.NoError(t, err) - assert.Equal(t, StorageModePlaintext, mode) - - persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) - require.NoError(t, err) - assert.Equal(t, "plaintext", persisted) -} - -func TestApplyLoginFallback_AlreadyPinned_DoesNotOverwrite(t *testing.T) { - hermetic(t) - ctx := t.Context() - configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") - require.NoError(t, os.WriteFile(configPath, []byte("[__settings__]\nauth_storage = plaintext\n"), 0o600)) - - // Override forces secure for this run, but the existing pin must be preserved - // so a one-off override flag does not silently change a stable user preference. - _, mode, err := applyLoginFallback(ctx, StorageModeSecure, true, fakeFactories(t)) - require.NoError(t, err) - assert.Equal(t, StorageModeSecure, mode) - - persisted, err := databrickscfg.GetConfiguredAuthStorage(ctx, configPath) - require.NoError(t, err) - assert.Equal(t, "plaintext", persisted) -} - func TestWrapForOAuthArgument(t *testing.T) { const ( host = "https://example.com" diff --git a/libs/databrickscfg/ops.go b/libs/databrickscfg/ops.go index b4045edc4ce..43b3fc70005 100644 --- a/libs/databrickscfg/ops.go +++ b/libs/databrickscfg/ops.go @@ -197,9 +197,9 @@ func SetDefaultProfile(ctx context.Context, profileName, configFilePath string) } // SetConfiguredAuthStorage writes the auth_storage key to the [__settings__] -// section. Used by auth login to pin the resolved storage mode after a -// successful login so subsequent commands skip the keyring probe and route -// directly to the chosen backend. +// 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 { From cf8cd919fae0f853ec46d41e28ad2d9679e689ee Mon Sep 17 00:00:00 2001 From: simon Date: Thu, 7 May 2026 13:23:05 +0200 Subject: [PATCH 7/7] auth/storage: make probe account per-call random Fixed name "__probe__" could collide with a user profile of the same name. The keyring cache uses the profile name as the account field, so the probe would write a dummy token under that account and then delete it, destroying the user's real stored token. Use a UUID-suffixed account per probe. uuid is already a direct dep, no new module required. Co-authored-by: Isaac --- libs/auth/storage/keyring.go | 17 +++++++++++------ libs/auth/storage/keyring_test.go | 3 +-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/libs/auth/storage/keyring.go b/libs/auth/storage/keyring.go index d16428a57b0..4f00b881521 100644 --- a/libs/auth/storage/keyring.go +++ b/libs/auth/storage/keyring.go @@ -8,6 +8,7 @@ import ( "time" "github.com/databricks/databricks-sdk-go/credentials/u2m/cache" + "github.com/google/uuid" "github.com/zalando/go-keyring" "golang.org/x/oauth2" ) @@ -17,10 +18,13 @@ 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__" +// keyringProbeAccountPrefix is prefixed onto a per-call random suffix to form +// the account name ProbeKeyring writes and deletes. A fixed name like +// "__probe__" could collide with a user profile of the same name (which is +// what keyringCache uses as the account field), so the probe would clobber +// and delete that user's stored token. Per-call randomness also means +// concurrent probes don't step on each other. +const keyringProbeAccountPrefix = "__probe_" // defaultKeyringTimeout is how long a single keyring operation is allowed // to run before the wrapper returns a TimeoutError. Matches the value used @@ -102,11 +106,12 @@ func probeWithBackend(backend keyringBackend, timeout time.Duration) error { timeout: timeout, keyringSvcName: keyringServiceName, } + account := keyringProbeAccountPrefix + uuid.NewString() tok := &oauth2.Token{AccessToken: "probe"} - if err := c.Store(keyringProbeAccount, tok); err != nil { + if err := c.Store(account, tok); err != nil { return fmt.Errorf("write: %w", err) } - if err := c.Store(keyringProbeAccount, nil); err != nil { + if err := c.Store(account, nil); err != nil { return fmt.Errorf("delete: %w", err) } return nil diff --git a/libs/auth/storage/keyring_test.go b/libs/auth/storage/keyring_test.go index 1057c75d20a..f2b933ee42b 100644 --- a/libs/auth/storage/keyring_test.go +++ b/libs/auth/storage/keyring_test.go @@ -272,8 +272,7 @@ func TestProbeKeyring(t *testing.T) { 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") + assert.Empty(t, backend.items, "probe must clean up after itself") } }) }