feat(security): add sanitized remote worker boundary - #93182
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 5d196c054cad0b84c958dab733a0a8d48e405cf9 against exact current main/base 2eaa863112d2980bbe6f15ea409a6a29e50964fe. The v2 publication is now a clean 2-commit child of current main and cleanly supersedes the stacked #93181 from the same author. I found two security-boundary blockers.
-
The remote preflight does not actually admit a manifest-bound pack.
build_context_pack()/verify_context_pack()implement the right manifest, inventory, hash, clean-worktree, and binding machinery, buthermes secure-worker auditaccepts only--packandaudit_profile_boundary()merely comparesterminal.cwdto that resolved path. It never accepts a manifest/policy, never callsverify_context_pack(), and never checks.git/hermes-secure-worker-pack.json. The clearest witness is the PR's owntest_rendered_ox_profile_passes_when_every_boundary_is_present: it does onlypack.mkdir()— no Git repo, manifest, binding marker, or sanitized inventory — and assertsallowed is True. Soprofile-render --cwd <arbitrary-dir>followed byaudit --pack <same-dir>can authorize an arbitrary directory, including a production checkout, while the advertised contract says remote inference only receives a manifest-bound sanitized workspace.Required fix: make remote audit take the manifest + policy and verify the exact pack/binding/inventory before ALLOW. Add negative witnesses for a bare directory, production checkout, tampered pack, extra file, wrong manifest, and wrong policy. Ideally the resulting admission receipt also carries the manifest digest/source commit that was actually authorized.
-
The tool-capability audit checks
toolsets, but CLI runtime authority isplatform_toolsets. The generated remote profile hastoolsets: []andplatform_toolsets.cli: [terminal, file].audit_profile_boundary()only rejectswebin the top-leveltoolsetslist; it never validatesplatform_toolsets. Currenthermes_cli.tools_config._get_platform_tools()readsconfig["platform_toolsets"][platform]and directly enables listed configurable toolsets. That means addingweb,browser,computer_use, etc. underplatform_toolsets.clican leave this preflight green while exposing host-network/host-side capabilities outside the offline Docker task-container boundary. The existing adversarial mutation test only appendswebto the non-authoritative top-level list, so it cannot catch the live escape.Required fix: audit the effective CLI toolset through the canonical resolver (or a shared pure projection) and require the exact admitted capability set —
terminal,file, plus only the single secure staging MCP path. Addplatform_toolsets.climutation tests, including a composite toolset such ashermes-clithat expands broader capabilities.
Architecture / interlocks:
- #93182 is the clean superseding publication for same-author #93181; preserve that lineage and retire the stacked object once this is the canonical delivery.
- #91414 is complementary guarded-local-prompt work from the same contributor, not a duplicate of this Docker/data boundary.
- #90050 owns the canonical container-config projection across terminal/file/code/prompt-probe creators. Since this PR introduces
docker_isolate_host_data, compose with that single authority rather than leaving another hand-copied security setting. Merge order in either direction needs to preserve the field across every container creator.
I did not find another blocker in the staging GitHub broker's fixed owner/repo binding, redirect policy, token narrowing, Docker host-data reuse partitioning, or the explicit fallback/data-collection checks.
Exact-head CI, Docker, and Nix are currently action_required; the author's focused 317-pass + Colima smoke is useful local evidence, but there is not yet a canonical executed GitHub CI receipt on this SHA.
5d196c0 to
e40f981
Compare
|
Addressed both review blockers and rebased this PR onto current Changes:
Verification with the canonical runner:
Latest commit: |
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking re-review of exact head e40f981cb7fa0a0c116c2c0dae46309c62fd9992 against its submitted base 7e67f64fcee9340f40a1c6f912fc650aa4984510, including the author’s response in #issuecomment-5391912855.
The two findings from my prior review are materially addressed at this head:
- remote audit now requires the pack, manifest, and policy, calls
verify_context_pack()plus_assert_manifest_binding(), and carries the verified manifest digest/source commit inBoundaryReport; - CLI authority is now checked through the canonical
_get_platform_tools()resolver, with directplatform_toolsets.cliand composite-toolset mutation witnesses.
That closes those two exact manifestations. This remains blocking because adversarial traversal of the completed boundary exposes three deeper authority breaks.
1. The trusted verifier executes and trusts model-owned Git control metadata
build_context_pack() creates .git inside the disposable pack. The whole pack is then mounted writable at /workspace so the worker can use Git. After the untrusted worker has run, verify_proposed_diff() invokes host-side Git against that same repository through _status_records() and:
git diff --binary HEAD --The verifier therefore treats the model-writable .git/config, index, refs, object database, attributes, and hook-adjacent Git configuration as trusted authority.
I reproduced both sides of the break in a clean disposable repository using the same verifier commands:
# Host-code execution during verifier-style git diff
git config diff.external /tmp/payload
git diff --binary HEAD --
# /tmp/payload executed
# Evidence suppression
git update-index --assume-unchanged file.txt
printf 'hidden change\n' > file.txt
git status --porcelain=v1 --untracked-files=all # empty
git diff --binary HEAD -- # emptyObserved output was:
external_diff_executed=yes
status_bytes=0
diff_bytes=0
So the remote worker can make the trusted host verifier execute an arbitrary program under the operator account, and it can alter an approved file while producing an empty changed_paths set and empty diff digest. The external manifest marker does not prevent either attack because both arise after binding, when Git itself is asked to interpret model-controlled metadata.
Required closure: never execute Git using a model-writable Git directory/index/config. Keep the admitted baseline and Git control plane outside the writable pack, or reconstruct a fresh trusted temporary repository from an external immutable baseline and compare current bytes there. The verifier must derive inventory and changed content independently of the worker’s .git; it should also run with system/global/local Git configuration disabled and no external diff/textconv/hook/fsmonitor execution. Add adversarial witnesses for diff.external, core.fsmonitor, .gitattributes/textconv, rewritten refs/HEAD/index, assume-unchanged, skip-worktree, and a symlinked/replaced .git.
2. staging_file_put can turn the typed broker into arbitrary GitHub Actions execution and egress
_repo_path() rejects traversal and .git, but accepts repository control-plane paths such as:
.github/workflows/remote-worker.yml
.github/actions/payload/action.yml
put_file() can write those paths directly to any supplied branch, including main. verify_repository_boundary() verifies repository identity/visibility and coarse pull/push/admin role flags; it does not establish that the token lacks Workflows: write, and it does not verify that Actions is disabled or that the staging repository has no executable workflow surface. The broker also accepts GitHub App installation tokens (ghs_), whose app installation may carry workflow authority.
GitHub explicitly supports modifying .github/workflows through this contents endpoint when the token has the corresponding workflow permission: https://docs.github.com/en/rest/repos/contents#create-or-update-file-contents
That means the remote model can create a workflow and trigger it with a second content write, obtaining arbitrary networked runner execution under the user’s GitHub identity despite the task container being offline. Even a minimally scoped token is not sufficient closure if an existing workflow in the staging repository executes modified repository content on push.
Required closure: make repository control-plane policy explicit and enforced by the broker, not an operator convention. At minimum deny .github/workflows/**, .github/actions/**, and other executable/control paths; constrain writes to a dedicated proposal namespace/branch; and verify that Actions is disabled for the staging repository (or refuse file-write capability when that cannot be proven). Add tests showing these paths are rejected before the HTTP client is reached and that an existing-workflow repository cannot pass admission.
3. Admission is not bound to the exact runtime identities it claims to authorize
The new audit verifies more fields, but two direct substitutions still pass the current logic:
config["model"]["base_url"] = "https://attacker.invalid/v1"
config["mcp_servers"]["secure-github-staging"]["command"] = "/tmp/evil"For the first mutation, _is_remote_provider() classifies the profile as remote solely because provider == "nous"; audit_profile_boundary() never requires the canonical Nous endpoint. The privacy attestation still matches the provider string while inference and the sanitized pack are redirected elsewhere.
For the second mutation, the MCP check requires only an absolute command path plus the expected -m hermes_cli.secure_github_broker arguments. An arbitrary executable therefore passes preflight and receives HERMES_STAGING_GITHUB_TOKEN in its environment.
The broader root cause is that cmd_audit() validates a handwritten projection of a raw YAML file, prints ALLOW, and exits. Although BoundaryReport now contains manifest_sha256 and source_commit, the CLI discards them; no immutable admission receipt is emitted or consumed by the process that actually launches Hermes. The audited file, merged defaults, environment-derived values, executable at the broker path, and runtime tool/config expansion can all diverge after preflight.
The policy proof is likewise version-only: PackManifest records policy_version, not a canonical policy digest. The new “wrong policy” test changes the version, but a policy with the same version and different excluded_segments/budgets is accepted. Audit also does not re-enforce the builder’s invariant that the manifest lives outside the model-writable pack; a manifest under .git plus a rewritten marker would make the proof model-controlled again.
Required closure: admit and launch one canonical effective runtime snapshot. Pin the exact model endpoint and broker executable/module identity, hash the complete resolved security configuration and full canonical policy, emit a receipt containing those identities plus manifest/image/source commitments, and have a secure-worker run gate consume that receipt atomically before constructing tools/processes. Re-verify the executable and all files at use time. Enforce that manifest/policy/receipt live outside the writable pack. Add direct mutation tests for endpoint substitution, arbitrary/symlinked broker commands, same-version policy drift, manifest-inside-pack, environment/default override drift, and mutation between audit and launch.
Repository state / interlocks
#93182remains the clean canonical publication and correctly supersedes the closed same-author#93181. Preserve that lineage.#91414remains complementary guarded-local-prompt work, not duplicate ownership.#90050still owns canonical container-config projection. The final secure-worker path should consume that authority rather than accumulating another partial runtime projection.- Current
mainis nowe3f695e5e00ef8718d8829fbe44fd3d2e36ed236, 120 commits ahead of this PR’s base, and GitHub currently reports the PR non-mergeable.cli.py,gateway/run.py,hermes_cli/main.py, andtools/terminal_tool.pyall changed in that interval, so the repair needs a fresh rebase and exact-head regression pass. - Exact-head CI, Docker, and Nix are all
action_requiredwith no executed job evidence. The reported focused run is useful local evidence, but it is not a canonical exact-head repository receipt.
The host-data mount suppression, container-reuse partition, fixed repository routing, redirect denial, credential narrowing, and the two newly repaired admission checks remain sound within the inspected diff. The blockers above are all at trust-boundary crossings: model-owned metadata becoming verifier authority, a typed repository capability expanding into a remote execution plane, and preflight identity not being the identity actually executed.
e40f981 to
40c67b0
Compare
|
Addressed the blocking re-review and rebased onto current Security changes:
Verification at
The only pytest warning was the sandbox denying |
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review complete at exact head 40c67b07fe93c2b983ebf0e17cfc1686a328254d.
The three security-boundary blockers from my prior review are materially closed on this head.
-
Trusted verification no longer invokes Git against the model-writable pack.
verify_proposed_diff()derives changes from the external manifest baseline and direct filesystem bytes, so model-controlled.gitmetadata cannot execute host commands or suppress modified-file evidence. The adversarial tests coverdiff.external,core.fsmonitor, attributes, altered HEAD/index state, assume-unchanged, and replaced/symlinked.gitwhile still detecting the changed approved file. -
The staging broker now constrains mutation authority to
hermes-proposal/*, rejects.github/workflows/**,.github/actions/**, and Dependabot control paths before HTTP, and requires repository Actions to be disabled during broker preflight. The tests exercise each of those negative boundaries and the proposal-branch constraint. -
Admission is now bound to the identities actually launched: exact Nous endpoint, exact non-symlink
sys.executable, broker module hash, canonical config/policy/manifest/source/image commitments, and an external immutable admission receipt.secure-worker runre-audits immediately before launch, verifies the receipt, isolatesHERMES_HOME, rejects provider/model/profile/toolset overrides, and the regression suite demonstrates audit-to-launch mutation failure. The manifest now binds the full canonical policy digest and pack root, and manifest/policy/config/receipt placement inside the writable pack fails closed.
I also checked the current targeted regressions in test_secure_worker.py, test_secure_worker_cli.py, and test_secure_github_broker.py; they cross the previously vulnerable authority boundaries rather than only testing helpers.
The remaining gate is repository execution authority, not an identified product defect: exact-head CI 32723101152, Docker 32723100164, and Nix 32723100407 are all action_required and expose no executed hosted jobs. Current upstream main has also advanced to 057dcdf236f8a6a26721c10fcc6ccb72726e272a since this head's rebase, although GitHub currently reports the PR mergeable. A fresh current-main composition plus executable exact-head hosted matrix is still required before merge.
Within the inspected security boundary, I do not see a remaining manifestation of my prior three blockers.
GitHub rejected an APPROVE submission from this connected account because explicit repository review approval permission is unavailable, so this COMMENT is the strongest exact-head review state I can publish through the current authority.
40c67b0 to
2a0c95a
Compare
|
Rebased cleanly onto current upstream main 74ad422; new exact head is 2a0c95aa0c. Re-ran the four focused boundary suites: 160 tests passed, 0 failed. The canonical wrapper then exited nonzero only while trying to write test_durations.json under this restricted worktree; the pytest subprocesses themselves all passed. git diff --check is clean and the rebased branch is pushed. |
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head re-review of 2a0c95aa0c29bbaa713e44a08b103ba213428081, including the author’s rebase and focused-test receipt in #issuecomment-5399074122.
Result: I found no new code blocker. The rebase preserves the previously reviewed security closure rather than introducing a materially different implementation.
Rebase verification
- The four rewritten commits retain the same subjects and per-commit change totals as the previously reviewed series. Across the corresponding commits I inspected, the secure-worker patches and resulting file blobs are unchanged; the commit IDs changed because the parent chain moved.
- The base movement from
ddbd928ee4e881f0c7b3536a00355647c6559fe2to74ad422d50e6d76e26ba5ce3b4d2a4d520e923ddoverlapped this PR only inhermes_cli/config_defaults.pyandhermes_cli/main.py. The secure-worker additions in both files remain semantically unchanged; only later line offsets moved inmain.py. - GitHub currently reports the PR mergeable. Current upstream
mainiscd297653fa4fac85f45f7d3ad8e361db0f14e9be, eight commits beyond the submitted base. That post-base delta touches none of this PR’s 24 changed paths, so it introduces no composition conflict in the current snapshot.
Security-boundary disposition
The three blockers from my earlier review remain closed at this exact head:
- The trusted verifier is independent of model-writable Git metadata. Proposal verification derives changes from the external manifest baseline and direct filesystem bytes rather than executing Git against the worker-controlled repository, with adversarial coverage for external diff/fsmonitor/attributes, rewritten Git state, index suppression flags, and replaced
.gitstate. - The typed GitHub broker cannot expand into a GitHub Actions execution plane. Mutation is restricted to
hermes-proposal/*; workflow, local-action, and Dependabot control paths are denied before HTTP; and broker admission requires Actions to be disabled in the staging repository. - Admission is bound to what is actually launched. The gate pins the Nous endpoint, broker executable and module, resolved config, full policy, manifest/source, and worker image into an external receipt;
secure-worker runre-audits immediately before construction and rejects audit-to-launch drift.
I also see no regression in host-data mount suppression, isolation-aware container reuse, fixed repository routing, redirect denial, credential narrowing, or the exact effective-toolset check.
Execution evidence
The author reports 160 focused tests passed, 0 failed, plus clean git diff --check. The canonical wrapper’s nonzero exit occurred only when it attempted to write test_durations.json in the restricted worktree; the underlying pytest subprocesses passed. I treat that as useful exact-head local evidence, not as a hosted repository receipt.
Exact-head CI, Docker, and Nix remain action_required and expose zero executed jobs. That is now the only merge gate I can identify: repository execution authority must authorize and complete the exact-head hosted matrix. It is not an identified product-code failure.
Interlocks
- #93182 remains the canonical same-author replacement for closed, unmerged #93181.
- #91414 remains complementary guarded-local-prompt work, not duplicate ownership.
- #90050 remains the canonical-container-projection interlock. Its eventual composition must preserve
docker_isolate_host_dataacross terminal, file, code-execution, and prompt-probe creators; I do not see a present blocker from it on this head.
Disposition: approval-style review; no code changes requested. Authorize/rerun the three exact-head hosted workflows and merge if that matrix is green. This connected account cannot submit an APPROVE review in this repository, so COMMENT is the strongest review state available.
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head re-review of merge head 7320983abd88ca05cb706f6582114b400413ccdd after origin/main was composed into the previously reviewed 2a0c95aa0c29bbaa713e44a08b103ba213428081 security boundary.
I re-checked the conflict-resolution surface rather than transferring the earlier verdict blindly. The merge conflicts were limited to cli.py, tests/tools/test_docker_environment.py, and tools/environments/docker.py. The resolution preserves the reviewed secure-worker host-data isolation contract while integrating current-main's shared-container identity work: DockerEnvironment still carries isolate_host_data, still suppresses credential/skills/cache mounts and the egress proxy under that mode, and the newly integrated reuse identity keeps the existing hermes-host-data isolated/ambient label in the container identity path. cli.py likewise retains docker_isolate_host_data while current main adds the separate docker_shared_container_key configuration. I do not see a conflict-resolution regression in the previously reviewed pack/verifier/launch-authority closure.
GitHub reports this head open and mergeable. Hosted execution authority has not run, however: CI 32841867402, Docker 32841866492, and Nix 32841866999 all ended action_required before jobs were created. I also attempted to re-run the exact-head CI through GitHub; the integration was denied with 403 Resource not accessible by integration. Therefore there is no new code blocker from this merge re-review, but I am not transferring green CI from 2a0c95... onto this new object. Exact-head workflow authorization/execution is the remaining publication requirement.
|
Exact-head verification of merge head 7320983abd88ca05cb706f6582114b400413ccdd in a clean worktree at that SHA confirms the re-review findings: DockerEnvironment retains isolate_host_data with credential/skills/cache mount suppression under isolation, while the integrated shared-container identity path keeps the hermes-host-data isolated/ambient label (tools/environments/docker.py:1414-1419); cli.py retains docker_isolate_host_data alongside the separate docker_shared_container_key configuration. A runtime import check of the merged modules passes. No code changes were made at this head. Agreeing that exact-head hosted workflow execution (CI/Docker/Nix currently action_required) remains the outstanding publication requirement. |
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking exact-head update for 7320983abd88ca05cb706f6582114b400413ccdd.
The previously reviewed secure-worker boundary remains materially intact on this head. I found one deterministic repository-gate failure that must be repaired before the hosted matrix is authorized.
Blocker: every product commit uses an unmapped local author email
The four non-merge commits in this PR are:
a53b6fbbaab617208976fe66d910cfb77bafa391b18e4f55306f5f4ee52e12e6e612bb0fb956cfa6ec76223e01711306a900ed51abadcd5c80c9ac2a2a0c95aa0c29bbaa713e44a08b103ba213428081
All four use mikedemott@Mikes-Mac-mini.local. This exact head has no contributors/emails/mikedemott@Mikes-Mac-mini.local, and the repository contains no legacy mapping for that email.
At this head, scripts/audit_pr_attribution.py scans git log $(git merge-base origin/main HEAD)..HEAD --format=%ae --no-merges; every email that is not a bot/known skip or an ID-based GitHub noreply address must resolve through contributors/emails/<email> or the frozen legacy map. mikedemott@Mikes-Mac-mini.local satisfies none of the skip rules, so the script will report it as unmapped and exit 1. The hosted contributor check will therefore fail as soon as jobs are allowed to execute.
Required closure, preserving the existing commit lineage rather than rewriting it:
python3 scripts/add_contributor.py \
'mikedemott@Mikes-Mac-mini.local' \
mrkillbob \
'PR #93182 secure-worker commits'
python3 scripts/audit_pr_attribution.pyCommit the generated mapping as a forward commit and include the green audit output in the PR.
Current-main composition and execution
Current main is 1fe0f2f3ac9748ce799272eb93bee2937b5ab802. GitHub now reports the graph diverged: main has advanced 208 commits from the common merge base while this PR carries its five unique commits. Post-head main changes include hermes_cli/main.py, one of this PR’s integration paths. The author’s August 26 clean-worktree confirmation proves 7320983...; it does not prove the new current-main composition.
After adding the mapping, compose onto current main, rerun the focused secure-worker/broker/Docker suites, and obtain executable exact-head CI, Docker, and Nix receipts. Runs 32841867402, 32841866492, and 32841866999 remain action_required with no jobs executed, so no earlier green evidence transfers to the replacement head.
I found no new manifestation of the previously closed verifier, GitHub-Actions escape, or audit-to-launch identity blockers. This review is narrowly blocking on the certain attribution failure plus fresh current-main/exact-object proof.
|
Addressed the current attribution blocker without rewriting the published commits or force-pushing. Exact head: Changes and composition:
Fresh exact-head evidence:
GitHub currently reports no checks/runs on this replacement head, so executable exact-head CI, Docker, and Nix receipts still require repository authorization/execution before publication. |
94b3f50 to
43a0a44
Compare
…ct-repair-20260827 # Conflicts: # contributors/emails/mikedemott@Mikes-Mac-mini.local
|
Merge conflict resolved: composed current Focused verification at resolved head The contributor-attribution mapping was already present at the prior head, so that blocker should already be clear. Remaining gate per the last review is repository execution authority (CI/Docker/Nix still |
# Conflicts: # cli.py # gateway/run.py # hermes_cli/config.py # hermes_cli/config_defaults.py # hermes_cli/main.py # tests/tools/test_docker_environment.py # tools/code_execution_tool.py # tools/environments/docker.py # tools/file_tools.py # tools/terminal_tool.py
|
Hermes automated repair (pr-repair-steward) Merged the pinned base 245e480 and preserved the secure-worker integration. The initial conflict resolution restored obsolete CLI parser code and caused a NameError in the real command entrypoint; correction a07e523 keeps the current parser and registers secure-worker through it. Updated the stale MCP test import without weakening credential-isolation assertions. Validation: 177 focused tests passed across secure worker, secure worker CLI, staging broker, Docker environment, and terminal configuration. The CLI regression executes the real secure-worker help command. Codex published the preserved merge and correction through the verified fork as mrkillbob with a normal fast-forward push. This is focused repair evidence, not full local-CI or merge approval. |
|
@codex review |
What does this PR do?
Adds a fail-closed secure-worker workflow for repository-capable local and remote models. It creates manifest-bound sanitized workspaces, requires an offline digest-pinned Docker task container, and exposes GitHub operations only through a typed broker bound to one private staging repository.
This preserves useful coding, branching, issue, pull-request, and review workflows without giving a remote model the production checkout, ambient host credentials, arbitrary GitHub targets, or general task-container egress.
Related Issue
No linked issue.
Type of Change
Changes Made
hermes secure-workercommands for pack creation, boundary audit, profile rendering, privacy attestation, diff verification, and safe destruction.How to Test
pytest -q -p no:cacheprovider tests/hermes_cli/test_secure_worker.py tests/hermes_cli/test_secure_worker_cli.py tests/hermes_cli/test_secure_github_broker.py tests/tools/test_docker_environment.py tests/tools/test_docker_network_config.py tests/tools/test_docker_cgroup_limits.py tests/tools/test_docker_session_isolation.py tests/tools/test_credential_files.py tests/tools/test_mcp_tool.py tests/tools/test_terminal_config_env_sync.py tests/hermes_cli/test_startup_fast_guards.py.examples/secure-worker/Dockerfile, record its immutable image ID, and render a profile withhermes secure-worker profile-render.hermes secure-worker auditwith the image lock, sanitized pack, and short-lived privacy attestation; verify it denies Docker/network/credential/fallback mutations.Focused verification: 317 passed. A live Colima smoke also confirmed non-root execution, absent ambient credentials and host auth paths, blocked task-container networking, and usable Git/Python tooling.
The repository-wide suite was not claimed green: an earlier full run reached an unrelated order-dependent failure that passes alone.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Not applicable; this is a CLI and security-boundary change.