mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-14 23:21:35 +00:00
fix(cli): close TOCTOU window in lite up's settings backup write
write_backup wrote the backup (which can embed the original apiKeyHelper/settings content) with plain open() + a chmod call after the fact -- the same permissive-until-corrected window already fixed for autoroute's config.yaml and Claude settings writes, and missed entirely when the backup file already exists with broader permissions. Moves secure_create (atomic-enough 0600 via fchmod before any content is written) to up.py, the module both lite up and lite autoroute share, and has autoroute/process.py import it from there instead of keeping its own copy.
This commit is contained in:
parent
fbef12baa9
commit
cb978a570d
3 changed files with 42 additions and 26 deletions
|
|
@ -9,40 +9,19 @@ import threading
|
|||
import time
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
from typing import IO, Iterator
|
||||
|
||||
import click
|
||||
import requests
|
||||
from pydantic import TypeAdapter
|
||||
|
||||
from ..up import secure_create
|
||||
|
||||
AUTOROUTE_DIR = Path.home() / ".litellm" / "autorouter"
|
||||
CONFIG_PATH = AUTOROUTE_DIR / "config.yaml"
|
||||
LOG_PATH = AUTOROUTE_DIR / "proxy.log"
|
||||
PID_RECORD_PATH = AUTOROUTE_DIR / "proxy.pid.json"
|
||||
|
||||
|
||||
@contextlib.contextmanager
|
||||
def secure_create(path: Path) -> Iterator[IO[str]]:
|
||||
"""Open path for writing with mode 0600 fixed up before any content is written.
|
||||
|
||||
A plain `open(path, "w")` creates a *new* file at the umask-derived default (commonly 0644)
|
||||
and leaves it world- or group-readable until a later `chmod` call catches up -- a real window
|
||||
in which a file holding a credential (a proxy master key, a Claude Code auth token) is readable
|
||||
by another local account. Passing the mode to `os.open` closes that window for a brand-new
|
||||
file, but `O_CREAT`'s mode argument is only applied on creation: if the file already exists
|
||||
(the common case for `~/.claude/settings.json`, which normally predates `lite autoroute up`)
|
||||
its old, broader permissions carry over untouched. `os.fchmod` right after opening -- before a
|
||||
single byte of the new content is written -- covers both cases.
|
||||
"""
|
||||
fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
|
||||
os.fchmod(fd, 0o600)
|
||||
f: IO[str] = os.fdopen(fd, "w")
|
||||
try:
|
||||
yield f
|
||||
finally:
|
||||
f.close()
|
||||
|
||||
|
||||
class ProcessLaunchError(Exception):
|
||||
"""Raised when the ephemeral proxy subprocess fails to come up healthy."""
|
||||
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
import atexit
|
||||
import contextlib
|
||||
import json
|
||||
import os
|
||||
import shlex
|
||||
|
|
@ -9,7 +10,7 @@ import threading
|
|||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
from types import FrameType
|
||||
from typing import Mapping
|
||||
from typing import IO, Iterator, Mapping
|
||||
|
||||
import click
|
||||
from pydantic import JsonValue, TypeAdapter
|
||||
|
|
@ -71,12 +72,32 @@ def merge_claude_settings(
|
|||
return {**settings, ENV_KEY: env, API_KEY_HELPER_KEY: api_key_helper}
|
||||
|
||||
|
||||
@contextlib.contextmanager
|
||||
def secure_create(path: Path) -> Iterator[IO[str]]:
|
||||
"""Open path for writing with mode 0600 fixed up before any content is written.
|
||||
|
||||
A plain `open(path, "w")` creates a *new* file at the umask-derived default (commonly 0644)
|
||||
and leaves it world- or group-readable until a later `chmod` call catches up -- a real window
|
||||
in which a file holding a credential is readable by another local account. Passing the mode to
|
||||
`os.open` closes that window for a brand-new file, but `O_CREAT`'s mode argument is only
|
||||
applied on creation: if the file already exists its old, broader permissions carry over
|
||||
untouched. `os.fchmod` right after opening -- before a single byte of the new content is
|
||||
written -- covers both cases.
|
||||
"""
|
||||
fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
|
||||
os.fchmod(fd, 0o600)
|
||||
f: IO[str] = os.fdopen(fd, "w")
|
||||
try:
|
||||
yield f
|
||||
finally:
|
||||
f.close()
|
||||
|
||||
|
||||
def write_backup(record: BackupRecord, backup_path: Path | None = None) -> None:
|
||||
path = backup_path if backup_path is not None else BACKUP_PATH
|
||||
path.parent.mkdir(exist_ok=True)
|
||||
with open(path, "w") as f:
|
||||
with secure_create(path) as f:
|
||||
json.dump({"existed": record.existed, "content": record.content}, f, indent=2)
|
||||
os.chmod(path, 0o600)
|
||||
|
||||
|
||||
def read_backup(backup_path: Path | None = None) -> BackupRecord | None:
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import json
|
||||
import shutil
|
||||
import stat
|
||||
import sys
|
||||
from unittest.mock import patch
|
||||
|
||||
|
|
@ -129,6 +130,21 @@ class TestBackupRoundTrip:
|
|||
_patch_paths(monkeypatch, tmp_path)
|
||||
assert read_backup() is None
|
||||
|
||||
def test_write_backup_restricts_permissions_for_a_new_file(self, monkeypatch, tmp_path):
|
||||
_settings_path, backup_path = _patch_paths(monkeypatch, tmp_path)
|
||||
write_backup(BackupRecord(existed=True, content={"a": 1}))
|
||||
assert stat.S_IMODE(backup_path.stat().st_mode) == 0o600
|
||||
|
||||
def test_write_backup_restricts_permissions_of_a_preexisting_permissive_file(self, monkeypatch, tmp_path):
|
||||
_settings_path, backup_path = _patch_paths(monkeypatch, tmp_path)
|
||||
backup_path.parent.mkdir(parents=True, exist_ok=True)
|
||||
backup_path.write_text("{}")
|
||||
backup_path.chmod(0o644)
|
||||
|
||||
write_backup(BackupRecord(existed=True, content={"a": 1}))
|
||||
|
||||
assert stat.S_IMODE(backup_path.stat().st_mode) == 0o600
|
||||
|
||||
def test_backup_file_always_removed_after_restore(self, monkeypatch, tmp_path):
|
||||
_settings_path, backup_path = _patch_paths(monkeypatch, tmp_path)
|
||||
write_backup(BackupRecord(existed=False, content=None))
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue