fix(egress): switch mitmproxy to connection_strategy=eager#974
Closed
Pangjiping wants to merge 2 commits into
Closed
fix(egress): switch mitmproxy to connection_strategy=eager#974Pangjiping wants to merge 2 commits into
Pangjiping wants to merge 2 commits into
Conversation
Lazy was originally chosen so denied requests could short-circuit before touching upstream, but in practice the egress policy defaults to allow and denies only a small minority of requests. The savings on the deny path do not justify the cost on the allow path: lazy checks the upstream connection pool per-request, and on h1 keepalive sessions this exposes a stale-conn race where the second request (e.g. POST /git-upload-pack right after GET /info/refs) picks an upstream conn the peer has already closed, surfacing as a silent transport error with no fatal output on the client. Eager opens the upstream connection alongside the client connection, so mitm's IO loop continuously observes upstream FIN/RST and a stale conn is detected before the client's next request arrives. The cost is a wasted TCP/TLS handshake for the small fraction of denied requests — acceptable because a denied flow still short-circuits with `flow.response = ...` before any HTTP write reaches upstream, so upstream sees no path / method / headers, only an unfinished handshake. Audited the bundled addons (system.py, custom.py): no hook depends on lazy-only behavior (no server_connect routing, no flow.request.host modification, no client-SNI-based upstream decisions).
Contributor
|
Changed directories: components. 📋 Recommended labels (based on changed files):
Other available labels:
💡 Tip: Use cc @Pangjiping |
Pangjiping
force-pushed
the
fix/egress-mitmproxy-connection-strategy-eager
branch
from
218a0fd to
2ae0422
Compare
…ibility PR opensandbox-group#951 moved the egress binary from /egress to /opt/opensandbox-egress/egress so the supervisor and binary could share a single grouped directory. External tooling and older deployment manifests may still reference the old /egress path; add a symlink so both paths resolve to the same binary. Symlink rather than COPY: zero extra image size, single source of truth for chmod and replacement, and `exec /egress` resolves to the supervisor-managed binary like before.
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
1 participant
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
Lazy was originally chosen so denied requests could short-circuit before touching upstream, but in practice the egress policy defaults to allow and denies only a small minority of requests. The savings on the deny path do not justify the cost on the allow path: lazy checks the upstream connection pool per-request, and on h1 keepalive sessions this exposes a stale-conn race where the second request (e.g. POST /git-upload-pack right after GET /info/refs) picks an upstream conn the peer has already closed, surfacing as a silent transport error with no fatal output on the client.
Eager opens the upstream connection alongside the client connection, so mitm's IO loop continuously observes upstream FIN/RST and a stale conn is detected before the client's next request arrives. The cost is a wasted TCP/TLS handshake for the small fraction of denied requests — acceptable because a denied flow still short-circuits with
flow.response = ...before any HTTP write reaches upstream, so upstream sees no path / method / headers, only an unfinished handshake.Audited the bundled addons (system.py, custom.py): no hook depends on lazy-only behavior (no server_connect routing, no flow.request.host modification, no client-SNI-based upstream decisions).
Testing
Breaking Changes
Checklist