feat(sessions,minimald,minvmd): static ingress, session policy state, VM-egress DM2 rejection - #556
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSession policy types move into ChangesSession networking policy enforcement and ingress port mapping
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/minimald/src/session_host.rs (1)
624-636: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake ingress teardown awaited, not fire-and-forget.
Dropspawns unexpose + detach and immediately returns, so callers that awaited the host task can proceed while external ports are still exposed; runtime shutdown can also cancel the cleanup. Move this into an explicit async teardown path that the host/session shutdown awaits, keepingDroponly as a last-resort fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/session_host.rs` around lines 624 - 636, The ingress teardown and switch detach operations are currently fire-and-forget within a spawned task, causing callers to proceed while cleanup is still pending and allowing runtime shutdown to potentially cancel the cleanup. Extract the async cleanup logic (the removal of ingress forwards via remove_ingress and the switch.lock().await.detach() call) into a separate dedicated async teardown method that can be explicitly awaited by callers during session/host shutdown. Keep the current Drop implementation pattern only as a last-resort fallback that spawns without awaiting, ensuring the critical cleanup path is properly synchronized and awaited by code that controls the session lifecycle.crates/minimald/src/net/mod.rs (1)
60-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBroaden the
SubnetExhaustedmessage now that it covers invalid prefixes.Line 154 uses
SubnetExhaustedfor prefixes below/8, where the subnet is rejected for MAC-collision safety rather than because no PTask address remains. The current Display text will mislead configuration failures.Suggested wording
- #[error("gvproxy subnet {0} is exhausted; no free PTask address remains")] + #[error("gvproxy subnet {0} cannot provide a safe PTask address")]Also applies to: 139-155
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/net/mod.rs` around lines 60 - 63, The error message for the SubnetExhausted variant in the NetError enum is too specific and misleading. The SubnetExhausted error is currently used for two different scenarios: when a subnet is actually exhausted of addresses and when a prefix is invalid (below /8) for MAC-collision safety. Update the #[error(...)] attribute text for the SubnetExhausted(SwitchSubnet) variant to use a more generic message that covers both cases, removing the specific mention of "no free PTask address remains" to avoid misleading configuration failures related to invalid prefixes.
🧹 Nitpick comments (2)
crates/minimald/src/session.rs (1)
256-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep policy validation active in test launches.
The new R2.1 gate is compiled only in the production launcher; tests using
MockLaunchercan still attach policies that production rejects. Mirror the validation in the#[cfg(test)]launcher so test behavior stays aligned.Suggested patch
#[cfg(test)] fn session_launcher( &mut self, _session: SessionHandle, ) -> Result<session_host::MockLauncher, AttachError> { + self.session + .record() + .validate_policy() + .map_err(AttachError::InvalidPolicy)?; Ok(session_host::MockLauncher) }Also applies to: 280-284
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/session.rs` around lines 256 - 271, The policy validation gate (R2.1) using record.validate_policy() is only active in the production SandboxLauncher but not in the test MockLauncher, causing tests to accept policies that production would reject. Mirror the validation logic from the production launcher into the test MockLauncher by calling the same record.validate_policy() method and returning AttachError::InvalidPolicy on failure, ensuring test behavior remains aligned with production behavior.crates/sessions/src/store.rs (1)
441-441: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise a non-default policy in the persistence fixture.
Line 441 seeds only
SessionPolicy::default(), so the loader tests still do not prove that a configured policy survives a disk round-trip. SinceRecord.policyis now the live source forGetSessionPolicy, make this fixture or the round-trip test use a non-defaultOwnIppolicy and assert it aftercreate/get.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sessions/src/store.rs` at line 441, The persistence fixture at line 441 only tests with the default SessionPolicy, which doesn't verify that non-default policies survive a disk round-trip. Update the fixture to use a non-default OwnIp policy instead of SessionPolicy::default(), then modify the corresponding round-trip test (the create/get test) to assert that the configured non-default policy is correctly retrieved after persistence, ensuring that Record.policy acts as the authoritative source for GetSessionPolicy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/net/policy.rs`:
- Around line 165-172: Add a timeout constraint around the stream operations in
the gvproxy control request handling block to prevent indefinite waits when
gvproxy stalls or fails to close the connection. Wrap the sequence of operations
starting with stream.write_all() through stream.read_to_end() with
tokio::time::timeout() to bound the maximum duration allowed for the write and
read operations. This ensures that if gvproxy becomes unresponsive, the request
does not hang indefinitely and allows teardown to proceed without leaving PTask
instances attached to the system.
- Around line 57-65: The protocol validation is missing from the
`Record::validate_policy()` method, allowing unsupported protocols like ICMP to
be sent to gvproxy. Add a validation check in `Record::validate_policy()` that
ensures the protocol field of ingress mappings is either TCP or UDP, rejecting
the policy with an appropriate error message if the protocol is ICMP or any
other unsupported type. This validation should occur before the protocol is
passed to the `protocol_str()` function.
In `@crates/minimald/tests/netns.rs`:
- Around line 232-240: The Command::output() calls in the retry loop (at the
bash command execution and the similar call mentioned at line 253) lack
per-process timeouts, which allows the deadline to be bypassed if a process
hangs. Wrap both Command::output() calls with tokio::time::timeout() to enforce
a timeout on each individual command execution. Set an appropriate timeout
duration (shorter than the overall deadline of 10 seconds) so that even if a TCP
connection succeeds but the subsequent read stalls, the individual command will
timeout and the loop can continue to retry until the overall deadline is
reached.
In `@crates/minvmd/src/net.rs`:
- Around line 513-518: The stop() method unconditionally sends SIGTERM to
self.pid after the supervisor may have already reaped the child process,
creating a race condition where the PID could be reused by an unrelated process.
Instead of directly calling signal_child() on self.pid, you need to either route
the termination signal through the supervisor (which still owns the child and
can safely terminate it), or add a guard to verify the child process is still
owned before signaling. This same issue applies to the second location at lines
541-542 where SIGKILL is sent. Ensure that signal delivery happens only while
the supervisor retains ownership of the child, not after it has been reaped.
In `@crates/minvmd/src/vm.rs`:
- Around line 31-32: The Dm5 variant lacks proper validation for VM egress
policies because it does not encode its underlying deployment model, allowing
DM5-on-DM2 configurations to silently accept vm_egress even without a VM
boundary to enforce it. Either encode the underlying deployment model within the
Dm5 variant and validate it accordingly, or add validation logic around line 115
(in the egress policy validation section referenced in the "Also applies to"
comment) to explicitly reject Dm5 when vm_egress is set until the underlying
deployment model can be determined. Check the validation blocks at lines 114-120
and 306-320 to ensure consistent rejection of unresolved Dm5 configurations with
vm_egress.
---
Outside diff comments:
In `@crates/minimald/src/net/mod.rs`:
- Around line 60-63: The error message for the SubnetExhausted variant in the
NetError enum is too specific and misleading. The SubnetExhausted error is
currently used for two different scenarios: when a subnet is actually exhausted
of addresses and when a prefix is invalid (below /8) for MAC-collision safety.
Update the #[error(...)] attribute text for the SubnetExhausted(SwitchSubnet)
variant to use a more generic message that covers both cases, removing the
specific mention of "no free PTask address remains" to avoid misleading
configuration failures related to invalid prefixes.
In `@crates/minimald/src/session_host.rs`:
- Around line 624-636: The ingress teardown and switch detach operations are
currently fire-and-forget within a spawned task, causing callers to proceed
while cleanup is still pending and allowing runtime shutdown to potentially
cancel the cleanup. Extract the async cleanup logic (the removal of ingress
forwards via remove_ingress and the switch.lock().await.detach() call) into a
separate dedicated async teardown method that can be explicitly awaited by
callers during session/host shutdown. Keep the current Drop implementation
pattern only as a last-resort fallback that spawns without awaiting, ensuring
the critical cleanup path is properly synchronized and awaited by code that
controls the session lifecycle.
---
Nitpick comments:
In `@crates/minimald/src/session.rs`:
- Around line 256-271: The policy validation gate (R2.1) using
record.validate_policy() is only active in the production SandboxLauncher but
not in the test MockLauncher, causing tests to accept policies that production
would reject. Mirror the validation logic from the production launcher into the
test MockLauncher by calling the same record.validate_policy() method and
returning AttachError::InvalidPolicy on failure, ensuring test behavior remains
aligned with production behavior.
In `@crates/sessions/src/store.rs`:
- Line 441: The persistence fixture at line 441 only tests with the default
SessionPolicy, which doesn't verify that non-default policies survive a disk
round-trip. Update the fixture to use a non-default OwnIp policy instead of
SessionPolicy::default(), then modify the corresponding round-trip test (the
create/get test) to assert that the configured non-default policy is correctly
retrieved after persistence, ensuring that Record.policy acts as the
authoritative source for GetSessionPolicy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c9d33cd-2e62-4ab0-b699-2c3ee5b10c67
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
crates/minimal2/src/main.rscrates/minimald-rpc/src/lib.rscrates/minimald/src/exec.rscrates/minimald/src/net/mod.rscrates/minimald/src/net/policy.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rscrates/minimald/src/sftp.rscrates/minimald/tests/netns.rscrates/minvmd/Cargo.tomlcrates/minvmd/examples/exec.rscrates/minvmd/src/error.rscrates/minvmd/src/net.rscrates/minvmd/src/vm.rscrates/minvmd/tests/minimald_session_e2e.rscrates/sessions/src/lib.rscrates/sessions/src/store.rsdocs/specs/05-spec-minvmd-gvproxy-pidfd/05-spec-minvmd-gvproxy-pidfd.md
💤 Files with no reviewable changes (2)
- crates/minvmd/Cargo.toml
- docs/specs/05-spec-minvmd-gvproxy-pidfd/05-spec-minvmd-gvproxy-pidfd.md
|
Revise claim for head 91fb7b5. |
|
Auto-revise 1 of 3. |
|
Commit pushed:
|
Review feedback addressedPushed a follow-up commit resolving the actionable CodeRabbit and Fixed
Deferred (with reasons)
|
Resolve actionable CodeRabbit and sdd-review findings on PR #556: - net/mod.rs: restore async tokio::fs for the gvproxy switch config write and socket teardown, removing blocking std::fs from async context (CLAUDE.md async standards). - net/mod.rs: report an out-of-range subnet prefix as the new NetError::InvalidPrefix, distinct from runtime SubnetExhausted. - sessions: reject non-TCP/UDP ingress port mappings in Record::validate_policy (UnsupportedIngressProtocol) so an ICMP mapping cannot reach gvproxy's forwarder as a defaulted protocol. - net/policy.rs: bound each gvproxy control request with a 5s timeout so a stalled switch cannot hang launch or teardown. - session.rs: mirror the R2.1 policy gate into the test MockLauncher. - store.rs: seed the persistence fixture with a non-default OwnIp policy and assert it survives the disk round-trip. - minvmd/vm.rs: reject vm_egress on DM5 (fail closed) until its underlying deployment model is resolved. - netns.rs: bound the ignored CI ingress probes with `timeout` so a stalled connect cannot bypass the retry deadline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
|
Auto-resolve conflict 1 of 3. |
|
/revise resolve the merge conflict with main: merge main into this branch (or rebase onto it) and resolve the conflicts, then push to this PR branch. The conflict appeared after sibling PR #555 merged; most collisions are append-only unions in shared files (the crate manifest, the lib module / pub use list) — take the union of both sides. |
… VM-egress DM2 rejection
Resolve actionable CodeRabbit and sdd-review findings on PR #556: - net/mod.rs: restore async tokio::fs for the gvproxy switch config write and socket teardown, removing blocking std::fs from async context (CLAUDE.md async standards). - net/mod.rs: report an out-of-range subnet prefix as the new NetError::InvalidPrefix, distinct from runtime SubnetExhausted. - sessions: reject non-TCP/UDP ingress port mappings in Record::validate_policy (UnsupportedIngressProtocol) so an ICMP mapping cannot reach gvproxy's forwarder as a defaulted protocol. - net/policy.rs: bound each gvproxy control request with a 5s timeout so a stalled switch cannot hang launch or teardown. - session.rs: mirror the R2.1 policy gate into the test MockLauncher. - store.rs: seed the persistence fixture with a non-default OwnIp policy and assert it survives the disk round-trip. - minvmd/vm.rs: reject vm_egress on DM5 (fail closed) until its underlying deployment model is resolved. - netns.rs: bound the ignored CI ingress probes with `timeout` so a stalled connect cannot bypass the retry deadline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebase took main's net.rs (SwitchSubnetError thiserror derive) but this branch's stale minvmd/Cargo.toml predated it. Restore thiserror.workspace + refresh the lock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
7e4eff9 to
c2cb3e6
Compare
This comment has been minimized.
This comment has been minimized.
Address two sdd-review findings on the static-ingress policy: - MEDIUM (spec compliance): the networking spec's Security Considerations require minimald to refuse to configure gvproxy to publish a host port below 1024 and emit a clear error with a remediation suggestion. `Record::validate_policy` did not check this, so an `external_port: 80` mapping reached gvproxy and failed opaquely. Add `PolicyError::PrivilegedPort` and reject any ingress mapping whose `external_port` is below 1024, naming the port and suggesting >= 1024. - LOW (dead code): `protocol_str`'s explicit `IpProto::Icmp => "icmp"` arm is unreachable now that `validate_policy` rejects non-TCP/UDP ingress, and "icmp" is not a transport the forwarder accepts. Drop the arm so the `_ => "tcp"` fallback (the documented `#[non_exhaustive]` default) covers ICMP and any future variant. Refs: #499 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Commit pushed:
|
Review feedback addressedPushed a follow-up commit ( Fixed
Proof
|
This comment has been minimized.
This comment has been minimized.
Add two unit tests asserting that, on a non-OwnIp PTask, the ingress protocol and privileged-port checks run before the network-mode check: an ICMP mapping surfaces as UnsupportedIngressProtocol and a host port below 1024 surfaces as PrivilegedPort, rather than IngressRequiresOwnIp. This locks the documented ordering against future rearrangement. Addresses a review comment on #556. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Commit pushed:
|
|
Addressed the review comment on
The change is test-only and purely additive (48 insertions, 0 deletions). Gate green for the touched crate:
|
There was a problem hiding this comment.
Generated by sdd-review for issue #556
Comments that could not be inline-anchored
crates/minimald/tests/netns.rs:276
LOW · correctness — The teardown assertion can pass even if remove_ingress was a no-op.
The server listener is killed at lines 251–252, before remove_ingress is called at line 256. The post-unexpose connection probe at lines 257–264 then tries exec 3<>/dev/tcp/127.0.0.1/EXTERNAL.
If remove_ingress were a no-op, gvproxy would still hold a listening socket on 127.0.0.1:EXTERNAL. Since gvproxy is a userspace TCP forwarder it completes the host-side 3-way handshake before dialin…
crates/minvmd/src/vm.rs:164
LOW · spec compliance (R2.5) — apply does not call validate_for; the DM2 rejection relies entirely on caller discipline.
R2.5 requires vm_egress to be "rejected on DM2 with typed ConfigurationError." The rejection logic in validate_for is correct and well-tested. However, apply (the function that actually programs libkrun) performs no internal call to validate_for. A caller that sets vm_egress and calls apply without first calling validate_for on a DM2 host will silentl…
This comment has been minimized.
This comment has been minimized.
A dynamic_allowed_range whose lower bound exceeds its upper bound (e.g. (8443, 8000)) describes no ports under the inclusive semantics, yet it passed validate_policy silently, was stored on the Record, and was returned verbatim by GetSessionPolicy — a misconfig that would persist until #553's dynamic-port-mapping layer consumed it. Add a PolicyError::InvalidDynamicRange { lo, hi } variant and reject a reversed range in validate_policy for an OwnIp PTask, so the operator gets a clear message at session creation. Equal bounds (a single-port range) remain valid. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Commit pushed:
|
|
Addressed the review comment on Fixed
Gate —
|
|
@coderabbitai ignore |
✅ Action performedReviews paused. |
This comment has been minimized.
This comment has been minimized.
The UC6 PTask-to-PTask and the ingress port-mapping netns proofs retry the connect until the gvproxy switch/relay/MAC-learning is ready, bounded by a deadline. 10s is too tight on slow CI runners: every attempt's `timeout 2` connect fails before readiness, exhausting the deadline and flaking with status 124. 30s gives slow runners headroom; fast runners still exit on first success. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
This comment has been minimized.
This comment has been minimized.
Address two sdd-review findings on PR #556: - sessions: reject an ingress `dynamic_allowed_range` whose lower bound is a privileged host port (< 1024) via a new `PolicyError::PrivilegedDynamicRange`, mirroring the static-mapping privileged-port check, so the misconfig is named at launch rather than surfacing opaquely when #553's dynamic-port layer consumes it. - minvmd: validate that each `vm_egress.allow_subnets` entry is a valid CIDR prefix on the deployment models where `vm_egress` is accepted (DM1/DM3/DM4), via a new `VmError::InvalidEgressSubnet`, mirroring the per-PTask egress check in `Record::validate_policy`. The CIDR check is shared through a new `EgressPolicy::first_invalid_subnet` helper reused by both validators. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Commit pushed:
|
|
Addressed the two open
The CIDR check is shared between the per-PTask and VM-wide validators through a new
|
sdd-validate · Implementation boundaryBoundary resolved: Implementation — all 17 changed files are implementation files (no spec, architecture, or spike files). Applying the implementation gate set. Task: closes #499 · Tracking issue: #478 (already at Gate 1 — Proof artifacts re-executed and passingInfo · All four proof artifacts deferred to consumer CI (infrastructure limit) This agent runs inside the gh-aw network-restricted container without a Rust toolchain or package-registry egress. Direct
All artifacts exist in the diff, are targeted at the stated requirements, and fail on base (the implementations they drive do not exist there). No artifact would pass against an empty PR. The PR body confirms green execution with Gate 2 — Changed files within task scopeTask scope lists: Warning · The task assigned ingress apply/teardown to Warning · The task lists Warning · Adds the R2.1 Info · Info · 1-line Info · No change touches a protected path ( Gate 3 — No real credentials in the diffClean. No tokens, keys, passwords, or other credentials found in the diff. ResultClean pass — no Blocker findings.
Lifecycle: tracking issue #478 already carries
|
… policy imports) #554's branch carried stale-base copies of work that landed via #556: a duplicate NetError::InvalidPrefix variant and unused IngressPolicy/ SessionPolicy imports in rpc.rs. Take main's net/mod.rs and trim the imports so the rebased branch builds against the post-#556 main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
|
Commit pushed:
|
…orward CLI (#554) * feat(minimald,minimal2): add HTTPS reverse proxy with mTLS auth and ssh-forward * fix(minimald,minimal2,minvmd): address PR #554 review comments - Fix stale `-f` comment in ssh-forward (was never passed to ssh) - Write client.key with mode 0o600 to prevent world-readable TLS key - Fix DER→PEM in IssueClientCert doc comments (minimald-rpc, rpc.rs) - Restore NetError::InvalidPrefix for misconfigured prefix (distinct from SubnetExhausted which signals a valid but exhausted subnet) - Restore R1.4 SIGTERM behavior: re-add attached_count to GvproxySwitch and PtaskAttachment Drop impl that signals gvproxy on last PTask detach - Add R1.4 test last_ptask_detach_terminates_switch - R4.9: validate session in cmd_ssh_forward via RPC before exec()ing ssh; pass session UUID as SSH username for server-side gate - R4.9: validate session exists in channel_open_direct_tcpip via ssh_username before accepting the forward channel Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(minimald,minvmd): address PR #554 review comments (round 2) Restore async filesystem I/O in minimald's gvproxy switch helpers, add SwitchSubnet::new prefix validation in minvmd, and extract the direct-tcpip relay into a testable helper with a unit test. - minimald/src/net/mod.rs: revert std::fs → tokio::fs in write_config (create_dir_all/write), ensure_running (remove_file), and stop (remove_file) — blocking I/O on async threads was a regression introduced by the previous revise pass (caught by CodeRabbit). - minvmd/src/net.rs: add assert!((8..=29).contains(&prefix)) to SwitchSubnet::new — rejects prefixes too wide for MAC uniqueness (/7 or wider) or too narrow for host-address space (/30+), mirroring the validation in minimald::SwitchSubnet::new. - minimald/src/connection.rs: extract relay_streams<A, B> generic helper from channel_open_direct_tcpip; add unit test relay_streams_forwards_bytes_bidirectionally using tokio::io::duplex to demonstrate the relay path without a live SSH stack (addresses sdd-validate proof-artifact blocker). Closes #502 * fix(minimald): drop JoinHandle in RPC dispatch to silence must-use warning The revise pass converted match arms from expression style (arm value = JoinHandle, discarded by outer `;`) to block style (spawn(...); inside blocks), which caused rustc to flag each spawn call with `unused_must_use` under -D warnings. Wrap each spawn call with drop() so the JoinHandle is explicitly consumed rather than silently discarded as a statement value. * fix(minimald): collapse nested if-let into a let-chain (clippy) * fix(minvmd): restore thiserror dep after stale-base rebase The rebase onto main took main's net.rs (with the SwitchSubnetError thiserror derive) but this branch's stale minvmd/Cargo.toml predated that dependency. Restore thiserror.workspace + refresh the lock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * fix: restore unrelated pidfd spec deleted by stale-base (#552) This branch predated the #552 pidfd spec merge, so its tree would delete docs/specs/05-spec-minvmd-gvproxy-pidfd/ on merge. Restore from main — different feature, out of #502 scope. (Same stale-base drop fixed on the sibling ingress PR.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * fix(minimald): close fail-open direct-tcpip auth and apply clippy io_other direct-tcpip: restructure the nested `if let Some(...) && let Ok(...)` session validation to fail-closed `let-else` chains. Previously a connection with no SSH username (or a non-UUID username) bypassed session validation entirely and forwarded to any reachable TCP endpoint. Now missing or non-UUID usernames are rejected with a structured tracing::warn before the TcpStream::connect is attempted. proxy.rs: replace three `io::Error::new(io::ErrorKind::Other, e)` calls in `CertAuthority::build_server_config` with `io::Error::other(e)` (clippy `io_other_error`). Semantically identical; resolves the outstanding clippy `-D warnings` failure on the `networking-proxy` feature. * ci: gate the networking-proxy mTLS reverse-proxy proofs The HTTPS/mTLS reverse proxy is behind the non-default networking-proxy feature, so the workspace test job never compiled or ran its proofs (R4.5 mtls_missing_cert_returns_401, UC2b mtls_valid_cert_routes_to_backend). Add an explicit step so they run as a required check (sdd-validate Gate 1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * fix(rpc): reply with readable error when IssueClientCert lacks networking-proxy When minimald is built without the networking-proxy feature, the IssueClientCert dispatch arm dropped the channel without a response, so the client saw an opaque EOF/channel-close instead of a readable "feature not enabled" message. Send an Errorable::Err over the channel before closing so the client's oneshot_rpc surfaces a clear diagnostic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style: cargo fmt the IssueClientCert error-reply fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * fix: resolve rebase overlap with #556 (drop dup InvalidPrefix, unused policy imports) #554's branch carried stale-base copies of work that landed via #556: a duplicate NetError::InvalidPrefix variant and unused IngressPolicy/ SessionPolicy imports in rpc.rs. Take main's net/mod.rs and trim the imports so the rebased branch builds against the post-#556 main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * fix(minimald): use a fixed ASCII SAN for issued client certs, username in CN minimal2 forwards the login username (USER/LOGNAME) as subject_cn, which both sign_client_cert paths passed to CertificateParams::new() as a SAN. rcgen parses SANs as DNS names and rejects non-ASCII, so a non-ASCII username broke cert issuance / minimal login. The proxy authenticates on CA-signed cert presence (not the SAN/CN), so use a fixed ASCII SAN and carry the username in the subject CN (UTF-8-safe). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS --------- Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Norrie Taylor <norrie@minimal.dev>
Implements the spike-pinned, in-sandbox-verifiable slice of Unit 2 networking
policy for #499, per the human's decision (a). Egress enforcement (R2.2,
relay-layer frame inspection) and dynamic port-mapping (R2.4) are split to
#553, which owns the relay-frame-filter security design — the proved gvproxy
v0.8.9 spike established that gvproxy has no per-client egress ACL API and its
management API is unix-socket-only, so enforcement cannot be built on the
endpoint the original task body assumed.
Closes #499
What ships
Wire-types-down + re-export (authorized cross-crate edit).
PortMapping,EgressPolicy,IngressPolicy, andSessionPolicynow live insessionsandare re-exported from
minimald-rpc(nosessions→minimald-rpccycle). Apolicy: SessionPolicyfield onsessions::Recordstores the policy configuredat launch; the ~10
Recordconstruction sites default it to an all-Nonepolicy.
Record::validate_policyrejects an egress/ingress section on aNoNet/HostNetPTask; enforced at session launch insession.rs.net/policy.rsapplies static ingressport_mappingsatOwnIplaunch viaPOST /services/forwarder/exposeon thegvproxy control socket (
session_host.rs), andunexposes them on sessionexit.
minvmd'sVmConfiggainsvm_egress+ aDeploymentModeenum;validate_forrejectsvm_egresson DM2 with a typedVmError::Configuration.GetSessionPolicyreturns the liveRecordpolicy instead of thehardcoded default.
PolicyWarnLimiter);the egress-drop firing site lands with enforcement in Unit 2 follow-up: R2.2 egress enforcement (relay-layer frame inspection) + R2.4 dynamic port-mapping #553.
Scope note
The task body attributed "apply ingress at launch" to
session.rs, but the liveOwnIplaunch/attach path moved intosession_host.rswith #547. The ingressapply/teardown is therefore wired there (the only place that holds the PTask's
lease IP and control socket), with
session.rsthreading the policy through andperforming the R2.1 gate.
net/mod.rsgains the one-linepub mod policy;. Noprotected paths, manifests, or
Cargo.lockwere touched.Proof artifacts
All in-sandbox proofs were run green with
cargo test --locked; the twoprivileged-netns enforcement proofs are compiling
#[ignore]tests that run inci-netns.yml(this sandbox cannot create privileged network namespaces),matching the #496/#547 precedent.
Test (R2.1) — egress/ingress on a non-
OwnIpPTask is a parse-time error.sessionsunit tests fail on base (Record::validate_policydoes not exist):Test (R2.6, CLI-backing) —
minimal session policy <id>returns the egressconfigured at launch.
minimaldRPC round-trip test; fails on base (handlerreturned a hardcoded default):
It creates an
OwnIpsession carryingEgressPolicy { allow_subnets: ["10.0.0.0/8"], .. }, reads it back overGetSessionPolicy, and asserts thereturned
egressmatches (andingressisNone, not the oldSome(IngressPolicy::default())).minimal2'ssession policyCLI alreadyserialises this response to JSON.
Test (R2.5) —
vm_egresson a DM2VmConfigis rejected with a typederror.
minvmdunit tests; fail on base (vm_egress/validate_for/VmError::Configurationdo not exist):Test (R2.3/R2.4-static request shape) — forwarder expose request mapping.
minimaldnet::policyunit tests; fail on base (module does not exist):Test (R2.3/R2.4-static end-to-end,
#[ignore],ci-netns.yml) — staticingress port mapping.
netns_ingress_static_port_mapping_exposes_then_unexposesapplies
IngressPolicy { port_mappings: [{external_port: 18080, internal_port: 80, proto: tcp}] }on a live gvproxy switch, connects to127.0.0.1:18080fromthe host (forwarded into the PTask's listener), then
unexposes and confirms theforward is gone. Drives the production
apply_ingress/remove_ingress, neitherof which exists on base, so it cannot pass against an empty PR.
Gate
cargo build --locked,cargo test --locked -p minimald -p minvmd -p sessions,cargo fmt --all -- --check, andcargo clippy -p sessions -p minimald-rpc -p minimald -p minvmd -p minimal2 --all-targets -- -D warningsall pass.Next step
Merging this pull request closes task sub-issue #499. Once every task sub-issue
of the tracking issue is closed, the pipeline advances that tracking issue to
sdd:donefor a final human review.Summary by CodeRabbit
GetSessionPolicynow returns the exact policy stored in the active session record.policy: Default::default()and added a netns ingress expose/unexpose integration test.