Skip to content

security: harden proxy and jail isolation - #105

Merged
ammario merged 3 commits into
mainfrom
security/audit-fixes
Sep 24, 2026
Merged

ammario merged 3 commits into
mainfrom
security/audit-fixes

Conversation

@ammar-agent

@ammar-agent ammar-agent commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Close policy-bypass, privileged-file, and jail-escape paths found during the security audit. This is behavior-changing hardening, not just a refactor. HTTP/TLS bodies remain streamed; protocol/header deadlines do not become whole-connection deadlines.

Risks / compatibility decisions to review first

  • Linux workload compatibility: strong mode now requires sudo from a non-root user. Private PID/proc namespaces, dropped groups, no_new_privs, and seccomp restrictions can break host Unix-socket clients/agents and some IPC workloads. Fixed system helper paths must exist (ip, nft, setpriv, unshare; docker for Docker mode).
  • Policy compatibility: structured results without an explicit allow now deny. Root-run --sh/--proc, including dry-run evaluation, require trusted root-owned executable paths and run unprivileged with a clean environment; inline shell commands are rejected. Command-mode JS files are snapshotted rather than hot-reloaded; server-mode reload remains.
  • Docker compatibility: only the local daemon at /var/run/docker.sock is used. Remote contexts are ignored; privileged/network/DNS/mount/port-publishing and other unrecognized pre-image flags are rejected. NET_RAW is dropped. Please review the allowlist against real workloads.
  • CA migration: privileged Linux credentials move to /var/lib/httpjail/ca. Replace trust in the old user-home CA. Recovery preserves a valid signing key, but a lost/corrupt key necessarily creates a new CA and requires renewed client trust (httpjail trust --install on macOS).
  • Operational changes: HTTP/1 headers must arrive within 10 seconds; evaluator output is capped at 64 KiB. Root request logs must belong to the invoking user in an owned directory. Logs still contain full URLs/query strings. Legacy resources without reliable canaries are retained for manual inspection, not blindly deleted.

Suggested review order

Area Start here Invariant to check
1. Policy and streaming limits src/rules/common.rs, src/rules/{proc,shell}.rs, src/limited_body.rs, src/proxy{,_tls}.rs Fail closed; bound reads/output; account for headers and trailers; preserve streaming and upgraded connections.
2. Privilege and process lifetime src/main.rs, src/jail/{mod,managed,weak}.rs, src/jail/linux/{mod,seccomp}.rs Stop/reap the command group before cleanup; prevent payload access to host process descriptors and privileged evaluators.
3. Network enforcement and cleanup src/jail/linux/{docker,nftables,resources}.rs Guard bridge INPUT/FORWARD and native IPv6; remove Docker network before its firewall guard; do not mistake live resources for orphans.
4. Credential/file safety src/tls.rs, log creation in src/main.rs Reject unsafe file paths; keep keys private; atomically publish and recover incomplete CA pairs without rotating valid keys.
5. Evidence and user guidance tests/{linux_integration,security_regressions,js_file_reload}.rs, inline unit tests, README/docs Regressions exercise the stated guarantees and documentation calls out breaking behavior.

Codex follow-up and simplification

Follow-up commits: b8b4f49 (review fixes and scope reduction), 6b9e736 (test stability). Net 108 fewer lines, including the added regressions.

  • CA recovery: one shared load/generate path, initialization lock, same-directory atomic replacement, key-first publication, and tests for missing/empty/truncated keys plus missing/corrupt/mismatched certificates. Healthy cached credentials stay byte-for-byte stable. macOS trust installation now goes through recovery first.
  • Quiet processor failures: timeout/restart diagnostics use debug, not warn; a regression checks default stderr remains empty for the silent stalled processor. The processor's own stderr is still inherited.
  • Less code to audit: removed redundant Docker IPv4 forwarding rules (the inet guard remains), unified normal/orphan cleanup, and dropped the unrelated global legacy-table sweeper. No new dependencies.
  • Corrected the CONNECT regression to accept either EOF or TCP reset, and serialized only the two tests sharing fixed proxy ports.

Validation

All six GitHub CI checks passed for 6b9e736 (Linux/macOS Clippy, format, unused dependencies, Linux tests, macOS integrations).

  • macOS: cargo fmt --check, cargo clippy --all-targets -- -D warnings, full cargo test (including weak-mode and security regressions).
  • Linux, isolated CI-1 checkout: formatting, all-target Clippy, 62 library tests, 33 privileged integration tests (3 helper entry points ignored as standalone tests), and both weak-mode byte-limit tests.
  • Earlier audit smoke checks: native HTTPS returned 200 with certificate verification enabled; Docker still used the local daemon despite a poisoned remote context.

Boundaries / evidence limits

  • Native strong mode is not filesystem isolation and does not close explicitly inherited descriptors. Policies and their parents must remain outside payload write access. Weak/macOS mode remains environment-only, not a hostile-code containment boundary.
  • Timeout supervision covers the command process group; deliberately detached descendants or daemon-managed containers need additional supervision. max_tx_bytes limits logical request bytes, not exact on-wire framing overhead.
  • The Linux IPv6 negative test and installed rule were checked, but the positive control could not reach the host veth even after removing the drop table. That environment does not independently prove the table's blocking effect.
  • CA recovery checks parseability and key pairing, not arbitrary same-key certificate identity/constraint changes. Interactive macOS keychain authorization is not exercised by automated tests.
  • Separate dependency risk: GitHub reports 12 default-branch dependency alerts, including 5 high. This PR does not update dependencies or claim to resolve those alerts.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-24T14:38:39.125197Z 22b0ef9 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22b0ef9324

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/tls.rs Outdated
Comment thread src/rules/proc.rs Outdated
@ammario
ammario merged commit 89b436e into main Sep 24, 2026
6 checks passed
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.

2 participants