Skip to content

harden(ssh): scope bulk sync tar extraction to .hermes - #10205

Closed
Stark-X wants to merge 1 commit into
NousResearch:mainfrom
Stark-X:fix/ssh-bulk-upload-home-permissions
Closed

harden(ssh): scope bulk sync tar extraction to .hermes#10205
Stark-X wants to merge 1 commit into
NousResearch:mainfrom
Stark-X:fix/ssh-bulk-upload-home-permissions

Conversation

@Stark-X

@Stark-X Stark-X commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix a regression in SSH bulk file sync where tar extraction was performed at /, allowing archive path components like home/<user>/... to touch the remote home directory. This can break SSH access after a seemingly successful one-shot command.

The user-visible failure mode is:

  • the agent uses the SSH backend for a one-shot command
  • that command succeeds
  • the bulk sync step widens the remote home directory permissions to something like 775
  • the next SSH attempt fails because OpenSSH StrictModes rejects the home directory permissions

This change keeps SSH bulk upload scoped to ~/.hermes:

  • stage files relative to the remote .hermes base instead of the absolute remote path
  • reject any remote path that falls outside the sync base
  • extract the tar stream into ~/.hermes instead of /
  • add regression coverage to ensure the staging tree never includes home/<user> path components

Root Cause

The bulk uploader mirrored absolute remote paths in the temporary staging directory and then extracted the archive with tar xf - -C /. That meant the archive could contain parent directories such as home/<user>, and tar would apply those directory entries while unpacking.

If the local staging directories are created under a permissive umask, the archive can carry directory modes like 775 for home/ and home/<user>/. When those entries are extracted at / on the remote host, the existing remote home directory can be widened as a side effect, causing later SSH authentication failures.

Why This May Reproduce Only In Some Environments

This regression depends on both the buggy extraction behavior and the surrounding local/remote permission model.

In the reported environment:

  • the local shell umask was 002
  • a minimal reproduction showed staging directories being created as 775
  • the remote SSH server used OpenSSH StrictModes yes

That combination makes the bug much easier to reproduce:

  • local umask=002 means staging directories under home/<user>/... default to group-writable modes like 775
  • extracting the archive at / can then apply those modes to the real remote home directory
  • OpenSSH with StrictModes yes checks the user's home directory and files before accepting login, so the next SSH attempt may be rejected immediately

This means the bug is still in the code regardless of environment, but hosts with umask=002 locally and StrictModes=yes remotely are much more likely to see it as a hard SSH failure rather than a silent permission drift.

Testing

  • source venv/bin/activate && uv run pytest tests/tools/test_ssh_bulk_upload.py tests/tools/test_ssh_environment.py -q

@Stark-X Stark-X changed the title fix(ssh): keep bulk sync extraction scoped to .hermes fix(ssh): prevent bulk sync from breaking later SSH logins Apr 15, 2026
@Stark-X
Stark-X marked this pull request as ready for review April 15, 2026 09:54
@Stark-X
Stark-X force-pushed the fix/ssh-bulk-upload-home-permissions branch from b240162 to 08aa37f Compare April 15, 2026 10:13
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround backend/ssh SSH remote execution backend/file-sync File sync across remote backends labels Apr 22, 2026
@Stark-X
Stark-X force-pushed the fix/ssh-bulk-upload-home-permissions branch from 46fc5a0 to b302f3c Compare May 11, 2026 08:45
@Stark-X Stark-X changed the title fix(ssh): prevent bulk sync from breaking later SSH logins harden(ssh): scope bulk sync tar extraction to .hermes May 11, 2026
@Stark-X

Stark-X commented May 11, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up context after rebasing this PR onto latest main:

The original #17767 failure mode has already been addressed upstream by #17898, which added --no-overwrite-dir to the remote tar extract command. That protects existing remote directory metadata from being clobbered and closes the main StrictModes lockout bug.

This PR is now intended as a narrower security hardening follow-up rather than the primary #17767 fix. Current main still builds the local tar staging tree from absolute remote paths via remote_path.lstrip("/") and still extracts the archive at /, so the archive can contain path components such as home/<user>/.hermes/.... With #17898 this should no longer overwrite existing directory modes, but it still leaves the bulk sync operation broader than necessary.

The change here scopes the tar stream to the remote Hermes directory itself:

So the remaining value is defense-in-depth: avoid archiving or extracting parent home-directory path components at all, and constrain bulk upload to the directory Hermes owns.

@kevhardy

Copy link
Copy Markdown

I am seeing this issue in my logs and this PR looks like the solution.

WARNING tools.environments.file_sync: file_sync: sync failed, rolled back state:
tar extract over SSH failed (rc=2):
tar: .: Cannot change mode to rwxr-xr-x: Operation not permitted
tar: ./home: Cannot change mode to rwxr-xr-x: Operation not permitted
tar: Exiting with failure status due to previous errors

@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #30172 — your commit was cherry-picked onto current main with authorship preserved (commit eb51fb6). Thanks @Stark-X! Both the .hermes-scoped extraction and the path-escape rejection are now on main.

@Stark-X
Stark-X deleted the fix/ssh-bulk-upload-home-permissions branch May 25, 2026 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/file-sync File sync across remote backends backend/ssh SSH remote execution P1 High — major feature broken, no workaround type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants