Make secure token storage the default storage mode#5272
Open
simonfaltum wants to merge 6 commits into
Open
Conversation
U2M tokens for the databricks-cli auth type now write to the OS-native keyring by default. Users who need the previous file-backed cache can opt back via DATABRICKS_AUTH_STORAGE=plaintext or auth_storage = plaintext under [__settings__] in .databrickscfg; the env var takes precedence. The login-time keyring probe and fallback (already on main) activate with this change. Co-authored-by: Isaac
Contributor
Approval status: pending
|
When the resolver returns secure from the default (no env, no config), login now writes auth_storage = secure to [__settings__] after the keyring Store succeeds. Subsequent invocations see source=Config, so the explicit-secure branch of applyLoginFallback surfaces a transient keyring probe failure as an error instead of silently demoting the user to plaintext. Without this pin, a working secure-storage user could get stranded on the file cache after a single flaky probe. No-op when mode is plaintext (silent fallback already happened) or when the user already chose a mode explicitly. Persistence failures are logged at debug and never block login. Co-authored-by: Isaac
- login.go: move PinSecureMode call out of the existing comment block so the "At this point... / The rest of the command focuses on" narration stays together - cache.go: trim PinSecureMode doc comment and acknowledge that concurrent logins racing the write is benign because both write the same value - cache_test.go: drop the unused wantSkipMsg struct field; strengthen TestPinSecureMode_PersistFailureIsSwallowed to assert no file was written (and that the underlying os.OpenFile failure is the real trigger) - u2m-secure-default test.toml: rephrase the fixture comment to keep internal Go API names out of test config Co-authored-by: Isaac
After the default flipped to secure, any test that runs the login command on linux (no D-Bus) hits applyLoginFallback, which silently persists auth_storage = plaintext to whatever DATABRICKS_CONFIG_FILE points at. TestProfileHostCompatibleViaCobra points it at the checked- in cmd/auth/testdata/.databrickscfg fixture, so the test run leaves a dirty working tree and CI's `git diff --exit-code` step fails. Two changes: 1. Move ResolveCacheForLogin in login.go to run after input validation (cluster/serverless mutex + positional-arg check) rather than before. Trivially-invalid commands now fail without probing the keyring, so TestLoginRejectsPositionalArgWithHostFlag / WithProfileFlag no longer hit applyLoginFallback. The "resolve before browser step" property the original comment cared about is preserved: cache resolution still happens before NewPersistentAuth and Challenge. 2. Force plaintext via DATABRICKS_AUTH_STORAGE in TestProfileHostCompatibleViaCobra, which legitimately passes all input validation and reaches the resolver. The test is about flag compatibility, not storage mode; pinning it to plaintext keeps it hermetic. Co-authored-by: Isaac
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Part of CLI GA. Storing long-lived U2M refresh tokens in a plain JSON file in the user's home directory is a security weakness: any process with home-directory access can read them. Now that the CLI is being positioned as a building block for local agent workflows, we want tokens in the OS-native secure store by default.
Changes
Before:
databricks-cliauth tokens were written to~/.databricks/token-cache.json. SettingDATABRICKS_AUTH_STORAGE=secureopted in to the OS keyring.Now: tokens are written to the OS keyring (macOS Keychain, Windows Credential Manager, Linux Secret Service) by default. Setting
DATABRICKS_AUTH_STORAGE=plaintext(or[__settings__] auth_storage = plaintextin~/.databrickscfg) opts back to the file cache; env wins over config. Users re-rundatabricks auth loginonce after upgrade. Old tokens intoken-cache.jsonare not migrated.The login-time fallback that silently downgrades to plaintext when the user did not explicitly ask for secure and the keyring is unreachable (already on main as dormant infrastructure) is now active.
After a successful keyring write, the resolved mode is now pinned (
auth_storage = securewritten to[__settings__]). From then on, the resolver sees source=Config (explicit), so a transient keyring probe failure on a later login surfaces as an error instead of silently demoting a working user to plaintext. Pin is best-effort: persistence errors are logged at debug and never block login.Implementation:
libs/auth/storage/mode.go: resolver default flips fromStorageModePlaintexttoStorageModeSecure. Comments on the constants and the resolver doc updated.libs/auth/storage/cache.go: removed "dormant today" comments that no longer apply; addedPinSecureMode(ctx, mode).cmd/auth/login.go+cmd/auth/token.go: callstorage.PinSecureMode(ctx, mode)after eachpersistentAuth.Challenge()succeeds (main login, discoveryLogin, runInlineLogin).libs/auth/storage/andcmd/auth/describe_test.goupdated for the new default. NewTestPinSecureModetable-driven cases plus idempotence and persist-failure swallowing.acceptance/script.prepare: forcesDATABRICKS_AUTH_STORAGE=plaintextat the root so existing auth acceptance tests keep exercising the file-backed path. Tests that want the resolver default override it.acceptance/cmd/auth/describe/u2m-plaintext-defaultrenamed tou2m-secure-default; output updated to assert secure mode is reported as the default. A[[Repls]]regex in itstest.tomlnormalizes the platform-dependent keyring lookup error.acceptance/cmd/auth/describe/u2m-json-output: regenerated JSON output reflects the new default. Thejqfilter on.token_storagekeeps output deterministic.NEXT_CHANGELOG.md: breaking-change entry under Notable Changes documenting the flip, the re-login requirement, and both opt-out paths.Test plan
./task checksclean./task lint-qcleango test ./libs/auth/... ./cmd/auth/... ./libs/databrickscfg/...passesgo test ./acceptance -run 'TestAccept/cmd/auth'passes on macOSgo test ./acceptance -run 'TestAccept/cmd/configure'passes (covers adatabricks-cliauth path outsidecmd/auth)u2m-secure-defaultacceptance test relies on a[[Repls]]regex to canonicalize the keyring lookup error. If Linux output diverges in an unexpected way (e.g. error appears on a different line), the regex needs tightening.DATABRICKS_AUTH_STORAGEunset,databricks auth login --profile Xwrites to the keyring and persistsauth_storage = secureto[__settings__].DATABRICKS_AUTH_STORAGE=plaintext databricks auth login --profile Xcontinues to write to~/.databricks/token-cache.jsonwith the host-key dual-write entry;[__settings__]is not modified.This pull request and its description were written by Isaac.