mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
fix(state): restrict sensitive store file permissions
response_store.db (api server) holds conversation history including tool payloads, prompts, and results. webhook_subscriptions.json holds per-route HMAC secrets. Under a permissive umask (e.g. 0o022, default on most distros) both files were created mode 0o644 — readable by other local users on shared boxes. - gateway/platforms/api_server.py: ResponseStore tightens itself + WAL/SHM sidecars to 0o600 after __init__, then trusts the inode. (Original contributor patch chmod'd after every _commit() — wasteful on a hot api_server path; chmod-on-create is sufficient since SQLite preserves mode bits across writes.) - hermes_cli/webhook.py: _save_subscriptions writes via tempfile.mkstemp (which itself creates the file with 0o600), chmods the temp before the atomic rename, and re-asserts 0o600 on the destination so an existing permissive file from before this fix gets narrowed. Tests cover (a) creation under permissive umask leaves 0o600 and (b) an existing 0o644 webhook_subscriptions.json gets narrowed on next save. Tests guarded with skipif os.name=='nt' since POSIX mode bits don't apply on Windows. Salvaged from PR #30917 by @Hinotoi-agent. Reworked the api_server.py side from chmod-on-every-commit to chmod-on-create. Co-authored-by: teknium1 <127238744+teknium1@users.noreply.github.com>
This commit is contained in:
parent
f378f00bfb
commit
3bace071bf
4 changed files with 116 additions and 5 deletions
|
|
@ -35,6 +35,7 @@ import re
|
|||
import sqlite3
|
||||
import time
|
||||
import uuid
|
||||
from pathlib import Path
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
try:
|
||||
|
|
@ -337,10 +338,12 @@ class ResponseStore:
|
|||
db_path = str(get_hermes_home() / "response_store.db")
|
||||
except Exception:
|
||||
db_path = ":memory:"
|
||||
self._db_path: Optional[str] = db_path if db_path != ":memory:" else None
|
||||
try:
|
||||
self._conn = sqlite3.connect(db_path, check_same_thread=False)
|
||||
except Exception:
|
||||
self._conn = sqlite3.connect(":memory:", check_same_thread=False)
|
||||
self._db_path = None
|
||||
# Use shared WAL-fallback helper so response_store.db degrades
|
||||
# gracefully on NFS/SMB/FUSE-mounted HERMES_HOME (same filesystem
|
||||
# issue addressed for state.db/kanban.db — see
|
||||
|
|
@ -361,6 +364,31 @@ class ResponseStore:
|
|||
)"""
|
||||
)
|
||||
self._conn.commit()
|
||||
# response_store.db contains conversation history (tool payloads,
|
||||
# prompts, results). Tighten to owner-only after creation so other
|
||||
# local users on a shared box can't read it. Run once at __init__
|
||||
# rather than after every commit — chmod-on-every-write is wasted
|
||||
# syscalls on a hot path.
|
||||
self._tighten_file_permissions()
|
||||
|
||||
def _tighten_file_permissions(self) -> None:
|
||||
"""Force owner-only permissions on the DB and SQLite sidecars."""
|
||||
if not self._db_path:
|
||||
return
|
||||
for candidate in (
|
||||
Path(self._db_path),
|
||||
Path(f"{self._db_path}-wal"),
|
||||
Path(f"{self._db_path}-shm"),
|
||||
):
|
||||
try:
|
||||
if candidate.exists():
|
||||
candidate.chmod(0o600)
|
||||
except OSError:
|
||||
logger.debug(
|
||||
"Failed to restrict response store permissions for %s",
|
||||
candidate,
|
||||
exc_info=True,
|
||||
)
|
||||
|
||||
def get(self, response_id: str) -> Optional[Dict[str, Any]]:
|
||||
"""Retrieve a stored response by ID (updates access time for LRU)."""
|
||||
|
|
|
|||
|
|
@ -11,8 +11,10 @@ hot-reloaded by the webhook adapter without a gateway restart.
|
|||
"""
|
||||
|
||||
import json
|
||||
import os
|
||||
import re
|
||||
import secrets
|
||||
import tempfile
|
||||
import time
|
||||
from pathlib import Path
|
||||
from typing import Dict
|
||||
|
|
@ -23,6 +25,7 @@ from hermes_cli.config import cfg_get
|
|||
|
||||
|
||||
_SUBSCRIPTIONS_FILENAME = "webhook_subscriptions.json"
|
||||
_SUBSCRIPTIONS_FILE_MODE = 0o600
|
||||
|
||||
|
||||
def _hermes_home() -> Path:
|
||||
|
|
@ -48,12 +51,33 @@ def _load_subscriptions() -> Dict[str, dict]:
|
|||
def _save_subscriptions(subs: Dict[str, dict]) -> None:
|
||||
path = _subscriptions_path()
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
tmp_path = path.with_suffix(".tmp")
|
||||
tmp_path.write_text(
|
||||
json.dumps(subs, indent=2, ensure_ascii=False),
|
||||
encoding="utf-8",
|
||||
# webhook_subscriptions.json contains per-route HMAC secrets — write
|
||||
# via tempfile + chmod 0o600 before the atomic rename so a permissive
|
||||
# umask cannot leave the secrets readable to other local users in the
|
||||
# window between create and rename.
|
||||
fd, tmp_name = tempfile.mkstemp(
|
||||
prefix=f".{path.name}.",
|
||||
suffix=".tmp",
|
||||
dir=path.parent,
|
||||
text=True,
|
||||
)
|
||||
atomic_replace(tmp_path, path)
|
||||
tmp_path = Path(tmp_name)
|
||||
try:
|
||||
with os.fdopen(fd, "w", encoding="utf-8") as fh:
|
||||
json.dump(subs, fh, indent=2, ensure_ascii=False)
|
||||
fh.flush()
|
||||
os.fsync(fh.fileno())
|
||||
os.chmod(tmp_path, _SUBSCRIPTIONS_FILE_MODE)
|
||||
atomic_replace(tmp_path, path)
|
||||
# Re-assert after rename in case the destination existed with a
|
||||
# broader mode and atomic_replace preserved it.
|
||||
os.chmod(path, _SUBSCRIPTIONS_FILE_MODE)
|
||||
except Exception:
|
||||
try:
|
||||
tmp_path.unlink(missing_ok=True)
|
||||
except OSError:
|
||||
pass
|
||||
raise
|
||||
|
||||
|
||||
def _get_webhook_config() -> dict:
|
||||
|
|
|
|||
|
|
@ -14,6 +14,8 @@ Tests cover:
|
|||
|
||||
import asyncio
|
||||
import json
|
||||
import os
|
||||
import stat
|
||||
import time
|
||||
import uuid
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
|
@ -128,6 +130,37 @@ class TestResponseStore:
|
|||
# resp_2 mapping should still be intact
|
||||
assert store.get_conversation("chat-b") == "resp_2"
|
||||
|
||||
@pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits are platform-specific")
|
||||
def test_file_store_created_owner_only_under_permissive_umask(self, tmp_path):
|
||||
"""response_store.db must be 0o600 on creation even under umask 022."""
|
||||
db_path = tmp_path / "response_store.db"
|
||||
store = None
|
||||
old_umask = os.umask(0o022)
|
||||
try:
|
||||
store = ResponseStore(max_size=10, db_path=str(db_path))
|
||||
store.put(
|
||||
"resp_secret",
|
||||
{
|
||||
"response": {"id": "resp_secret"},
|
||||
"conversation_history": [{"role": "tool", "content": "dummy-marker"}],
|
||||
},
|
||||
)
|
||||
finally:
|
||||
os.umask(old_umask)
|
||||
if store is not None:
|
||||
store.close()
|
||||
|
||||
assert stat.S_IMODE(db_path.stat().st_mode) == 0o600
|
||||
# WAL/SHM sidecars are owner-only too when present. WAL mode may be
|
||||
# unavailable on some filesystems (NFS/SMB) — only assert when the
|
||||
# sidecar files actually exist.
|
||||
for sidecar in (
|
||||
db_path.with_name(db_path.name + "-wal"),
|
||||
db_path.with_name(db_path.name + "-shm"),
|
||||
):
|
||||
if sidecar.exists():
|
||||
assert stat.S_IMODE(sidecar.stat().st_mode) == 0o600
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _IdempotencyCache
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@
|
|||
import json
|
||||
import os
|
||||
import pytest
|
||||
import stat
|
||||
from argparse import Namespace
|
||||
from pathlib import Path
|
||||
|
||||
|
|
@ -145,6 +146,31 @@ class TestPersistence:
|
|||
path.write_text("broken{{{")
|
||||
assert _load_subscriptions() == {}
|
||||
|
||||
@pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits are platform-specific")
|
||||
def test_save_creates_secret_file_owner_only_under_permissive_umask(self):
|
||||
old_umask = os.umask(0o022)
|
||||
try:
|
||||
_save_subscriptions({"demo": {"secret": "TOPSECRET", "prompt": "x"}})
|
||||
finally:
|
||||
os.umask(old_umask)
|
||||
|
||||
path = _subscriptions_path()
|
||||
assert stat.S_IMODE(path.stat().st_mode) == 0o600
|
||||
assert "TOPSECRET" in path.read_text(encoding="utf-8")
|
||||
|
||||
@pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits are platform-specific")
|
||||
def test_save_narrows_existing_broad_secret_file_mode(self):
|
||||
# Simulate a pre-existing 0o644 file from before this hardening landed.
|
||||
path = _subscriptions_path()
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.write_text(json.dumps({"old": {"secret": "stale", "prompt": "x"}}))
|
||||
path.chmod(0o644)
|
||||
|
||||
_save_subscriptions({"demo": {"secret": "FRESH", "prompt": "x"}})
|
||||
|
||||
assert stat.S_IMODE(path.stat().st_mode) == 0o600
|
||||
assert "FRESH" in path.read_text(encoding="utf-8")
|
||||
|
||||
|
||||
class TestWebhookEnabledGate:
|
||||
def test_blocks_when_disabled(self, capsys, monkeypatch):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue