mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
* fix(docker): heal pairing-dir ownership after `docker exec` writes (#10270) The official Docker image runs the gateway as the unprivileged `hermes` user (uid 10000) via `gosu`, but `docker exec` defaults to root. Approval files written by `docker exec <container> hermes pairing approve <code>` end up as `-rw------- root:root`, and the post-gosu gateway process cannot read them. The approval is silently ignored — the user keeps hitting 'Unauthorized user' on every message. The entrypoint's existing top-level chown is gated on the top-level $HERMES_HOME being mis-owned, so on warm boots (where /opt/data is already hermes:hermes) the recursive chown is skipped — meaning a container restart does NOT self-heal the bug either. Three-part fix: 1. docker/entrypoint.sh: chown the platforms/pairing/ (and legacy pairing/) subtree on every container start, regardless of the top-level decision. The directory is tiny (a few JSON files), so the unconditional chown is effectively free. Container restart now self-heals. 2. gateway/pairing.py: PairingStore._load_json was swallowing PermissionError under its bare 'except OSError' branch, which is what made this a silent failure. Split it out: log a WARNING that names the file, the gateway's uid, the file's owner/mode, and the exact docker exec -u hermes workaround. Still falls back to {} so the gateway stays up. 3. website/docs/user-guide/security.md: add a Docker tip to the pairing-CLI section pointing users at `docker exec -u hermes …` up front. Reproduced end-to-end in a containerized harness — before the fix the gateway sees 0 approved users after `docker exec` + restart; after the fix it sees the expected 1, and the file on disk goes from `root:root 600` back to `hermes:hermes 600` on next start. Fixes #10270 * fix(pairing): gate os.geteuid for Windows in PermissionError warning
This commit is contained in:
parent
e2fe529efb
commit
de7e0a8875
4 changed files with 131 additions and 0 deletions
|
|
@ -287,6 +287,23 @@ if [ -d "$HERMES_HOME/cron" ]; then
|
|||
chown_hermes_tree "$HERMES_HOME/cron"
|
||||
fi
|
||||
|
||||
# Always reset ownership of pairing data on every boot, same docker-exec/
|
||||
# root-write reason as profiles/ and cron/. `docker exec <container>
|
||||
# hermes pairing approve …` defaults to uid=0 and writes 0600 root-owned
|
||||
# approval files that the unprivileged hermes gateway cannot read,
|
||||
# silently leaving the approved user unauthorized (#10270). The targeted
|
||||
# data-volume chown above only runs when the top-level $HERMES_HOME is
|
||||
# mis-owned, so warm boots skip it — this block makes a container restart
|
||||
# self-heal. Tiny directory (a handful of small JSON files), so the cost
|
||||
# is negligible.
|
||||
if [ -d "$HERMES_HOME/platforms/pairing" ]; then
|
||||
chown_hermes_tree "$HERMES_HOME/platforms/pairing"
|
||||
fi
|
||||
# Legacy location (pre-consolidated layout).
|
||||
if [ -d "$HERMES_HOME/pairing" ]; then
|
||||
chown_hermes_tree "$HERMES_HOME/pairing"
|
||||
fi
|
||||
|
||||
# Reset ownership of hermes-owned top-level state files on every boot.
|
||||
# The targeted data-volume chown above only covers hermes-owned
|
||||
# *subdirectories*; loose state files living directly under $HERMES_HOME
|
||||
|
|
|
|||
|
|
@ -20,6 +20,7 @@ Storage: ~/.hermes/pairing/
|
|||
|
||||
import hashlib
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
import secrets
|
||||
import tempfile
|
||||
|
|
@ -35,6 +36,8 @@ from gateway.whatsapp_identity import (
|
|||
from hermes_constants import get_hermes_dir
|
||||
from utils import atomic_replace
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
# Unambiguous alphabet -- excludes 0/O, 1/I to prevent confusion
|
||||
ALPHABET = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789"
|
||||
|
|
@ -213,6 +216,30 @@ class PairingStore:
|
|||
if path.exists():
|
||||
try:
|
||||
return json.loads(path.read_text(encoding="utf-8"))
|
||||
except PermissionError as e:
|
||||
# Surface this loudly: a 0600 file owned by a different user
|
||||
# (classic Docker symptom: `docker exec` runs as root and writes
|
||||
# the file, then the gateway process — running as `hermes` after
|
||||
# gosu drop — can't read it) would otherwise be swallowed by
|
||||
# the generic OSError branch below, silently leaving the user
|
||||
# marked unauthorized. See issue #10270.
|
||||
try:
|
||||
st = path.stat()
|
||||
owner_info = f"owner_uid={st.st_uid} mode={oct(st.st_mode)[-4:]}"
|
||||
except OSError:
|
||||
owner_info = "<stat failed>"
|
||||
# os.geteuid doesn't exist on Windows; the Docker scenario is
|
||||
# POSIX-only, but the gateway (and this fallback) runs anywhere.
|
||||
euid = os.geteuid() if hasattr(os, "geteuid") else "n/a"
|
||||
logger.warning(
|
||||
"Pairing file %s exists but is not readable as uid=%s (%s; %s). "
|
||||
"If you ran `docker exec <container> hermes pairing approve ...` as root, "
|
||||
"re-run with `docker exec -u hermes <container> ...` and "
|
||||
"chown the existing file to the hermes user, or restart the "
|
||||
"container so the entrypoint can fix ownership.",
|
||||
path, euid, owner_info, e,
|
||||
)
|
||||
return {}
|
||||
except (json.JSONDecodeError, OSError):
|
||||
return {}
|
||||
return {}
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import json
|
|||
import os
|
||||
import sys
|
||||
import time
|
||||
from pathlib import Path
|
||||
from unittest.mock import patch
|
||||
|
||||
import pytest
|
||||
|
|
@ -651,3 +652,71 @@ class TestListAndClear:
|
|||
store.generate_code("discord", "user2")
|
||||
count = store.clear_pending()
|
||||
assert count == 2
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Unreadable approved-list file logs a warning instead of failing silently
|
||||
# (issue #10270: Docker `docker exec` writes root-owned 0600 files that the
|
||||
# post-gosu gateway can't read; the previous OSError swallow turned the bug
|
||||
# into a mystery "Unauthorized user" message)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestUnreadablePairingFile:
|
||||
def test_permission_error_logs_warning_and_returns_empty(self, tmp_path, caplog):
|
||||
import logging
|
||||
import builtins
|
||||
|
||||
approved_path = tmp_path / "weixin-approved.json"
|
||||
approved_path.write_text(
|
||||
'{"o9cq80fake@im.wechat": {"user_name": "x", "approved_at": 0}}'
|
||||
)
|
||||
|
||||
real_open = builtins.open
|
||||
|
||||
def fake_read_text(self, *a, **kw):
|
||||
# Path.read_text uses Path.open internally; raise PermissionError
|
||||
# to mimic a 0600 file owned by a different uid.
|
||||
raise PermissionError(13, "Permission denied", str(self))
|
||||
|
||||
with patch("gateway.pairing.PAIRING_DIR", tmp_path), \
|
||||
patch.object(Path, "read_text", fake_read_text), \
|
||||
caplog.at_level(logging.WARNING, logger="gateway.pairing"):
|
||||
store = PairingStore()
|
||||
result = store._load_json(approved_path)
|
||||
|
||||
assert result == {}, "should fall back to empty dict, not raise"
|
||||
assert any(
|
||||
"not readable" in rec.getMessage() and "#10270" not in rec.getMessage()
|
||||
or "not readable" in rec.getMessage()
|
||||
for rec in caplog.records
|
||||
), f"expected a warning about unreadable pairing file, got {caplog.records!r}"
|
||||
# And the warning should include actionable advice
|
||||
msgs = " ".join(rec.getMessage() for rec in caplog.records)
|
||||
assert "docker exec" in msgs
|
||||
assert "-u hermes" in msgs
|
||||
|
||||
def test_is_approved_returns_false_when_file_unreadable(self, tmp_path, caplog):
|
||||
"""End-to-end: an unreadable approved.json must not crash the gateway,
|
||||
and the affected user must stay unauthorized (the documented fallback
|
||||
behaviour) rather than triggering a 500."""
|
||||
import logging
|
||||
|
||||
approved_path = tmp_path / "weixin-approved.json"
|
||||
approved_path.write_text(
|
||||
'{"o9cq80fake@im.wechat": {"user_name": "x", "approved_at": 0}}'
|
||||
)
|
||||
|
||||
def fake_read_text(self, *a, **kw):
|
||||
raise PermissionError(13, "Permission denied", str(self))
|
||||
|
||||
with patch("gateway.pairing.PAIRING_DIR", tmp_path), \
|
||||
patch.object(Path, "read_text", fake_read_text), \
|
||||
caplog.at_level(logging.WARNING, logger="gateway.pairing"):
|
||||
store = PairingStore()
|
||||
ok = store.is_approved("weixin", "o9cq80fake@im.wechat")
|
||||
|
||||
assert ok is False
|
||||
# The warning must fire — otherwise this is the silent-failure bug.
|
||||
assert any(rec.levelno == logging.WARNING for rec in caplog.records), \
|
||||
"PermissionError on approved.json must produce a WARNING log line"
|
||||
|
|
|
|||
|
|
@ -332,6 +332,24 @@ hermes pairing revoke telegram 123456789
|
|||
hermes pairing clear-pending
|
||||
```
|
||||
|
||||
:::tip Docker users: run pairing commands as the `hermes` user
|
||||
The official Docker image runs the gateway as the unprivileged `hermes` user
|
||||
(uid 10000) via `gosu`, but `docker exec` defaults to root. Approval files
|
||||
created by root are written with mode `0600 root:root` and the gateway
|
||||
cannot read them — the approval is silently ignored ([#10270][i10270]).
|
||||
|
||||
Always pass `-u hermes`:
|
||||
|
||||
```bash
|
||||
docker exec -u hermes hermes-agent hermes pairing approve telegram ABC12DEF
|
||||
```
|
||||
|
||||
If you already ran the command as root and the user is still unauthorized,
|
||||
restart the container — the entrypoint will fix ownership on the next start.
|
||||
|
||||
[i10270]: https://github.com/NousResearch/hermes-agent/issues/10270
|
||||
:::
|
||||
|
||||
**Storage:** Pairing data is stored in `~/.hermes/pairing/` with per-platform JSON files:
|
||||
- `{platform}-pending.json` — pending pairing requests
|
||||
- `{platform}-approved.json` — approved users
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue