feat(cli): add --linux-network-host-mode for program start - #1733
feat(cli): add --linux-network-host-mode for program start#1733crazywriter1 wants to merge 9 commits into
Conversation
|
@crazywriter1 is attempting to deploy a commit to the Gnosis Guild Team on Vercel. A member of the Team first needs to authorize it. |
5925bde to
b6b1d95
Compare
Toby1009
left a comment
Toby1009
left a comment
There was a problem hiding this comment.
LGTM
Please update this branch with the latest main before merging.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (10)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ChangesLinux network host mode
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to Callback integrations that rely on HTTP redirects could fail under the updated response handling; confirm redirects are unsupported or add coverage before merging. Sequence Diagram(s)sequenceDiagram
participant CLI
participant ProgramSupport
participant StartScript
participant Docker
participant WebhookHandler
CLI->>ProgramSupport: Start with linux_network_host_mode
ProgramSupport->>StartScript: Forward --linux-network-host-mode
StartScript->>Docker: Configure host or bridge networking
Docker->>WebhookHandler: Send callback URL
WebhookHandler->>WebhookHandler: Preserve or rewrite localhost URL
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/support-scripts/ctl/container`:
- Around line 55-80: Update the existing-container branch around the docker ps
check to verify that the container’s network mode and
INTERFOLD_SKIP_LOCALHOST_REWRITE setting match the requested
LINUX_NETWORK_HOST_MODE. When they differ, remove/recreate the container so
docker run applies the requested DOCKER_NET_ARGS and DOCKER_ENV_ARGS; otherwise
preserve the current docker exec behavior, or reject the command with a clear
removal instruction.
In `@crates/support/app/src/main.rs`:
- Around line 150-161: Validate callback_url before dispatching the background
task, including the skip-rewrite branch: allow only approved schemes and hosts,
and reject private or loopback destinations. Apply this validation in the
/run_compute request flow before the reqwest::Client::post spawn, while
preserving the existing localhost rewrite behavior when
INTERFOLD_SKIP_LOCALHOST_REWRITE is disabled.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ba92779-3915-4e08-a24e-3d9dd632b24e
📒 Files selected for processing (10)
crates/cli/src/program.rscrates/support-scripts/ctl/containercrates/support-scripts/ctl/startcrates/support-scripts/src/lib.rscrates/support-scripts/src/program.rscrates/support-scripts/src/program_dev.rscrates/support-scripts/src/program_risc0.rscrates/support-scripts/src/traits.rscrates/support/README.mdcrates/support/app/src/main.rs
|
I've updated the branch with the latest main. Please let me know if you'd like me to address the remaining CodeRabbit suggestions as well. @Toby1009 |
ctrlc03
changed the title
0dc586b to
68e713c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@ctrlc03 Thanks for the review sir. Addressed your two suggestions in the latest push ( CodeRabbit also suggested container mode mismatch handling and callback URL validation. Should I address those in this PR as well? |
yes please can you address coderabbit comments too? |
|
Addressed the remaining CodeRabbit feedback:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/support/app/src/main.rs`:
- Around line 107-139: Update the webhook HTTP client construction around
reqwest::Client::new so redirect destinations cannot bypass
validated_callback_url’s host policy. Prefer disabling automatic redirects for
webhook delivery, or configure redirect handling to apply the same
loopback/private/reserved destination validation to every target while
preserving permitted callback behavior.
- Around line 91-105: Update host_is_private_or_reserved and the callback
validation/dispatch flow to resolve hostname targets before sending requests,
reject any resolved loopback, private, link-local, multicast, unspecified, or
reserved address, and prevent DNS rebinding by pinning the validated destination
address for dispatch; alternatively enforce that callback hosts match configured
origins. Preserve existing rejection behavior for invalid hosts and direct IP
targets.
- Around line 100-105: Update the IP validation expression around the visible
`ip` checks to replace unavailable `IpAddr::is_private()` and
`IpAddr::is_link_local()` calls with a parenthesized match over `IpAddr::V4` and
`IpAddr::V6`, invoking the corresponding address-family-specific methods while
preserving the existing multicast, unspecified, and special IPv4 checks.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 78295c27-20b8-49be-a62c-c29086403ef7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/support-scripts/ctl/containercrates/support/app/src/main.rs
|
Applied redirect Policy::none() to match e3-program-server. The DNS resolution/pinning suggestions go beyond what program server does today. Happy to discuss separately if you want stricter SSRF policy across both servers. |
please make sure you fix all coderabbit issues. thanks |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/support/app/src/main.rs`:
- Around line 421-431: Update host_mode_pins_localhost to use an IP-literal URL
instead of localhost, avoiding DNS resolver dependence while still exercising
loopback pinning. Replace the invalid callback.url.host().unwrap().is_loopback()
assertion by matching url::Host::Ipv4 and Host::Ipv6 and checking is_loopback()
on the contained address, then separately verify the expected host_header
behavior with a localhost input.
- Around line 154-162: Update the callback request flow around pin_callback_url
to preserve the original hostname in the URL instead of replacing it with the
resolved IP. Configure the reqwest ClientBuilder resolve mapping to pin the
hostname to the resolved address, and remove the host_header override while
retaining correct IPv4/IPv6 handling.
- Around line 114-131: Canonicalize IPv4-mapped IPv6 addresses before
destination classification: update ip_is_private_or_reserved and
ip_is_safe_public_destination to convert Ipv6Addr::to_ipv4_mapped() results into
IpAddr::V4 and reuse the existing IPv4 checks, while preserving normal IPv6
handling. Add regression tests covering mapped loopback and private addresses,
including ::ffff:127.0.0.1 and ::ffff:10.0.0.1.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5633d630-01c0-4641-900c-894e74cc9a88
📒 Files selected for processing (1)
crates/support/app/src/main.rs
|
Maybe I think the callback security changes have grown beyond what #548 needed, and #548 asked for an optional Linux host-networking flag. The PR now adds 412 lines, Three concerns:
I suggest keeping the flag, plumbing, non-Linux guard, container recreation, and Two bugs worth fixing either way:
Happy to be corrected if I have misunderstood the intended trust |
|
@Toby1009 Thanks for the detailed review. The callback security work grew because @ctrlc03 asked me to address the remaining CodeRabbit comments; I wasn't aiming to expand #548 beyond the flag. I agree it's gotten large and the policy inconsistency with e3-program-server is worth discussing separately. Happy to split the security changes out and keep this PR to flag + plumbing + container recreation + Policy::none(), and fix the two container script bugs either way. Leaving the call to you and @ctrlc03. Just say which direction and I'll update the branch. |
Lets Docker use --network=host so callbacks work without host.local rewrite or firewall tweaks for the bridge network. Closes theinterfold#548
Use tracing::warn! for the dev-mode notice and reject --linux-network-host-mode on non-Linux hosts.
Recreate the support container when the requested network mode changes, and validate callback URLs before dispatching webhooks.
Match e3-program-server by using reqwest redirect Policy::none() so callback validation cannot be bypassed via redirects.
Resolve and pin webhook destinations, reject unsafe addresses, disable redirects, and add callback validation tests to address CodeRabbit SSRF findings.
Pin resolved IPs, reject redirects, and block private/loopback addresses in host-network mode to address CodeRabbit SSRF findings.
95f2d9a to
4692970
Compare
@crazywriter1 I still lean towards trimming this back to what #548 The tests for the new validation don't run anywhere as far as I can tell. The IP denylist still isn't settled after three CodeRabbit rounds, and these are Here's what I'd keep, if trimming makes sense:
Two things in
|
Remove URL validation, DNS pinning, and tests added in 13e4cdf/46929708. Keep Policy::none() redirect blocking and INTERFOLD_SKIP_LOCALHOST_REWRITE host-mode conditional. ctl/container: propagate docker exec exit status instead of exit 0. ctl/container: wait up to 5 s for --rm cleanup before docker run to avoid name conflict on mode-mismatch recreate.
|
Hey @Toby1009, trimmed it down as you suggested. Kept:
Dropped Also fixed the two
|
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
3 participants
Summary
--linux-network-host-modetointerfold program startfor Linux Docker host networkinglocalhost→host.localrewrite when the flag is set (INTERFOLD_SKIP_LOCALHOST_REWRITE)crates/support/README.mdCloses #548
Test plan
CARGO_BUILD_JOBS=1 cargo build -p e3-cli./target/debug/interfold program start --help | grep -A2 linux-network-host-modectl/start/ctl/containerdry-runs with stub dockerSummary by CodeRabbit
New Features
Bug Fixes
Documentation