Repository navigation
docs(spec-minvmd-gvproxy-pidfd): minvmd: signal gvproxy switch via pidfd to close the independent-crash recycled-PID window - #552
Merged
Conversation
Replace raw-PID kill with pidfd_send_signal in GvproxySwitch to close the recycled-PID window that opens when gvproxy crashes independently. Closes #550 tracking issue spec phase (tracking issue stays open). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new spec document is added under Changesminvmd GvproxySwitch pidfd spec
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Author
|
norrietaylor
enabled auto-merge (squash)
June 23, 2026 23:08
norrietaylor
approved these changes
Jun 23, 2026
norrietaylor
added a commit
that referenced
this pull request
Jun 24, 2026
#556's branch predated the #552 pidfd spec merge, so its tree lacked docs/specs/05-spec-minvmd-gvproxy-pidfd/ and would delete it on merge. Restore it from main — it is a different feature's spec, out of #499 scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS
norrietaylor
added a commit
that referenced
this pull request
Jun 24, 2026
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
norrietaylor
added a commit
that referenced
this pull request
Jun 24, 2026
… VM-egress DM2 rejection (#556) * feat(sessions,minimald,minvmd): static ingress, session policy state, VM-egress DM2 rejection * fix(minimald,sessions,minvmd): address review findings on net policy 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> * fix(minvmd): restore thiserror dep after stale-base rebase 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 * feat(sessions,minimald): reject privileged ingress host ports below 1024 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> * test(sessions): pin validate_policy check ordering on non-OwnIp ingress 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> * fix(minimald): bind ingress to loopback, align warn window and fields Address sdd-review findings on PR #556 (crates/minimald/src/net/policy.rs): - Bind static ingress forwards to 127.0.0.1 instead of all host interfaces (the empty-host ":PORT" form), matching R2.3 ("a process on the host can connect") and the spec's "no external exposure by default" so the forward stays off the LAN. Update expose_request, its doc, and its unit tests. - Set WARN_MIN_INTERVAL to 60s to match R2.2's "first drop per PTask per rule per minute" rate-limit window (was 1s). - Give PolicyWarnLimiter::warn the R2.7 structured fields (session_id, direction, remote_addr, proto, rule_matched) via a new Direction enum, so the #553 enforcement firing site cannot silently drop them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(minimald): log detach failure in ingress-apply rollback The ingress-apply rollback path in attach_own_ip discarded the detach result with `let _ =`, swallowing any error. The normal teardown path logs detach failures via tracing::warn!; mirror that here so a cascading failure (ingress apply fails, then detach also fails) is no longer invisible in the logs. Carry the originating ingress error as a field for context. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: restore unrelated pidfd spec deleted by stale-base (#552) #556's branch predated the #552 pidfd spec merge, so its tree lacked docs/specs/05-spec-minvmd-gvproxy-pidfd/ and would delete it on merge. Restore it from main — it is a different feature's spec, out of #499 scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * feat(sessions,minimald): validate egress subnet CIDRs at launch Address review feedback on the PR #556 networking-policy slice: - Reject malformed `allow_subnets` entries for an `OwnIp` PTask at launch. `Record::validate_policy` now parses each egress subnet as a CIDR prefix and returns the new `PolicyError::InvalidSubnet` naming the offending string, rather than storing an invalid prefix verbatim and letting it surface opaquely when #553's enforcement layer parses it. Validation is dependency-free (std::net), permitting host bits and both IP families. - Document that `IngressPolicy::dynamic_allowed_range` is stored and returned by `GetSessionPolicy` but not yet enforced; dynamic port-mapping is split to #553. - Guard `protocol_str`'s `#[non_exhaustive]` fallback arm with a `debug_assert!` so any future caller that reaches it without passing `validate_policy` is caught in test runs, while TCP stays the production fallback rather than a silently misrouted request. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(sessions,minimald): log policy-violation proto in snake_case Add a `Display` impl for `IpProto` rendering the lowercase transport name, and switch `PolicyWarnLimiter::warn`'s structured field from `?proto` (Debug: `Tcp`/`Udp`/`Icmp`) to `%proto` (Display: `tcp`/`udp`/`icmp`) so the log field matches `IpProto`'s `snake_case` serde wire representation, per PR review on R2.7 log-field consistency. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(minimald): cap gvproxy control-response read at 64 KiB read_to_end on the gvproxy control socket had no byte cap; a misbehaving socket that streams without closing could buffer unbounded memory for the whole GVPROXY_CONTROL_TIMEOUT window. Bound the read with AsyncReadExt::take at MAX_CONTROL_RESPONSE (64 KiB), far above any legitimate forwarder reply. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0143kv2BRrRqGxmVwwHskQtS * fix(minimald): validate session policy at CreateSession time R2.1 deferred policy/network-mode validation to first attach: a CreateSession carrying an invalid policy (e.g. egress on a non-OwnIp PTask) was written to the store and only rejected when a client later opened a shell, leaving an invalid session persisted indefinitely. Validate the record in the CreateSession handler so an invalid session is never stored, and surface the typed PolicyError to the client as a clean CreateSession error (InvalidInput) rather than a transport failure. Covered by a new rpc test asserting a HostNet+egress create is rejected and nothing is left in the store. Addresses review feedback on #556. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Norrie Taylor <norrie@minimal.dev>
norrietaylor
added a commit
that referenced
this pull request
Jun 24, 2026
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
norrietaylor
added a commit
that referenced
this pull request
Jun 24, 2026
…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>
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.
Summary
This spec covers replacing raw-PID
kill(2)withpidfd_send_signal(2)inGvproxySwitch(crates/minvmd/src/net.rs) to structurally close the recycled-PID signal hazard that opens when gvproxy crashes independently.Demoable unit:
pidfd: OwnedFdfield toGvproxySwitch(opened viapidfd_openat spawn), replacesignal_child(pid, ...)withsignal_pidfd(pidfd, ...)on all teardown paths (stop(),Drop), and add a test assertingpidfd_send_signalreturnsESRCHon a reaped child.Proof artifacts:
cargo test -p minvmd net::tests::pidfd_signal_to_reaped_child_returns_esrchpasses (test does not exist on base branch).grep -q 'pidfd' crates/minvmd/src/net.rspasses after the PR lands.Tracking issue: #550
Next step
This is a single-PR (fast-path) spec. Merging this spec PR returns the tracking issue to
sdd:fastpath; comment/approveon the tracking issue to dispatch the implementation — or comment/approvenow (while this PR is open) to record the approval and let the merge dispatch the implementation automatically (merge and approve commute, ADR 0024).Closes #551
Summary by CodeRabbit