fix(orchestrator): order egress config/firewall updates to close BYOP enable race

[arkamar]

The Update RPC mutates the in-netns kernel firewall and the in-memory egress config non-atomically. Since 1fc3820 ("feat(api): SOCKS5 egress proxy on sandbox network config (BYOP) (#2642)"), the kernel was relaxed before the userspace proxy learned about the SOCKS5 tunnel, so internal-destined TCP could briefly be direct-dialed from the host.

Order the two by direction: enabling BYOP publishes the proxy config before relaxing the firewall; disabling re-drops internal TCP first. The ordering lives in a single transitionEgress helper used by both the forward path and the rollback. The rollback fails closed - a failed kernel re-drop never publishes the weaker config - and reports the error instead of discarding it. Create/resume is already safe: the firewall is configured before the sandbox is visible to the proxy.

The helper takes the kernel-update func as a parameter so tests can observe the ordering; TestTransitionEgress pins publish-before-relax on enable, kernel-first on tighten, and both failure modes.

[cursor]

PR Summary

Medium Risk
Touches live sandbox network egress and firewall ordering on the Update path; behavior change is intentional but security-relevant if ordering regresses.

Overview
Sandbox Update egress changes now go through transitionEgress, which keeps the in-memory egress config and the in-netns firewall aligned. Enabling BYOP (SOCKS5 proxy) publishes proxy config before the kernel is relaxed; tightening or disabling applies firewall rules before the weaker config is published, closing a window where internal TCP could bypass the proxy. Rollbacks use the same helper and fail closed—failed kernel re-tighten does not publish weaker config, with errors reported via telemetry. Tests cover tighten-first on firewall failure and the enable/tighten ordering in transitionEgress.

^{Reviewed by Cursor Bugbot for commit 7cd4d7b. Bugbot is set up for automated code reviews on this repo. Configure here.}

[gemini-code-assist]

Code Review

This pull request refactors the egress configuration transition logic in the orchestrator server to ensure consistent state transitions between the in-memory configuration and the kernel firewall. It introduces helper functions transitionEgress and applyNetworkEgress to enforce a "loosen last, tighten first" ordering, and adds corresponding unit tests to verify this behavior and rollback scenarios. The review feedback suggests adding explicit nil checks for the egress configuration parameters in these helper functions to enhance robustness and prevent potential nil-pointer dereferences.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[codecov]

❌ 5 Tests Failed:

Tests completed	Failed	Passed	Skipped
3435	5	3430	7

View the top 3 failed test(s) by shortest run time

github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir/depth_1_lists_root_directory

Stack Traces | 0.01s run time

=== RUN   TestListDir/depth_1_lists_root_directory
=== PAUSE TestListDir/depth_1_lists_root_directory
=== CONT  TestListDir/depth_1_lists_root_directory
    filesystem_test.go:96: 
        	Error Trace:	.../tests/envd/filesystem_test.go:96
        	Error:      	Received unexpected error:
        	            	unavailable: 502 Bad Gateway
        	Test:       	TestListDir/depth_1_lists_root_directory
--- FAIL: TestListDir/depth_1_lists_root_directory (0.01s)

github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)

Stack Traces | 0.01s run time

=== RUN   TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)
=== PAUSE TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)
=== CONT  TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)
    filesystem_test.go:96: 
        	Error Trace:	.../tests/envd/filesystem_test.go:96
        	Error:      	Received unexpected error:
        	            	unavailable: 502 Bad Gateway
        	Test:       	TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)
--- FAIL: TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory) (0.01s)

github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir/depth_0_lists_only_root_directory

Stack Traces | 0.02s run time

=== RUN   TestListDir/depth_0_lists_only_root_directory
=== PAUSE TestListDir/depth_0_lists_only_root_directory
=== CONT  TestListDir/depth_0_lists_only_root_directory
    filesystem_test.go:96: 
        	Error Trace:	.../tests/envd/filesystem_test.go:96
        	Error:      	Received unexpected error:
        	            	unavailable: 502 Bad Gateway
        	Test:       	TestListDir/depth_0_lists_only_root_directory
--- FAIL: TestListDir/depth_0_lists_only_root_directory (0.02s)

github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir

Stack Traces | 1.6s run time

=== RUN   TestListDir
=== PAUSE TestListDir
=== CONT  TestListDir
--- FAIL: TestListDir (1.60s)

View the full list of 1 ❄️ flaky test(s)

github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir/depth_3_lists_all_directories_and_files

Flake rate in main: 30.77% (Passed 9 times, Failed 4 times)

Stack Traces | 0.01s run time

=== RUN   TestListDir/depth_3_lists_all_directories_and_files
=== PAUSE TestListDir/depth_3_lists_all_directories_and_files
=== CONT  TestListDir/depth_3_lists_all_directories_and_files
    filesystem_test.go:96: 
        	Error Trace:	.../tests/envd/filesystem_test.go:96
        	Error:      	Received unexpected error:
        	            	unavailable: 502 Bad Gateway
        	Test:       	TestListDir/depth_3_lists_all_directories_and_files
--- FAIL: TestListDir/depth_3_lists_all_directories_and_files (0.01s)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[claude]

No bugs found, but this reorders firewall/config mutations to close a BYOP egress-proxy security race, so it's worth a human sanity check given the security-sensitive surface.

What was reviewed:

• Traced transitionEgress's two branches (publish-before-relax on BYOP enable, kernel-first on tighten) against both the forward and rollback call sites in Update().
• Verified Firewall.ApplyRules' nftables flush is a single atomic batch that resets the conn on error, backing the "kernel keeps its rules on failure" assumption the rollback relies on.
• Checked the previously-flagged "rollback closure unreachable" point: it's dead code today (egress is always the last update in the list, so nothing after it can trigger its rollback) but harmless and forward-compatible, not a bug.

Extended reasoning...

Overview

The PR reorders how packages/orchestrator/pkg/server/sandboxes.go's Update RPC mutates the in-netns nftables firewall vs. the in-memory sandbox egress config, extracting the logic into a new transitionEgress helper shared by the forward apply and the rollback closure. When enabling BYOP it now publishes the proxy config before relaxing the firewall; when disabling/tightening it updates the firewall first. The rollback path now fails closed and reports failures via telemetry instead of silently discarding them. Test coverage is solid: TestTransitionEgress directly pins the ordering and both failure modes, and existing Update-level tests still pass.

Security risks

This is itself a security fix: it closes a window where the kernel firewall could be relaxed (letting internal TCP be dialed directly) before the userspace SOCKS5 proxy learned about the tunnel. The fix's correctness leans on the nftables netlink flush being atomic (verified in Firewall.ApplyRules — a single conn.Flush() with the conn reset on error), so a failed updateInternet call is assumed to leave prior rules intact. That assumption held up on inspection but is exactly the kind of infra-level invariant worth a second set of eyes.

Level of scrutiny

Firewall/egress-proxy ordering in the orchestrator is security-sensitive production code (guards sandbox network isolation for BYOP customers), so this warrants human review even though the logic checked out and is well tested. It's a narrowly-scoped fix with no unrelated changes, which lowers risk, but the subtlety of the ordering invariant and its dependence on nftables atomicity semantics is not something to auto-approve.

Other factors

The bug-hunting pass raised and ruled out one candidate (egress rollback closure unreachable via Update()) — confirmed as dead-but-harmless code since egress is always the last item in the updates slice today, so nothing after it can fail and trigger its rollback. No other issues found during my own read of the diff and the supporting network/firewall code.

[jakubno]

otherwise lgtm