diff --git a/litellm/litellm_core_utils/cli_token_utils.py b/litellm/litellm_core_utils/cli_token_utils.py index 3ea369767cf..b4e6d04cdf4 100644 --- a/litellm/litellm_core_utils/cli_token_utils.py +++ b/litellm/litellm_core_utils/cli_token_utils.py @@ -249,21 +249,44 @@ def clear_cli_token(*, vault: SecretVault = SYSTEM_KEYRING) -> SecretClear: outcome: Final = vault.erase() record: Final = _read_token_file() settled: Final = _nothing_left_behind(outcome, record) - left_on_disk: Final = _end_the_local_login(record) - unnoted: Final = _note_what_the_keychain_answered(outcome, record) - if unnoted is not None: - _put_the_token_file_back(unnoted) + left_on_disk: Final = _end_the_local_login(outcome, record) if left_on_disk is not None: return left_on_disk return SecretErased() if settled else outcome -def _put_the_token_file_back(record: CliTokenRecord) -> None: - with suppress(OSError): - _write_token_file(record) +def _end_the_local_login(outcome: SecretErase, record: CliTokenRecord | None) -> CredentialNotCleared | None: + if _names_a_login_the_keychain_kept(outcome, record): + return _move_token_file_to_note() + left_on_disk: Final = _remove_token_file_or_its_secret(record) + _note_what_the_keychain_answered(outcome, record) + return left_on_disk -def _end_the_local_login(record: CliTokenRecord | None) -> CredentialNotCleared | None: +def _names_a_login_the_keychain_kept(outcome: SecretErase, record: CliTokenRecord | None) -> bool: + match outcome: + case KeyringDisabled() | KeyringNotInstalled() | KeyringUnreachable(): + return record is not None and not _holds_a_secret(record) + case SecretErased() | SecretStranded(): + return False + + +def _move_token_file_to_note() -> CredentialNotCleared | None: + """Turn the token file of a login the keychain could not be asked to end into the note that ends it later. + + The file holds no secret, so it is already the record a later logout or login pairs with the + keychain entry to revoke the login, and renaming it needs no free space on a full disk. The file + never stays behind as a login that signs the user back in once the keychain unlocks: a note path + that will not take it leaves the file where it is, and the logout says so. + """ + try: + os.replace(get_cli_token_file_path(), _keychain_not_cleared_note_path()) + except OSError as error: + return CredentialNotCleared(str(error)) + return None + + +def _remove_token_file_or_its_secret(record: CliTokenRecord | None) -> CredentialNotCleared | None: removal: Final = _remove_token_file() if removal is None or record is None or _scrub_file_secret(record): return None @@ -318,48 +341,27 @@ def _read_keychain_not_cleared_note() -> CliTokenRecord | None: return None -def _note_what_the_keychain_answered(outcome: SecretErase, record: CliTokenRecord | None) -> CliTokenRecord | None: +def _note_what_the_keychain_answered(outcome: SecretErase, record: CliTokenRecord | None) -> None: """Keep the note that a logout left the keychain uncleared for exactly as long as that is true. - A keychain that could not be read kept this logout from revoking the login, so the note holds - that login's record without its secret, and the next logout or login that finds the keychain - answering pairs it with the entry to revoke it. One that would not give up its entry had already - handed this logout the refresh token, so its note holds nothing to revoke again. A login that - kept its secret in the file never reached the keychain, so the note an earlier logout left still - names the login the keychain holds and stays, and a logout with no file to end keeps it too. + A keychain that would not give up its entry had already handed this logout the refresh token, + so its note holds nothing to revoke again. A login that kept its secret in the file never + reached the keychain, so the note an earlier logout left still names the login the keychain + holds and stays, and a logout with no file to end keeps it too. - Only a note that cannot take an unrevoked login's record keeps that login from being revoked - later, so that record comes back for the token file to keep, as it did before the note existed. - Any other note that will not be written or removed is no reason to fail the logout, and the - warning printed now is then the only one the user gets. + The secret is off disk by the time this runs, so a note that will not be written or removed is + no reason to fail the logout. The warning printed now is then the only one the user gets. """ note: Final = _keychain_not_cleared_note_path() - match outcome: - case SecretErased(): - with suppress(OSError): - note.unlink(missing_ok=True) - return None - case SecretStranded(): - with suppress(OSError): - _write_keychain_not_cleared_note(note, None) - return None - case KeyringDisabled() | KeyringNotInstalled() | KeyringUnreachable(): - return _note_a_keychain_left_unchecked(note, record) - - -def _note_a_keychain_left_unchecked(note: Path, record: CliTokenRecord | None) -> CliTokenRecord | None: - if record is None: - return None - if not _holds_a_secret(record): - try: - _write_keychain_not_cleared_note(note, record) - except OSError: - return record - return None with suppress(OSError): - if not note.exists(): - _write_keychain_not_cleared_note(note, None) - return None + match outcome: + case SecretErased(): + note.unlink(missing_ok=True) + case SecretStranded(): + _write_keychain_not_cleared_note(note, None) + case KeyringDisabled() | KeyringNotInstalled() | KeyringUnreachable(): + if record is not None and not note.exists(): + _write_keychain_not_cleared_note(note, None) def _write_keychain_not_cleared_note(note: Path, record: CliTokenRecord | None) -> None: diff --git a/tests/unit/litellm_core_utils/test_cli_token_utils.py b/tests/unit/litellm_core_utils/test_cli_token_utils.py index ea19852ed56..19f9f263ee4 100644 --- a/tests/unit/litellm_core_utils/test_cli_token_utils.py +++ b/tests/unit/litellm_core_utils/test_cli_token_utils.py @@ -128,6 +128,10 @@ class _MkstempThatNeedsTheOldFileGone: return _REAL_MKSTEMP(*args, **kwargs) +def _full_disk_mkstemp(*args, **kwargs): + raise OSError(errno.ENOSPC, "No space left on device") + + _REAL_REPLACE = os.replace @@ -1022,7 +1026,8 @@ class TestClearCliToken: ): """A read-only `~/.litellm` accepts no removal of the token file and no new file beside it, and still lets the one it has be shortened. The secret goes, the file stays because nothing - else can, and the logout after it warns again rather than calling the keychain clean. + else can, and the logout after it says the file is still there, since a keychain that answers + again would sign it back in, rather than calling the machine logged out. The key is a realistic length so the file genuinely shrinks: a rewrite in place that leaves the tail of the old contents behind hands the next run a file it cannot parse.""" @@ -1031,7 +1036,7 @@ class TestClearCliToken: path.parent.chmod(0o500) try: assert clear_cli_token(vault=vault) == KeyringUnreachable() - assert clear_cli_token(vault=vault) == KeyringUnreachable() + assert isinstance(clear_cli_token(vault=vault), CredentialNotCleared) finally: path.parent.chmod(0o700) @@ -1053,6 +1058,24 @@ class TestClearCliToken: assert not path.exists() assert "sk-legacy" not in _everything_on_disk(isolated_home) + def test_a_full_disk_keeps_a_locked_keychain_login_revocable_without_signing_it_back_in( + self, isolated_home, secret_vault_factory, monkeypatch + ): + vault = secret_vault_factory() + save_cli_token( + CliTokenRecord(base_url=SERVER, key="sk-first", refresh_token="rt-first", timestamp=1000.0), vault=vault + ) + monkeypatch.setattr("litellm.litellm_core_utils.private_json.tempfile.mkstemp", _full_disk_mkstemp) + vault.available = False + + assert clear_cli_token(vault=vault) == KeyringUnreachable() + + vault.available = True + left = load_cli_token_left_in_keychain(vault=vault) + assert left is not None + assert left.refresh_token == "rt-first" + assert load_cli_token(vault=vault) is None + @pytest.mark.skipif(os.geteuid() == 0, reason="root ignores directory permissions") def test_a_metadata_file_that_will_not_go_is_not_worth_alarming_the_user_over( self, isolated_home, secret_vault_factory diff --git a/tests/unit/proxy/client/cli/test_auth_commands.py b/tests/unit/proxy/client/cli/test_auth_commands.py index 8737bd2e69d..babf44c1cc8 100644 --- a/tests/unit/proxy/client/cli/test_auth_commands.py +++ b/tests/unit/proxy/client/cli/test_auth_commands.py @@ -1984,6 +1984,7 @@ class TestPkceLogoutCommand: result = self.runner.invoke(logout, obj={"secret_vault": unlocked}) assert locked_logout.exit_code == 0, locked_logout.output + assert "could not be removed" in locked_logout.output assert result.output == "Logged out successfully. Authentication token cleared.\n" assert unlocked.blob is None assert [post for session in _FakeSession.instances for post in session.posts] == [