diff --git a/litellm/litellm_core_utils/cli_token_utils.py b/litellm/litellm_core_utils/cli_token_utils.py index 15b1390f337..4e01dd723ce 100644 --- a/litellm/litellm_core_utils/cli_token_utils.py +++ b/litellm/litellm_core_utils/cli_token_utils.py @@ -153,19 +153,33 @@ def _keep_the_secret_in_the_file(record: CliTokenRecord, outcome: SecretWrite) - def clear_cli_token(*, vault: SecretVault = SYSTEM_KEYRING) -> SecretErase: """Remove the credential from both stores. Reports whether the keychain is now free of it. - A file that holds no secret of its own is kept when the keychain will not confirm the entry is - gone, because it is the only remaining record that something is still in there to remove. That - is what lets a later run tell a machine with a credential it cannot reach apart from one that - never had a login at all. Anything still holding a secret is removed either way. + A logout the keychain never answered keeps the token file, with its secret taken out, because + that file is the only remaining record that something may still be in there to remove. It is + what lets a later run tell a machine with a credential it cannot reach apart from one that never + had a login at all, and taking it away would leave the next logout answering the warning this + one just issued with a false all-clear. The secret goes either way. """ outcome: Final = vault.erase() record: Final = _read_token_file() settled: Final = _nothing_left_behind(outcome, record) - if settled or record is None or record.key is not None: + if settled or not _keep_the_unchecked_keychain_on_record(outcome, record): Path(get_cli_token_file_path()).unlink(missing_ok=True) return SecretErased() if settled else outcome +def _keep_the_unchecked_keychain_on_record(outcome: SecretErase, record: CliTokenRecord | None) -> bool: + """Whether the token file, stripped of its secret, is worth keeping as the note that says so. + + Only a keychain that could not be reached leaves the question open. One that answered for itself + is remembered without any help from the file, and a file it can still pair a live entry with + would leave the machine signed in to the login that was just ended. A copy that cannot be + replaced with a secret-free one is not kept either, because the secret goes first. + """ + if record is None or isinstance(outcome, SecretStranded): + return False + return _scrub_file_secret(record) + + def _nothing_left_behind(outcome: SecretErase, record: CliTokenRecord | None) -> bool: """Whether the keychain can be trusted to hold no credential of ours once the file is gone. 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 0cf1e55d363..162e5dd4b67 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 @@ -477,6 +477,18 @@ class TestClearCliToken: assert clear_cli_token(vault=vault) == SecretStranded() assert not _token_file(isolated_home).exists() + def test_a_keychain_that_will_not_release_the_secret_still_ends_the_local_login( + self, isolated_home, secret_vault_factory + ): + """The warning this returns says the machine is logged out locally and the keychain entry is + what is left over. Keeping the file that names that entry makes the first half untrue: every + later command reads the credential straight back out of the keychain and keeps working.""" + _write_metadata_only_file(isolated_home) + vault = secret_vault_factory(blob=_blob(), erasable=False) + + assert clear_cli_token(vault=vault) == SecretStranded() + assert load_cli_token(vault=vault) is None + @pytest.mark.parametrize( "failure", [KeyringDisabled(), KeyringUnreachable(), KeyringDiscardsWrites()] ) @@ -491,7 +503,7 @@ class TestClearCliToken: vault = secret_vault_factory(available=False, failure=failure) assert clear_cli_token(vault=vault) == failure - assert not _token_file(isolated_home).exists() + assert json.loads(_token_file(isolated_home).read_text()).get("key") is None def test_a_second_logout_still_reports_the_keychain_it_could_not_clear( self, isolated_home, secret_vault_factory @@ -539,7 +551,25 @@ class TestClearCliToken: clear_cli_token(vault=vault) - assert not _token_file(isolated_home).exists() + assert "sk-legacy" not in _token_file(isolated_home).read_text() + + def test_a_repeat_logout_never_answers_its_own_warning_with_an_all_clear( + self, isolated_home, secret_vault_factory + ): + """Sign in while the keychain works, sign in again once it has gone out of reach so the + second secret lands in the file, then log out twice. The first logout cannot say the first + login's entry is gone, and says so. If the second one reads the file the first one took + away as proof of a clean keychain, it retracts that warning while the credential behind it + is still live.""" + vault = secret_vault_factory() + save_cli_token(CliTokenRecord(base_url=SERVER, key="sk-first"), vault=vault) + vault.available = False + save_cli_token(CliTokenRecord(base_url=SERVER, key="sk-second"), vault=vault) + + assert clear_cli_token(vault=vault) == KeyringUnreachable() + assert clear_cli_token(vault=vault) == KeyringUnreachable() + assert vault.blob is not None + assert "sk-second" not in _token_file(isolated_home).read_text() @pytest.mark.parametrize("failure", [KeyringNotInstalled(), KeyringDisabled(), KeyringUnreachable()]) def test_logging_out_of_a_machine_that_never_logged_in_invents_nothing_to_warn_about( diff --git a/tests/test_litellm/proxy/client/cli/test_auth_commands.py b/tests/test_litellm/proxy/client/cli/test_auth_commands.py index e491dbd5aca..8e1551c0720 100644 --- a/tests/test_litellm/proxy/client/cli/test_auth_commands.py +++ b/tests/test_litellm/proxy/client/cli/test_auth_commands.py @@ -1074,7 +1074,7 @@ class TestKeychainBackedCommands: assert result.exit_code == 0 assert "could not be removed" in result.output - assert json.loads((isolated_home / ".litellm" / "token.json").read_text()).get("key") is None + assert not (isolated_home / ".litellm" / "token.json").exists() def test_print_token_explains_a_locked_keychain_instead_of_printing_nothing( self, isolated_home, secret_vault_factory