-
Notifications
You must be signed in to change notification settings - Fork 168
Make secure token storage the default storage mode #5272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9cee127
be527b0
3bf73db
a7eeb47
1561bfa
0ac6f88
ba66682
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| Ignore = [ | ||
| "home" | ||
| ] | ||
|
|
||
| # This test runs against the real OS keyring at Lookup time (no writes). | ||
| # macOS produces a clean miss; Linux without a usable D-Bus session bus | ||
| # produces a backend error. Normalize both so the assertion stays on the | ||
| # resolved storage mode, not the lookup outcome. | ||
| [[Repls]] | ||
| Old = 'Unable to authenticate: error getting token: .*' | ||
| New = 'Unable to authenticate: error getting token: [KEYRING_LOOKUP_ERROR]' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import ( | |
| "testing" | ||
|
|
||
| "github.com/databricks/cli/cmd/root" | ||
| "github.com/databricks/cli/libs/auth/storage" | ||
| "github.com/databricks/cli/libs/cmdctx" | ||
| "github.com/databricks/cli/libs/databrickscfg/profile" | ||
| "github.com/stretchr/testify/assert" | ||
|
|
@@ -114,6 +115,10 @@ func TestProfileHostConflictTokenViaCobra(t *testing.T) { | |
| // NOT with a conflict error). | ||
| func TestProfileHostCompatibleViaCobra(t *testing.T) { | ||
| t.Setenv("DATABRICKS_CONFIG_FILE", "./testdata/.databrickscfg") | ||
| // Force plaintext so the storage resolver does not probe the OS keyring | ||
| // and silently persist auth_storage = plaintext to the checked-in fixture | ||
| // on CI runners without a usable keyring. | ||
| t.Setenv(storage.EnvVar, "plaintext") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The outcome is the same, right? Or does setting this env var omit the |
||
|
|
||
| ctx := cmdctx.GenerateExecId(t.Context()) | ||
| cli := root.New(ctx) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,10 +59,6 @@ func ResolveCache(ctx context.Context, override StorageMode) (cache.TokenCache, | |
| // The timeout is ambiguous (locked vs hung); a misdiagnosis fails | ||
| // the final Store rather than silently downgrading to plaintext. | ||
| // | ||
| // Rules 1 and 2 are dormant today: the resolver default is plaintext, so | ||
| // (mode=Secure, explicit=false) is unreachable. They activate when the | ||
| // default flips to secure at GA. | ||
| // | ||
| // 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. | ||
|
|
@@ -120,12 +116,6 @@ 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 at GA 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: | ||
|
|
@@ -168,12 +158,38 @@ func applyLoginFallback(ctx context.Context, mode StorageMode, explicit bool, f | |
| // in .databrickscfg so subsequent commands skip the (slow/blocking) keyring | ||
| // probe and route straight to the file cache. | ||
| // | ||
| // 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 | ||
| // at GA and lets default-on-broken-keyring users avoid a 3s probe on every | ||
| // command. | ||
| // Only called on the (mode=Secure, explicit=false) probe-failure branch. | ||
| // Persisting the fallback 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") | ||
| return databrickscfg.SetConfiguredAuthStorage(ctx, string(StorageModePlaintext), configPath) | ||
| } | ||
|
|
||
| // PinSecureMode persists auth_storage = secure to [__settings__] when the | ||
| // user is currently on the secure-from-default path. Once pinned, subsequent | ||
| // invocations see source=Config (explicit), so applyLoginFallback returns an | ||
| // error on a transient keyring probe failure instead of silently demoting | ||
| // the user to plaintext. | ||
| // | ||
| // No-op when mode is not secure or when the user already chose a mode | ||
| // explicitly. Best-effort: persistence failures are logged at debug and | ||
| // never block login. Concurrent logins racing this write is benign because | ||
| // both write the same value. | ||
| func PinSecureMode(ctx context.Context, mode StorageMode) { | ||
| if mode != StorageModeSecure { | ||
| return | ||
| } | ||
| _, source, err := ResolveStorageModeWithSource(ctx, "") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not blocking: empty string for a typed value. This should say something like |
||
| if err != nil { | ||
| log.Debugf(ctx, "pin secure mode: resolve: %v", err) | ||
| return | ||
| } | ||
| if source.Explicit() { | ||
| return | ||
| } | ||
| configPath := env.Get(ctx, "DATABRICKS_CONFIG_FILE") | ||
| if err := databrickscfg.SetConfiguredAuthStorage(ctx, string(StorageModeSecure), configPath); err != nil { | ||
| log.Debugf(ctx, "pin secure mode: persist: %v", err) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as function on L164 except debug logging. Shouldn't both log at debug? |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u2m --> "OAuth tokens for interactive logins (
auth_type = databricks-cli)"...