From 0f1e09b5558160ad18db40387d63ebb90a1daf4a Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Thu, 20 Aug 2026 11:43:53 -0700 Subject: [PATCH] fix(cli): keep the refresh token in the OS keychain, not in token.json `lite login --pkce` mints a refresh token that buys a fresh key from the proxy on demand, so it is the credential just as much as the key is. Moving the key into the keychain left it behind in ~/.litellm/token.json, where any process running as the user can read it and renew the login for itself. It now travels with the key: `save_cli_token` writes both into the keychain entry, the token file keeps only metadata, and `lite logout` takes it out of the file whether or not the keychain answers. Upgrading finds one sign-in split across the two stores, the key already in the keychain and the refresh token still on disk. That case rejoins the two halves into a single entry before scrubbing the file, so the write never replaces a live key with nothing, and a machine that refuses the scrub keeps what it has rather than having the key rolled back out from under it. --- litellm/litellm_core_utils/cli_token_utils.py | 83 ++++++++--- litellm/proxy/client/README.md | 6 +- .../test_cli_token_utils.py | 130 +++++++++++++++++- 3 files changed, 194 insertions(+), 25 deletions(-) diff --git a/litellm/litellm_core_utils/cli_token_utils.py b/litellm/litellm_core_utils/cli_token_utils.py index b45513c5ea3..ee506a69ef9 100644 --- a/litellm/litellm_core_utils/cli_token_utils.py +++ b/litellm/litellm_core_utils/cli_token_utils.py @@ -4,10 +4,10 @@ CLI Token Utilities SDK-level utilities for reading the credential minted by `lite login`. Non-secret metadata lives in ~/.litellm/token.json. The secret material (the -bearer key, plus a JWT when one is issued) lives in the OS keychain when the -machine has one, and in that same 0600 file otherwise. This module hides the -split from callers, and migrates a legacy plaintext file into the keychain the -first time it reads one. +bearer key, the refresh token that renews it, and a JWT when one is issued) +lives in the OS keychain when the machine has one, and in that same 0600 file +otherwise. This module hides the split from callers, and migrates a plaintext +file into the keychain the first time it reads one. This module has no dependencies on proxy code and can be safely imported at the SDK level. """ @@ -111,13 +111,20 @@ class CliTokenSecret(BaseModel): secret minted for one server is never handed to another, even if the metadata file is edited underneath us. `timestamp` is the sign-in this secret came from, which is what decides it against a secret still on disk. + + Every field a thief could sign in with belongs here, which is why the + refresh token is one of them: it buys a fresh key from the proxy on demand, + so leaving it on disk would leave the login readable there. `key` is + optional because the file can hold a refresh token without one, and moving + that into the keychain must not invent a key to go with it. """ model_config = ConfigDict(frozen=True) base_url: str - key: str + key: str | None = None jwt_token: str = "" + refresh_token: str | None = None timestamp: float = 0.0 @@ -155,7 +162,7 @@ def save_cli_token(record: CliTokenRecord, *, vault: SecretVault = SYSTEM_KEYRIN staged: Final = _stage_token_file(_without_secret(stamped)) if isinstance(staged, CredentialNotSaved): return staged - outcome: Final = SecretStored() if stamped.key is None else vault.write(_encode_secret(stamped, stamped.key)) + outcome: Final = vault.write(_encode_secret(stamped)) if _holds_a_secret(stamped) else SecretStored() if isinstance(outcome, SecretStored): return outcome if _commit_token_file(staged) else CredentialNotRecorded() discard_staged_json(staged) @@ -390,8 +397,9 @@ def _apply_vault_secret(record: CliTokenRecord, blob: str, vault: SecretVault) - secret is usually left on disk by a keychain that would not take it, which makes the file the fresher of the two. It is the older one when a login the keychain did take could not replace the file afterwards, and serving that one would put a superseded credential back in use. Equal - stamps are one login sitting in both stores, left by a migration whose scrub was refused, so - that branch retries the migration rather than trading one credential for another. + stamps are one login sitting in both stores, left by a migration whose scrub was refused or by + an upgrade that took the key into the keychain and left the refresh token behind, so that branch + rejoins the halves and retries the migration rather than trading one credential for another. A scrub the file refuses leaves that superseded secret where it lies, which is the state the login already named when it could not replace the file, and which `lite logout` reports rather @@ -400,21 +408,46 @@ def _apply_vault_secret(record: CliTokenRecord, blob: str, vault: SecretVault) - superseded one back out. """ secret: Final = _decode_secret(blob, record.base_url) - if secret is None or (record.key is not None and secret.timestamp <= record.timestamp): - return _migrate_file_secret(record, vault) + if secret is None or (_holds_a_secret(record) and secret.timestamp <= record.timestamp): + return _migrate_file_secret(_rejoined(record, secret), vault, replacing=secret) _scrub_file_secret(record) return record.model_copy( update=MappingProxyType( { "key": secret.key, "jwt_token": secret.jwt_token, + "refresh_token": secret.refresh_token, "timestamp": max(secret.timestamp, record.timestamp), } ) ) -def _migrate_file_secret(record: CliTokenRecord, vault: SecretVault) -> CliTokenRecord | None: +def _rejoined(record: CliTokenRecord, secret: CliTokenSecret | None) -> CliTokenRecord: + """Put one sign-in's secret material back together when each store holds part of it. + + Upgrading from the release that kept only the key in the keychain leaves the refresh token + behind in the file, so a single login sits across both stores. Filling in whatever the file is + missing before the migration writes its entry is what stops that write from replacing a live key + with nothing. Only a matching stamp is one login. Two stamps are two logins, and pairing one's + key with the other's refresh token would build a credential neither store ever held. + """ + if secret is None or secret.timestamp != record.timestamp: + return record + return record.model_copy( + update=MappingProxyType( + { + "key": record.key if record.key is not None else secret.key, + "jwt_token": record.jwt_token or secret.jwt_token, + "refresh_token": record.refresh_token if record.refresh_token is not None else secret.refresh_token, + } + ) + ) + + +def _migrate_file_secret( + record: CliTokenRecord, vault: SecretVault, *, replacing: CliTokenSecret | None = None +) -> CliTokenRecord | None: """Move a file-held secret into the vault, but only once the file's copy can be taken away. The scrubbed file is staged first so a directory that will not accept it stops the migration @@ -426,23 +459,28 @@ def _migrate_file_secret(record: CliTokenRecord, vault: SecretVault) -> CliToken asked to take the new entry back, so the migration finishes on a directory that would only ever have refused it. Rolling back is the last resort, and a rollback the keychain also refuses leaves the secret in both stores until the next read, which retries this same migration. + + Only an entry this migration put there is taken back. `replacing` names one that was already in + the keychain, whose material the new entry carries forward, so erasing it would take away the + half the file never had, and a machine that refuses the scrub is exactly the one with nowhere + else to keep it. The next read finds the same two halves and tries the move again. """ - if record.key is None: + if not _holds_a_secret(record): return None staged: Final = _stage_scrubbed_file(record) if staged is None: return record - if not isinstance(vault.write(_encode_secret(record, record.key)), SecretStored): + if not isinstance(vault.write(_encode_secret(record)), SecretStored): discard_staged_json(staged) return record - if not _commit_token_file(staged) and not _overwrite_file_secret(record): + if not _commit_token_file(staged) and not _overwrite_file_secret(record) and replacing is None: vault.erase() return record def _scrub_file_secret(record: CliTokenRecord) -> bool: """Leave no secret material in the token file once the vault holds it""" - if record.key is None and not record.jwt_token: + if not _holds_a_secret(record): return True staged: Final = _stage_scrubbed_file(record) if staged is not None and _commit_token_file(staged): @@ -487,13 +525,22 @@ def _commit_token_file(staged: str) -> bool: return True +def _holds_a_secret(record: CliTokenRecord) -> bool: + """Whether the record carries anything that would sign someone in as this user""" + return record.key is not None or bool(record.jwt_token) or record.refresh_token is not None + + def _without_secret(record: CliTokenRecord) -> CliTokenRecord: - return record.model_copy(update=MappingProxyType({"key": None, "jwt_token": ""})) + return record.model_copy(update=MappingProxyType({"key": None, "jwt_token": "", "refresh_token": None})) -def _encode_secret(record: CliTokenRecord, key: str) -> str: +def _encode_secret(record: CliTokenRecord) -> str: return CliTokenSecret( - base_url=record.base_url, key=key, jwt_token=record.jwt_token, timestamp=record.timestamp + base_url=record.base_url, + key=record.key, + jwt_token=record.jwt_token, + refresh_token=record.refresh_token, + timestamp=record.timestamp, ).model_dump_json() diff --git a/litellm/proxy/client/README.md b/litellm/proxy/client/README.md index 3a39bec5ee6..1fff68677cc 100644 --- a/litellm/proxy/client/README.md +++ b/litellm/proxy/client/README.md @@ -331,7 +331,7 @@ sequenceDiagram CLI->>Proxy: Poll /sso/cli/poll/login_id with poll_secret header Proxy->>CLI: Return {"status": "ready", "key": "jwt"} - CLI->>CLI: Save key to the OS keychain (metadata to ~/.litellm/token.json) + CLI->>CLI: Save the secret to the OS keychain (metadata to ~/.litellm/token.json) ``` ### Authentication Commands @@ -365,7 +365,7 @@ The CLI provides these authentication commands: ### Token Storage -The key itself goes into the OS keychain (macOS Keychain, Windows Credential Manager, or the Linux Secret Service) under service `litellm-cli`, account `credential`. Only the non-secret session metadata is written to `~/.litellm/token.json`, in a `0700` directory with `0600` file permissions: +The key itself, together with the refresh token that renews a `--pkce` credential, goes into the OS keychain (macOS Keychain, Windows Credential Manager, or the Linux Secret Service) under service `litellm-cli`, account `credential`. Only the non-secret session metadata is written to `~/.litellm/token.json`, in a `0700` directory with `0600` file permissions: ```json { @@ -378,7 +378,7 @@ The key itself goes into the OS keychain (macOS Keychain, Windows Credential Man } ``` -Keychain storage needs the `keyring` package, which ships with `pip install 'litellm[cli]'`. Headless boxes and CI runners usually have no keychain either. In all of those cases the key stays in the same `0600` file alongside the metadata, exactly as it did before, and `lite login` names which one applies: the package is missing, the machine has no keychain, or you set `LITELLM_CLI_DISABLE_KEYRING=1` to force the file even where a keychain exists. A `token.json` written by an older `lite` keeps working and is moved into the keychain, and scrubbed from the file, the first time a keychain-capable `lite` reads it. +Keychain storage needs the `keyring` package, which ships with `pip install 'litellm[cli]'`. Headless boxes and CI runners usually have no keychain either. In all of those cases the key and the refresh token stay in the same `0600` file alongside the metadata, exactly as they did before, and `lite login` names which one applies: the package is missing, the machine has no keychain, or you set `LITELLM_CLI_DISABLE_KEYRING=1` to force the file even where a keychain exists. A `token.json` written by an older `lite` keeps working and is moved into the keychain, and scrubbed from the file, the first time a keychain-capable `lite` reads it. That includes a refresh token left behind by the release that moved only the key. `lite logout` clears both stores. If the keychain is locked at that moment it says so, and re-running it once the keychain is unlocked finishes the job. diff --git a/tests/test_litellm/litellm_core_utils/test_cli_token_utils.py b/tests/test_litellm/litellm_core_utils/test_cli_token_utils.py index b8ac1987453..7e7eee5373f 100644 --- a/tests/test_litellm/litellm_core_utils/test_cli_token_utils.py +++ b/tests/test_litellm/litellm_core_utils/test_cli_token_utils.py @@ -80,8 +80,28 @@ def _write_metadata_only_file(home): return path -def _blob(base_url=SERVER, key="sk-vault", jwt_token="", timestamp=0.0): - return json.dumps({"base_url": base_url, "key": key, "jwt_token": jwt_token, "timestamp": timestamp}) +def _blob(base_url=SERVER, key="sk-vault", jwt_token="", timestamp=0.0, refresh_token=None): + return json.dumps( + { + "base_url": base_url, + "key": key, + "jwt_token": jwt_token, + "refresh_token": refresh_token, + "timestamp": timestamp, + } + ) + + +def _write_key_only_keychain_file(home, *, refresh_token="rt-live", timestamp=2000.0): + """What the release that kept only the key in the keychain left on disk: metadata, plus the + refresh token in the clear.""" + path = _token_file(home) + path.parent.mkdir(exist_ok=True) + path.write_text( + json.dumps({"base_url": SERVER, "user_id": "u-1", "refresh_token": refresh_token, "timestamp": timestamp}) + ) + path.chmod(0o600) + return path _REAL_MKSTEMP = tempfile.mkstemp @@ -161,6 +181,56 @@ class TestLoadCliToken: assert (record.key, record.jwt_token) == ("sk-a", "jwt-a") + def test_the_refresh_token_round_trips_through_the_vault(self, isolated_home, secret_vault_factory): + """A refresh token mints a fresh key from the proxy on demand, so it is the credential just + as much as the key is, and it has to come back out of the keychain to be usable.""" + _write_metadata_only_file(isolated_home) + vault = secret_vault_factory(blob=_blob(key="sk-a", refresh_token="rt-a")) + + record = load_cli_token(vault=vault) + + assert (record.key, record.refresh_token) == ("sk-a", "rt-a") + + def test_a_plaintext_refresh_token_is_moved_off_disk(self, isolated_home, secret_vault_factory): + path = _write_legacy_file(isolated_home, refresh_token="rt-legacy") + vault = secret_vault_factory() + + record = load_cli_token(vault=vault) + + assert record.refresh_token == "rt-legacy" + assert "rt-legacy" not in path.read_text() + assert json.loads(vault.blob)["refresh_token"] == "rt-legacy" + + def test_an_upgrade_that_left_the_refresh_token_on_disk_rejoins_it_with_the_key( + self, isolated_home, secret_vault_factory + ): + """The release before this one took the key into the keychain and left the refresh token + behind, so upgrading finds one sign-in split across both stores. The read has to end with + the whole credential in the keychain, not with whichever half it happened to prefer.""" + path = _write_key_only_keychain_file(isolated_home) + vault = secret_vault_factory(blob=_blob(key="sk-live", timestamp=2000.0)) + + record = load_cli_token(vault=vault) + + assert (record.key, record.refresh_token) == ("sk-live", "rt-live") + assert "rt-live" not in path.read_text() + assert json.loads(vault.blob)["key"] == "sk-live" + assert json.loads(vault.blob)["refresh_token"] == "rt-live" + + def test_a_superseded_refresh_token_on_disk_never_outlives_the_keychain( + self, isolated_home, secret_vault_factory + ): + """Two stores, two sign-ins, and the newer one is in the keychain. Handing back its key with + the older one's refresh token would build a credential neither store ever held, and would + renew the login the user already replaced.""" + path = _write_legacy_file(isolated_home, key="sk-old", refresh_token="rt-old", timestamp=1000.0) + vault = secret_vault_factory(blob=_blob(key="sk-new", refresh_token="rt-new", timestamp=2000.0)) + + record = load_cli_token(vault=vault) + + assert (record.key, record.refresh_token) == ("sk-new", "rt-new") + assert "rt-old" not in path.read_text() + def test_legacy_plaintext_file_still_authenticates_and_is_migrated(self, isolated_home, secret_vault_factory): """A token.json written by an older `lite` keeps working, and reading it moves the secret into the keychain and scrubs it from disk.""" @@ -361,6 +431,36 @@ class TestSaveCliToken: assert json.loads(vault.blob)["key"] == "sk-new" assert load_cli_token(vault=vault).key == "sk-new" + def test_the_refresh_token_goes_to_the_keychain_and_never_to_the_file( + self, isolated_home, secret_vault_factory + ): + vault = secret_vault_factory() + + stored = save_cli_token( + CliTokenRecord(base_url=SERVER, key="sk-new", refresh_token="rt-new", timestamp=time.time()), + vault=vault, + ) + + assert stored == SecretStored() + assert "rt-new" not in _token_file(isolated_home).read_text() + assert json.loads(vault.blob)["refresh_token"] == "rt-new" + assert load_cli_token(vault=vault).refresh_token == "rt-new" + + def test_the_refresh_token_falls_back_to_the_owner_only_file_with_the_key( + self, isolated_home, secret_vault_factory + ): + """A machine with no keychain keeps the whole credential in the 0600 file, refresh token + included, because a renewal that cannot be stored logs the user out on the next command.""" + vault = secret_vault_factory(available=False, failure=KeyringNotInstalled()) + + save_cli_token( + CliTokenRecord(base_url=SERVER, key="sk-new", refresh_token="rt-new", timestamp=time.time()), + vault=vault, + ) + + assert json.loads(_token_file(isolated_home).read_text())["refresh_token"] == "rt-new" + assert load_cli_token(vault=vault).refresh_token == "rt-new" + def test_falls_back_to_the_owner_only_file_when_there_is_no_keychain(self, isolated_home, secret_vault_factory): stored = save_cli_token( CliTokenRecord(base_url=SERVER, key="sk-new", timestamp=time.time()), @@ -628,6 +728,26 @@ class TestScrubFailure: assert json.loads(path.read_text()).get("key") is None + @pytest.mark.skipif(os.geteuid() == 0, reason="root ignores file permissions") + def test_a_rejoin_the_file_refuses_never_takes_the_key_with_it( + self, isolated_home, secret_vault_factory, monkeypatch + ): + """Rolling the rejoined entry back would erase a key that was safely in the keychain before + this read began, and the file it would fall back to is the one that has just refused to be + rewritten. The duplicate refresh token stays until a later read can finish the move.""" + path = _write_key_only_keychain_file(isolated_home) + vault = secret_vault_factory(blob=_blob(key="sk-live", timestamp=2000.0)) + monkeypatch.setattr("litellm.litellm_core_utils.private_json.os.replace", _refuse_replace) + path.chmod(0o400) + + record = load_cli_token(vault=vault) + + assert (record.key, record.refresh_token) == ("sk-live", "rt-live") + assert json.loads(vault.blob)["key"] == "sk-live" + assert json.loads(vault.blob)["refresh_token"] == "rt-live" + assert vault.erases == 0 + + class TestClearCliToken: def test_removes_the_credential_from_both_stores(self, isolated_home, secret_vault_factory): vault = secret_vault_factory() @@ -717,12 +837,14 @@ class TestClearCliToken: ): """Keeping a record of the unreachable keychain must never mean keeping the cleartext copy the user just asked to be rid of.""" - _write_legacy_file(isolated_home) + _write_legacy_file(isolated_home, refresh_token="rt-legacy") vault = secret_vault_factory(available=False, failure=KeyringUnreachable()) clear_cli_token(vault=vault) - assert "sk-legacy" not in _token_file(isolated_home).read_text() + left_on_disk = _token_file(isolated_home).read_text() + assert "sk-legacy" not in left_on_disk + assert "rt-legacy" not in left_on_disk def test_a_repeat_logout_never_answers_its_own_warning_with_an_all_clear( self, isolated_home, secret_vault_factory