Repository navigation
fix(spec): reject metadata IPv4 aliases - #130
Merged
senamakel merged 195 commits intoOct 23, 2026
Merged
Conversation
Extend the malformed-host cases to include trailing-dot forms of the metadata hostnames, ensuring the allowlist validator rejects them as well. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
Metadata host matching now strips terminal root dots before comparing, so names like `Metadata.Google.Internal.` are refused instead of slipping through the allowlist check. The contract spec was updated to document the case-insensitive, root-dot-tolerant matching. Auto-committed-on: cuttlefish Co-authored-by: Medulla <medulla@tinyhumans.ai>
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): fix(spec): reject metadata IPv4 aliases. The whole-pull-request diff ships far more than its title and body describe. It adds an opt-in Docker egress enforcement stack: a resolver-backed planning module (`plan_egress_allowlist`, `PinnedEgressPlan`, `EgressResolver`), a `DockerEgressController` lifecycle trait with a `NativeLinuxDockerEgressController` that verifies a local rootful Linux Engine, `iptables`/`ip6tables` `DOCKER-USER`/`INPUT` chains and bridge visibility, resolves and pins destinations, rejects loopback/metadata/IPv6-link-local and in-subnet targets, and installs default-drop filters before the workload starts. `DockerSandbox` gains `with_egress_controller`, a `ControllerHost` wrapper that pins every Docker command to the rootful socket, and rollback/cleanup bookkeeping retained for retry when egress cleanup fails. Core gains a `HostExecutionLocation` capability (`Local`/`Remote`/`Unknown`, defaulting to `Unknown`) that LocalHost, LimitedLocalHost, JailLocalHost and SshHost implement. Args generation gains pinned `--add-host` entries and a loopback-only `--dns`. The metadata-IP-alias change itself is a small hardening in `EgressAllowlist::is_metadata_host` with terminal-root-dot trimming and a legacy-numeric-IPv4 parse check. The description lane (Answered) reports the PR body still only describes the metadata alias change, so the description must be rewritten. 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
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>
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>
…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
feat(docker): add pinned egress planning seam
tinysweeper
Bot
dismissed
their stale review
October 23, 2026 18:56
tinysweeper could not review the latest push, so its earlier approval no longer speaks for this pull request.
tinysweeper
Bot
added
priority: p2
![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: b49713019f
ℹ️ 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".
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.
Reject legacy numeric IPv4 spellings that normalize to the known cloud metadata addresses, using the existing
parse_legacy_ipv4helper. Regression coverage includes octal and hexadecimal aliases.This PR is stacked on TinyBox PR #127 (
tinybox-27-loopback-policy) and includes PR #128's terminal DNS root-dot normalization because both changes touchis_metadata_host. Its diff against #127 therefore includes both #128's root-dot normalization and the metadata IP alias change. No DNS lookup or DNS-answer filtering is added.Refs #27
Part of tinyhumansai/openhuman#7328