fix(gateway): stop revoking managed-mode group access on the flush dir - #77717
fix(gateway): stop revoking managed-mode group access on the flush dir#77717ZHJay wants to merge 1 commit into
Conversation
`_get_flush_dir()` created `$HERMES_HOME/pending_messages` with an unconditional `mode=0o700` and then chmod'd it 0700 again, so on a managed (NixOS) install both mechanisms overrode permissions the module deliberately configures. `nix/nixosModules.nix` pre-creates only stateDir, .hermes, cron, sessions, logs, memories, plugins at 2770 (setgid, group-rwx) via systemd.tmpfiles (lines 711-719, mirrored by the activation loop at 740-743). `pending_messages` is NOT in those rules, so on a managed host it is always created lazily at runtime and these two lines were the only thing setting its mode. The service runs with `UMask = "0007"`, commented "files created by the gateway should be group-writable so interactive users in the hermes group can read/write them" (line 907), and the activation script avoids `chown -R` specifically to keep the setgid bit alive "for group access by hostUsers" (lines 135-136). `container.hostUsers` get a `~/.hermes` symlink to that same stateDir, so the gateway service and an interactive CLI share one `$HERMES_HOME` at two different uids. Measured under real managed conditions (2770 parent, umask 0007), dir absent: before pending_messages 0o700 (no group) payload 0o600 after pending_messages 0o770 (group-rwx) payload 0o600 The payload files stay 0o600 either way, so this widens the directory only — which is what has to widen, because the directory mode is what gates the *second* process. Measured: `mkdir(exist_ok=True)` on a dir owned by the other uid succeeds silently, and the `mkstemp` inside it then fails EACCES, so the flush is lost rather than merely unreadable. At 0770 it succeeds. Blast radius is real but bounded, and worth stating precisely: the automatic `recover_pending_to_db()` reader is gateway-only (`gateway/run.py:26505`), so in the common case writer and reader are the same service. What makes this more than cosmetic is `flush_agent_history_to_file`, whose payloads are deliberately skipped by automatic recovery and left for a human — "so an operator can salvage the conversation after repairing state.db". A 0700 dir owned by the service uid locks that operator out of their own transcript. Fix shape follows NousResearch#77579 (`2d766ac55`): branch on `is_managed()` at the creation site, not just at reconciliation. The house precedent is `ensure_hermes_home`, which already branches on `is_managed()` at its own creation site (`hermes_cli/config.py:896`) and whose managed branch creates `logs/curator` with a bare `mkdir(parents=True, exist_ok=True)`. The hand-rolled `os.chmod(flush_dir, 0o700)` is replaced by a delegation to `hermes_cli.config._secure_dir`, the single place that implements this policy for HERMES_HOME and every subdirectory `ensure_hermes_home` creates. The hand-rolled version diverged from it on three axes: it ran in managed mode, it ignored `HERMES_HOME_MODE`, and it was unguarded — and `chmod` by a non-owner raises EPERM, which propagated out of `_get_flush_dir()` into callers that all wrap this module in `except Exception: pass`, degrading the shutdown flush to a silent no-op. 18 `gateway/` modules already import from `hermes_cli.config`, so the dependency direction is established. Not using NousResearch#77655's `secure_mkdir`: it does not exist on `main` (NousResearch#77655 is separately open), and importing it would couple two open PRs. Non-managed behavior for the mode itself is unchanged at 0700 under both umask 022 and umask 077. `HERMES_HOME_MODE=0701` now applies (0o701 rather than 0o700), a deliberate consequence of delegating: every sibling HERMES_HOME subdir honours that hatch, the payload files remain 0600, and `uuid4().hex` filenames are not guessable from an execute-only directory. 6 -> 16 tests. Teeth proven per mechanism by reverting each individually and restoring byte-identically (sha256 a25a1c80..bbd77 re-confirmed after each): managed-branch mkdir mode=0o700 restored -> 2 failures unmanaged mkdir `mode=` stripped -> 1 failure (isolation test only) managed early-return deleted -> 3 failures reconciler try/except removed -> 1 failure The mkdir mode and the chmod DO mask each other, as expected: stripping `mode=` from the unmanaged mkdir left both plain "is it 0700?" tests green, because `_secure_dir` alone produced 0700. Only the isolation test that stubs the reconciler to a recording no-op caught it. `atomic_json_write(..., mode=0o600)` in `_write_payload` is left exactly as is. Measured: a fresh file is 0o600 with or without the argument under umask 022/077/007, because `mkstemp` creates at 0600 and `_restore_file_mode` no-ops with no prior mode. The argument only changes the overwrite path, which `uuid4().hex` filenames make unreachable. It does close a real fchmod-before-rename window for the overwrite case, so removing it would be churn; adding it anywhere else would be a no-op. Verified: ruff 0.16.1 clean on both changed files, scripts/check-windows-footguns.py --diff upstream/main clean, scripts/check_subprocess_stdin.py clean. Regression set (77 files matching shutdown_flush|pending_messages) 558 passed / 1 failed, the failure being the pre-existing SSRF-guard baseline in test_telegram_thread_fallback.py, confirmed by reproducing it with this file reverted to upstream.
|
Answering the three
The label is the right instinct on this file, but the direction is inverted. The loss mechanism on
Measured on the real
Payloads stay owner-only in both. The verbatim user content in them is not more exposed by this change; the directory is what gates whether the other process can write at all.
Two things worth flagging rather than leaving implicit:
This branch merges cleanly into current |
What does this PR do?
gateway/shutdown_flush.py::_get_flush_dir()creates$HERMES_HOME/pending_messageswith an unconditionalmode=0o700and chmods it0700again:On a managed (NixOS) install, both mechanisms override group permissions the module deliberately configures. This is pre-existing upstream code, not a regression from a recent PR.
The NixOS evidence (
nix/nixosModules.nixon currentmain):stateDir,.hermes,cron,sessions,logs,memories,pluginsat2770(setgid, group-rwx) viasystemd.tmpfiles, mirrored by the activation loop at 740-743.pending_messagesis absent from both lists — so on a managed host it is always created lazily at runtime, and these two lines are the only thing setting its mode.UMask = "0007", commented "files created by the gateway should be group-writable so interactive users in the hermes group can read/write them."chown -Rspecifically because it strips setgid, "destroying the 2770 permissions … for group access by hostUsers."container.hostUsersget a~/.hermessymlink to that samestateDir, so the gateway service and an interactive CLI share one$HERMES_HOMEat two different uids.Measured under real managed conditions (2770 parent,
umask 0007, dir absent):0o700(no group)0o6000o770(group-rwx)0o600The payload files stay
0o600either way — only the directory widens, which is the part that gates the second process. Measured consequence:mkdir(exist_ok=True)on a dir owned by the other uid succeeds silently, then themkstempinside it failsEACCES. So the flush is lost, not merely unreadable, and because every caller wraps this module inexcept Exception: passit fails silently.Blast radius, stated honestly
The automatic reader
recover_pending_to_db()is called from exactly one place,gateway/run.py:26505— gateway-only. In the common case writer and reader are the same service, so this is narrower than a user-facing directory.What makes it more than cosmetic:
flush_agent_history_to_filepayloads are deliberately skipped by automatic recovery (if payload.get("reason") == "shutdown-with-unpersisted-agent-history": continue) and documented as left for a human — "so an operator can salvage the conversation after repairing state.db." A0700dir owned by the service uid locks that operator out of their own transcript. Plus, on managed hosts the two uids sharing$HERMES_HOMEare both gateway processes, so theEACCESwrite-loss path is reachable without any CLI involvement.Related Issue
Refs #77472 — the
pending_messages/item in cluster R-DUMP ("flush files raw unredacted; 0600 skipped on Windows"). This PR addresses the directory-mode half of that item on managed installs; the payload files were already0600onmainand stay0600here. The sibling file-mode items in that cluster are #77520 (trajectories / MoA traces //save) and #77655 (a2a logs, batch trajectories,sessions/saved); its unbounded-growth item is #78395 and its redactor-correctness item is #78379.Note on that issue's framing: the report characterises this path as an at-rest exposure. It is not one on POSIX — the payloads are already
0o600and the directory0o700onmain. The real defect runs the opposite direction, which is what this PR fixes:0o700is forced even on a managed install, revoking the group accessnix/nixosModules.nixdeliberately configures (UMask = "0007"atnixosModules.nix:907,~/.hermesat0o2770setgid).No dedicated issue for this specific defect. Related context: #72680 (the incident that introduced this module), and #14181 tracks the same managed/shared-runtime permission class for
SKILL.md. Sibling PR #77579 (head4195f1e98) is precedent for the fix shape, not a parent — the two touch disjoint files and can land in either order.fix(gateway): resolve session_key in shutdown-flush recovery, open, headb1db79117) — shares two files with this PR. The production hunks are disjoint: fix(gateway): resolve session_key in shutdown-flush recovery #75536 editsrecover_pending_to_db(from line 168) while this edits_get_flush_dir(lines 36-46), sogateway/shutdown_flush.pyshould merge cleanly.tests/gateway/test_shutdown_flush.pywill conflict textually — both changes are pure appends at the same anchor (line 132 of a 134-line file), fix(gateway): resolve session_key in shutdown-flush recovery #75536 adding 110 lines and this one 293. Semantically compatible; the conflict is positional only. Happy to rebase behind whichever lands first.Type of Change
Changes Made
gateway/shutdown_flush.py_get_flush_dir()branches onis_managed()at the creation site, mirroringensure_hermes_home(hermes_cli/config.py:896) and itslogs/curatorbare-mkdir precedent. Managed installs getmkdir(parents=True, exist_ok=True)so the inherited setgid +UMask=0007lands2770._managed_install()helper — local import, failure means "not managed" (an unimportable config module is the single-user source install, where owner-only is right).os.chmod(flush_dir, 0o700)is replaced by_reconcile_flush_dir_mode(), which delegates tohermes_cli.config._secure_dir— the single owner of this policy for HERMES_HOME and every subdirensure_hermes_homecreates.tests/gateway/test_shutdown_flush.py— 5 → 16 tests, three new classes. (Baseline measured at merge basecb11a7e25: 5 test functions, noparametrize; currentupstream/mainalso has 5.)Why delegate the chmod rather than guard it inline
The hand-rolled chmod diverged from the shared policy on three axes, each independently a defect:
HERMES_HOME_MODE, which every sibling dir honours;chmodby a non-owner raisesEPERM, which propagated straight out of_get_flush_dir()into callers that all swallow exceptions, silently degrading the flush to a no-op.17
gateway/modules already import fromhermes_cli.config(counted onupstream/main), so the dependency direction is established, not new.Not using #77655's
secure_mkdir: it does not exist onmain(#77655 is separately open), so importing it would couple two open PRs. Following that author's own precedent, this branch stands alone.HERMES_HOME_MODEnow applies (0o701instead of0o700) as a deliberate consequence of delegating. #77655 argued the hatch exists for traversal to a served subdir and nothing is served here. I went the other way for this site:pending_messagesis a peer ofsessions/andlogs/, which all honour it, the payload files stay0600, anduuid4().hexfilenames aren't guessable from an execute-only directory. Happy to gate it if a maintainer prefers consistency with #77655 instead.Relationship to #74897
#74897 moved
write_filenew files from0600to umask-derived0644. Reading it, the real story is that_atomic_write's chmod branch only ran when the target already existed, so new files silently keptmktemp's0600— an accident, not a policy — and #74897 restored umask-derived permissions for a path the user named with a documented interop contract (#70856: Obsidian LiveSync, Docker volumes, NAS mounts). This change moves in the same direction for managed mode: removing a hardcoded mode in favour of the configured umask.How to Test
Managed-mode fresh creation (the case at issue):
16 passed.
TestFlushDirManagedPermissionsreproduces all three real conditions —HERMES_MANAGED=nixos, parent chmod'd2770,umask 0007, dir absent.Teeth, per mechanism (each reverted individually, restored byte-identically, sha256 re-confirmed):
mkdir(..., mode=0o700)restoredmode=strippedtry/exceptremovedThe mkdir mode and the chmod do mask each other, as suspected. Stripping
mode=from the unmanaged mkdir left both plain "is it 0700?" tests green, because_secure_diralone produced0700. Onlytest_creation_mode_alone_is_owner_only, which stubs the reconciler to a recording no-op, caught it. That's why the isolation fixture exists rather than a second final-mode assertion.Full octal matrix, dir and payload, before/after, measured under a temp
HERMES_HOME:umask 0220o7000o7000o6000o600umask 0770o7000o7000o6000o600umask 0007, parent2770, dir absent0o7000o7700o6000o60007500o7000o7500o6000o600HERMES_HOME_MODE=0701,umask 0220o7000o7010o6000o600On
atomic_json_write(..., mode=0o600)in_write_payload— left untouchedMeasured, so it doesn't get "fixed" needlessly: a fresh file is
0o600with or without the argument under umask022/077/007, becausemkstempcreates at0600and_restore_file_modeno-ops with no prior mode. The argument only changes the overwrite path, whichuuid4().hexfilenames make unreachable. It does close a real fchmod-before-rename window for the overwrite case, so removing it would be churn. No change made.Checklist
Code
pytest tests/ -qand all tests pass — not run. I ran the canonical./scripts/run_tests.shon the changed file (16/16) plus the 77-file regression set matchingshutdown_flush|pending_messages: 558 passed, 1 failed. The one failure istest_telegram_thread_fallback.py::test_send_image_upload_fallback_blocks_connect_time_rebind, a pre-existing SSRF-guard baseline failure — confirmed baseline by reproducing it withgateway/shutdown_flush.pyreverted toupstream/main.test_shutdown_forensics.py(Linux-only) is likewise baseline-failing in this worktree, verified the same way.Documentation & Housekeeping
_get_flush_dir)HERMES_MANAGED/HERMES_HOME_MODE; no newHERMES_*env varos.name != "posix"). Windows ACL enforcement is deliberately left to fix(security): enforce owner-only ACLs on Windows in _secure_file #77527 rather than duplicated here. macOS verified directly; setgid inheritance confirmed on Darwin (2770parent →0770child underumask 0007)Screenshots / Logs