Repository navigation
feat(docker): add pinned egress planning seam - #131
Merged
senamakel merged 190 commits intoOct 23, 2026
Merged
Conversation
Expose an egress module that resolves and pins allowlist targets so sandbox network access can be planned against stable addresses. The new types and plan_egress_allowlist are re-exported from the crate root for callers. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Tiny Sweeper reviewFindingsNo active actionable findings. Advisory notes
Verdict: Incomplete · Priority: medium Diagnostics and supporting detailsFull summaryPull request purpose (title): feat(docker): add pinned egress planning seam. Tiny Sweeper completed its review; deterministic results follow. No latest-push delta is available. Unreviewed paths
Partial review reasons
Review snapshot
Completeness: Incomplete Features
Tests
Before merge
Agent review detailscritique
security
tests
commits
description
Evidence and run details
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f0dda8d37
ℹ️ 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".
Hostname normalization no longer strips the trailing root dot, so absolute names such as "service." stay distinct from their relative counterparts instead of collapsing into the same allowlist entry. Tests were extended to cover the absolute-name case and to disambiguate Result from the crate's Error type. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds tests asserting that IP literal allowlist entries are pinned without resolution and that forbidden literals are rejected, plus a test that resolver failures surface as backend errors. The existing hostname test now also checks that both configured ports are retained. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add assertions for the execution_location accessor on the local and SSH hosts, and on the SSH host's inner host, so the new reporting behaviour is exercised for local, remote, and unknown cases. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Introduce a HostExecutionLocation enum and an execution_location method on the Host trait so security-sensitive controllers can determine whether a host runs commands locally or remotely instead of inferring it from the host's registration name. Custom hosts default to Unknown until they explicitly attest to their locality. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Implement execution_location on LocalHost so callers can tell that commands run on the local machine rather than a remote host. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
SshHost now implements execution_location and returns Remote, so callers can tell that commands run on the far machine rather than locally. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add tests that verify the egress controller prepares the allowlist before the container starts and cleans up after destroy, and that a controller failure rolls back the box record before it is dropped. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Register the new egress_enforcement module and re-export DockerEgressController and NativeLinuxDockerEgressController so callers can enforce egress policy through the native Linux backend. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The Docker sandbox now accepts an optional egress controller that can rewrite docker argv before execution, letting callers install per-box allowlist filtering. Without a controller the sandbox keeps refusing NetworkPolicy::Allowlist as before. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The remove_container helper now passes its argv straight to run_docker instead of wrapping it with docker_argv, since the caller already supplies the full command. This removes a redundant layer of argument construction with no change in behaviour. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a validate_host method to the DockerEgressController trait with a default no-op implementation, and have the native Linux controller check that the host is supported, the target is Linux, and the engine and firewall are available. Also skip forcing the local rootful docker env prefix when the command already starts with env. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Wrap the host in a ControllerHost that rewrites docker argv through the egress controller before execution, so observed hosts and sandboxes with a controller attached get the same egress handling as the primary host. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The docker sandbox now installs an egress filter through the egress controller when a box requests an allowlist network policy, instead of refusing the policy outright. The controller validates the host and prepares the default-drop filter before the container starts, and any failure during create tears the egress state back down so no filter is left behind. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformat the egress enforcement and sandbox modules to satisfy rustfmt, wrapping long argument lists and error strings and collapsing short ones. No behaviour changes. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The firewall output helper now converts the lossy stdout to an owned value before returning it, so the result no longer borrows from the command output. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Failed container creates now run a shared rollback path that tears down egress rules and records the attempt for later cleanup when that teardown itself fails, instead of silently discarding the failure record. Egress enforcement also installs allowlist rules only on the forward chain and drops all host-origin traffic on the input chain, so allowlisted destinations no longer apply to traffic originating from the host. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
…identity Add a test asserting that the native Linux egress controller refuses to create a sandbox when the host has no explicit local identity, returning ConstraintNotEnforced for the network constraint without issuing any host commands. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Update the test resolver to return std::io::Result so it matches the EgressResolver trait signature. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Extend the live Docker test imports with BoxId, EgressAllowlist, and the egress controller types so the suite can exercise network policy enforcement against a real daemon. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a live test that runs two listeners in a test-owned bridge container and checks the native firewall controller permits the allowlisted port while blocking other raw TCP, using a fixed resolver so no public service or Docker host path is involved. The test tears down the sandbox and listener container before asserting so failures do not leak resources. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a unit test asserting that a failed egress cleanup keeps the box record so a later cleanup retry can find it, and extend the live allowlist test to prove that direct raw TCP to the host gateway is refused while the pinned destination still connects. The CI workflow now runs the new live test. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
…cases The single atomic-chain test was replaced by two focused tests: one asserting that cleanup reclaims an empty attempt chain after a restart, and one asserting that cleanup refuses a nonempty unmarked chain and issues no delete, flush, or chain-removal commands. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
…reates The ownership check used by ambiguous network create reconciliation now also compares the attempt label, so a network left behind by an earlier create attempt for the same box is no longer mistaken for this attempt's network. Inspection and verification both parse the extra label and treat a mismatch as a foreign network to leave alone. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a Linux-only test asserting that when a network create fails after its side effect, cleanup inspects the attempt label and leaves a network belonging to an older attempt untouched instead of removing it. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The attempt-label assertion now spells out the full label string instead of relying on the shared constant, and two prepared native hosts are bound as mutable so the tests can adjust their state. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Two test bindings had the wrong mutability, so the host is now declared mutable where its fields are assigned and immutable where they are not. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Two test bindings had the wrong mutability, so the first now declares the prepared host as mutable and the second no longer does. This keeps the tests compiling without changing what they exercise. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The README now documents that each attempt gets a fresh random token stored on the Docker network and folded into chain names, and that chains are created exclusively before receiving their owner marker. It also explains the reclaim rules for unmarked chains after a restart and that ambiguous orphans fail closed. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The fake host now records which chains have rules added and reports missing rules for -C checks, so the egress enforcement tests can exercise the rule verification path. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test host now resolves the iptables or ip6tables binary once per invocation and reuses it for the marked-chain and chains-with-rules bookkeeping, instead of re-deriving it inline at each call site. A chains_with_rules removal was also added to the -D path so deletions stay consistent with insertions. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test host now clones the firewall identifier for the marked-chains removal and reuses the original value for the chains-with-rules removal, instead of re-deriving it from argv. This keeps both removals keyed on the same firewall value. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformatted the egress enforcement module and its tests to satisfy rustfmt, wrapping long call chains and assertions and collapsing short conditions onto single lines. No behaviour changed. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The firewall binary lookup was computed twice in a row, so the first binding was dropped and the second reused. The remaining binding is now cloned at the insertion site to keep the value available. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The cleanup test now also records the forward and input chains as having rules and carrying the owner marker, so the refusal path is exercised against chains that look genuinely owned by another instance. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Rename the test to make clear it covers rollback of both the created chain and the network when the owner marker install fails. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a Linux-only test asserting that cleanup succeeds and issues only the two expected chain-existence probes when the box network is already absent and no matching hooks remain, without running any delete, flush, or remove commands. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The cleanup test asserted exactly two `-S` invocations, which is brittle against extra read-only listing calls. It now checks that at least two occur, keeping the intent while tolerating additional chain inspections. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a test that makes the owned network removal fail after an ambiguous create, verifying the cleanup error is retained and the network is inspected twice. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a fault injection hook to the prepared native host so a single `docker network rm` invocation can be made to fail, letting tests exercise the cleanup path when network removal errors out. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The ambiguous create rollback test now binds the prepare result and includes it along with the recorded commands in the assertion message, so a failure shows what the controller actually returned instead of only the mismatch. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The ambiguous create test now asserts on the simulated lost command response message instead of the owned egress network rollback failure, so the assertion matches the error the test actually injects. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The test now compiles the network-removal assertion only on Linux, since the behaviour it exercises is platform-specific and broke builds elsewhere. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
The no_matching_hooks field and its initializer are now behind a cfg(target_os = "linux") attribute, matching the other Linux-only fields in PreparedNativeHost. This avoids an unused-field warning on other platforms. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
feat(docker): enforce pinned Linux egress allowlists
tinysweeper
Bot
dismissed
their stale review
October 23, 2026 18:47
tinysweeper could not review the latest push, so its earlier approval no longer speaks for this pull request.
tinysweeper
Bot
added
priority: p2
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9a5a70aa1
ℹ️ 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 on lines
+1188
to
+1192
| "env".into(), | ||
| "-u".into(), | ||
| "DOCKER_HOST".into(), | ||
| "-u".into(), | ||
| "DOCKER_CONTEXT".into(), |
There was a problem hiding this comment.
When a sandbox configured with this controller receives DOCKER_HOST, DOCKER_CONTEXT, DOCKER_TLS_VERIFY, or DOCKER_CERT_PATH in BoxSpec.env or ExecRequest.env, this wrapper silently removes the requested workload value. In the inspected sandbox/args.rs, both run and add_exec_context emit name-only --env KEY flags, while their callers supply values through the CLI process environment. Docker documents that if such a variable is absent, “the variable is unset in the container.” (Docker CLI documentation) Consequently, installing the controller breaks workloads that need these variables, including exec-time overrides. Separate daemon-connection sanitization from workload environment transport so the local socket remains pinned without dropping explicitly requested container variables.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a reusable Docker crate planner that resolves each distinct normalized hostname once and returns immutable host/port/IP targets for a future proxy or firewall. The planner rejects the whole plan if any answer is a loopback or cloud metadata address, including IPv4-mapped metadata answers. Safe private addresses remain allowed under the current policy contract.
Fake resolver tests prove one lookup per normalized hostname even when a resolver would return loopback on a later call, retain the first pinned answer, reject mixed safe/forbidden answers, and permit private addresses.
This is planning only: it does not install firewall rules, filter traffic, or enable
NetworkPolicy::AllowlistinDockerSandbox::create, which continues to refuse allowlists as unsupported. It does not claim allowlist enforcement or complete DNS-rebinding protection.Stacked on TinyBox PR #130 (
tinybox-27-metadata-ip-alias).Refs #27
Part of tinyhumansai/openhuman#7328