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 7 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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ChangesLinux network host mode
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant ProgramSupport
participant StartScript
participant Docker
participant CallbackHandler
CLI->>ProgramSupport: Start with linux_network_host_mode
ProgramSupport->>StartScript: Forward --linux-network-host-mode
StartScript->>Docker: Configure host or bridge networking
Docker->>CallbackHandler: Send callback URL
CallbackHandler->>CallbackHandler: Validate and preserve loopback URL
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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
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.
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? |
Recreate the support container when the requested network mode changes, and validate callback URLs before dispatching webhooks.
|
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
Match e3-program-server by using reqwest redirect Policy::none() so callback validation cannot be bypassed via redirects.
|
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 |
Resolve and pin webhook destinations, reject unsafe addresses, disable redirects, and add callback validation tests to address CodeRabbit SSRF findings.
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
Pin resolved IPs, reject redirects, and block private/loopback addresses in host-network mode to address CodeRabbit SSRF findings.
|
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. |
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