Skip to content

Restrict the UDS mirror sockets directory to its owner and publish mirror metadata atomically - #3019

Open
kriszyp wants to merge 14 commits into
mainfrom
fix/uds-mirror-dir-hardening
Open

kriszyp wants to merge 14 commits into
mainfrom
fix/uds-mirror-dir-hardening

Conversation

@kriszyp

@kriszyp kriszyp commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

Restricts the TLS unix-domain-socket mirrors' sockets directory to its owner (mode 0700) at every mirror creator, publishes each mirror's metadata .yaml by atomic rename, and stops an isolated application's worker from reporting ready without a bound mirror.

⊙ Problem

The TLS unix-domain-socket mirrors skip TLS and trust the PROXY v2 identity a fronting proxy passes, so the sockets directory (<hdbBase>/sockets) is their only access gate. It is created with the default mode, 0755 under a typical umask, so other local users can traverse it. Separately, writeUdsMetadata rewrote the mirror's .yaml in place, so a concurrent reader or an overlapping worker could observe a truncated file.

Background: fix: make UDS mirror cleanup ownership-aware, not path-based (#2035) deferred the atomic write, and Keep per-worker UDS mirror sockets alive across an overlapping HTTP worker restart (#2964) covered restart survival.

❓ Your call: Is owner-only access warranted on every install? I judged yes: the mirror's trust comes from an identity the proxy asserts, and the directory is the portable boundary. On Linux, connecting also needs write permission on the socket file (unix(7)), so 0755 alone does not show that another user can connect. The alternative, trusting the parent directory's mode, is rejected below.

💡 Solution

After upgrade, an existing <hdbBase>/sockets directory is tightened to 0700 on the first secure-port start.

❓ Your call: Forcing 0700 removes group and other traversal. A fronting proxy that runs as a different non-root UID, or relies on a group-shared directory, stops reaching the mirrors after upgrade. Shared workers then serve without mirrors and log the error. Fabric is unaffected on a source-level read: Central Manager's host provisioning runs the host-manager service as root, and host-manager spawns symphony with no UID or GID override, so symphony runs as root too. This was not checked on a live host. Self-hosted operators need a release-note line. Restoring group access needs a config option; this PR adds none.

❓ Your call: Symlinks and foreign ownership are not checked. mkdir, stat and chmod follow a sockets symlink, and the owner is not compared to the Harper UID. I did not add an lstat owner check: an operator can legitimately symlink sockets to another volume, and planting either case needs write access to the Harper root, which already holds the config and keys.

⚠️ Look hardest: the readiness refusal in startServers. It is the only thing that stops an isolated worker from reporting ready with no ingress, and it counts bound mirrors through hasUdsMirror.

⚖️ Alternatives

Framing-Verdict: better-alternative-exists (53cba34eac9d)

❓ Your call: the planning review returned better-alternative-exists, and the implementation adopted its alternative in full rather than overruling it. The review found three gaps in the original plan: it enforced the directory only at the native HTTP creator, though Bun and raw TLS create the directory on their own; it hand-rolled a temp-file-and-rename writer although atomicWriteFile already exists; and propagating an enforcement error would leave the server cached and registered without its handlers. The shipped design is the one the review proposed: one helper called at all three creators, atomicWriteFile(…, { maxRetries: 0 }), and log-and-skip the mirror while the TLS listener stays up. The design was not re-submitted for a clearing verdict after adoption.

  • Set process.umask(0o077) at boot: rejected. The umask is process-wide, so it changes the mode of every file Harper creates, including databases and logs, and it cannot tighten an existing directory.
  • Move the mirrors to a per-user runtime directory (XDG_RUNTIME_DIR): rejected. Fronting proxies discover mirrors at <hdbBase>/sockets, so this changes that contract.
  • Rely on the parent directory's mode: rejected. mountHdb creates a fresh root with HDB_FILE_PERMISSIONS (0700), but it does not repair an existing or widened root, so the invariant would still fail on upgraded installs.
  • Hand-roll a temp file and rename in writeUdsMetadata: rejected in favor of the existing atomicWriteFile in config/configUtils.ts, which names a unique sibling, renames, and removes the temp file on either failure. One atomic-write implementation, not two.
  • Keep the default rename retries: rejected. atomicWriteFile retries with Atomics.wait on the calling thread, and maxRetries: 0 keeps a serving worker from blocking during a certificate reload.

🔧 Changes

✅ Verification

  • npm run test:unit:main at 1d0366f: 6447 passing, 202 pending, 1 failing. The failure is nonInteractiveSpawn git credential scoping in unitTests/components/gitCredentials.test.js. It inherits the dispatch worker's GIT_* environment, and with those variables unset the file passes 19/19.
  • npm run test:unit:resources at 1d0366f: 3951 passing, 54 pending, 0 failing.
  • npm run test:integration:all at 1d0366f: 2296 passing, 0 failing, 18 skipped, 6 cancelled. All 6 cancellations are in the OllamaBackend suite, which needs Ollama models this machine does not have.
  • npx mocha unitTests/server/udsMirrorDirectory.test.js at 4bdde9a: 10 passing. The suite proves that creation yields 0700, that a rename replaces the yaml (the inode changes), that an open descriptor keeps the previous complete generation, and that a non-empty directory at the yaml path does not throw or leave a temp file. The same file also covers tightening a pre-created 0755 directory, restoring an owner-unusable 0500 directory, idempotence, a regular file occupying the path, and a failed temp write keeping the previous yaml (non-root POSIX).
  • npm run test:integration -- integrationTests/server/uds-mirror-overlapping-restart.test.ts and integrationTests/components/isolated-application.test.ts: 8/8 passing with the readiness check in place. The overlapping-restart test asserts 0700 after startup; it would fail on base, where the default mkdir mode is 0755.
  • npm run lint:required, prettier --check on the changed files, and npm run check:design-docs: pass.

Not verified:

  • The Bun and raw TLS mirror creators are not run by any test here; this environment has no Bun.
  • The isolated-worker refusal is not exercised end to end. A sockets path occupied by a regular file would show it.
  • A proxy running under another UID, and a proxy's behavior when the yaml is replaced by rename on certificate reload: no reader of this file exists in this repo. A source-level read of host-manager shows it watches the sockets directory itself (so a rename keeps firing events), re-reads each -<port>.yaml by path after a 500 ms debounce, and its -<port>.yaml filename filter skips the .tmp sibling. Not exercised live.

🤖 Generated by Claude Sonnet 5 (Claude Code); posted via @kriszyp.

Related PRs: #372 independent (same files, different concern), #562 independent (adds no non-erasable TypeScript syntax), #2532 independent, #2763 independent, #2946 independent, #2956 overlaps (same http.ts area; merged cleanly through this rebase, no conflict), #2981 independent (its admission flow runs before this gate in threadServer.js; no duplication), #3029 independent (no overlapping behavior), #3035 independent (merged; its ownership gates are preserved, this PR is additional), #3069 independent (different DESIGN.md section)
Complexity: complicated

Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=14; full=4 @ b7f9179

Review-Attention: study ~9m (hot: http.ts, threadServer.js; decisions: exact-0700, pool-degrade-vs-fail, gate-placement, any-vs-all-mirrors) @ b7f9179

@kriszyp kriszyp added this to the v5.3 milestone Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Release cherry-pick v5.3: conflict

Cherry-pick onto v5.3 produced conflicts on commit(s): 396ab5ffcf7f78578407e9c2054e31a50d0440f5 7b26d4c87cce258c6751000dfcd4d98f62f9f564 5970700ab02021422d756102d2cf3a715022ceae cd6eff0e64b401bc0385967634ecd19683f9b638

The conflict markers are committed on branch cherry-pick/v5.3/pr-3019.
Integration tests are not running until conflicts are resolved.

@claude please review branch cherry-pick/v5.3/pr-3019 and suggest a patch that resolves the conflict markers (<<<<<<< / ======= / >>>>>>>) introduced by cherry-picking PR #3019 onto v5.3. Post the suggested patch as a comment on this PR — do not push.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances the security of Unix Domain Socket (UDS) mirrors by ensuring the sockets directory is restricted to owner-only access (0o700) across all worker environments. It introduces the ensureSocketsDirectory helper to enforce these permissions, updates metadata writing to be atomic, and adds comprehensive integration and unit tests. The review feedback correctly identifies a style guide violation in the new unit test file, recommending that Node built-in modules use the node: prefix for imports.

Comment thread unitTests/server/udsMirrorDirectory.test.js Outdated
kriszyp and others added 12 commits October 6, 2026 19:27
The TLS UDS mirrors skip TLS and trust the PROXY v2 identity a fronting
proxy passes, so the sockets directory is their only access gate. It was
created with the default mode (0755 under a typical umask). Every creator
now forces it to 0700 before binding (http.ts, threadServer.js Bun and raw
TLS paths), tightening a pre-existing directory with a logged warning, and
skips that mirror when the directory cannot be secured.

writeUdsMetadata publishes the sibling yaml through atomicWriteFile, so a
reader never sees a truncated file while overlapping workers or cert
reloads rewrite it.

Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
…rkers loudly

ensureSocketsDirectory now computes the sockets path and returns it, or
undefined when a shared worker cannot secure it. The path join no longer
runs for servers without a UDS mirror, and the three creators share one
guard. An isolated application's worker is served only through its
mirror, so its failure throws instead of starting without a listener.

Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
An isolated worker's directory failure was thrown after the server was
cached, so later registrations reused a listener with no mirror and the
worker still reported started. The check now runs before the cache
assignment (http.ts) and before any registration (raw TLS onSocket), so a
failure leaves nothing registered and the component load reports it.

Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
A mirror that cannot be secured is skipped and logged. An isolated
application is reachable only through its mirror, so its worker now fails
startup when the worker registered none, instead of reporting ready with
no ingress. The directory helper no longer throws, which leaves server
registration unchanged.

Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
A mirror whose bind failed stays registered, so the readiness check must
skip paths in failedUdsPaths or an isolated worker reports ready with
nothing listening.

Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
Other suites in the same mocha process re-point the base path, so a path
captured at module load names a directory the helper never touches.

Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
…tory suite

Dispatch-Task: harper-uds-mirror-dir-hardening
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J7YjJTe8TxQEVdys3cRbrB
@kriszyp
kriszyp force-pushed the fix/uds-mirror-dir-hardening branch from 4877612 to 3a4111a Compare October 7, 2026 01:35
kriszyp and others added 2 commits October 6, 2026 19:51
Review found two related gaps: afterEach still ran after a failed beforeEach
containment assert (SOCKETS_DIR was assigned before the assert), and the
assert itself used startsWith, which treats a sibling directory like
<root>-evil as contained. Both could let test cleanup rm -rf a directory
outside the sandboxed test root. isWithinTestRoot() resolves and checks a
real path boundary, and SOCKETS_DIR is only assigned after it passes.

Dispatch-Task: pr-maint-6991910019c44bbcd3ab780c19e52cd5
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review found both wrote the rationale for a reviewer rather than the
invariant for the next reader, and one overstated the failed-assertion
case (the previous test's validated target stays set, not nothing).

Dispatch-Task: pr-maint-6991910019c44bbcd3ab780c19e52cd5
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant