Fix ignored QUIC connections never relaying past the ClientHello - #8339
Open
kasnder wants to merge 2 commits into
Open
Fix ignored QUIC connections never relaying past the ClientHello#8339kasnder wants to merge 2 commits into
kasnder wants to merge 2 commits into
Conversation
When an addon sets `ignore_connection` in the `tls_clienthello` hook, `ClientQuicLayer.receive_handshake_data` swaps the QUIC layers for an ignored `UDPLayer`. From the first event after the ClientHello, mitmproxy then raises AssertionError: Unexpected event type at UDPLayer.start: Expected Start, got DataReceived(client, ...) and nothing is relayed in either direction, so the connection stalls. With `--mode reverse:quic://` and an aioquic client, the handshake never completes and the target server sees no datagrams at all. Two things go wrong, both because the swap does not account for the layers it replaces. 1. The parent layer's dispatch is patched twice, and neither assignment holds. `parent_layer._handle_event = replacement_layer._handle_event` captures a bound method while the UDP layer is still in its `start` state; `UDPLayer.start` later sets `self._handle_event = self.relay_messages` on the replacement layer, which the parent never re-reads. The accompanying `parent_layer.handle_event = replacement_layer.handle_event` cannot compensate, because an outer layer may already hold a bound reference to the original -- `NextLayer._ask` caches exactly that as `NextLayer._handle` when it decides the layer stack, which happens on the first datagram, before the ClientHello is parsed. Events arrive through the cached method, find the parent unpaused, and dispatch into the frozen `start` state. Keep `handle_event` untouched and replace only `_handle_event`, with a dispatcher that delegates to the UDP layer's current state function. `Layer.handle_event` re-reads `self._handle_event` for every event, so that is the only assignment a stale cache cannot defeat. The parent then keeps owning blocking and pause handling, and datagrams that arrive while `OpenConnection` is pending are relayed in order once it completes. 2. A command issued before the swap by one of the replaced layers can still be in flight, and its reply is routed to the parent, which now dispatches it into the UDP layer. `RawQuicLayer(force_raw=True)` builds a datagram `UDPLayer` in its constructor, so `--mode reverse:quic://` reliably has an outstanding `UdpStartHook` when the ClientHello arrives. Drop such replies: the parent resumes the UDP layer's own blocking commands itself, so anything reaching the dispatcher belongs to a layer that no longer exists. Neither facet is a race, and neither depends on the connection strategy. The first fires on the first post-ClientHello event in either server state, and the `OpenConnectionCompleted` reply is misdelivered the same way -- it is `replacement_layer` that holds `_paused` while the event reaches `parent_layer`. The existing `test_passthrough_from_clienthello` misses this because `tutils.Playbook` re-resolves `self.layer.handle_event` for every event, so it always reaches the patched attribute, and `make_client_tls_layer` wires child layers by hand to avoid `NextLayer`. The added tests drive events through a reference bound up front, which is what a real server run does, and feed back a reply the ignored layer never asked for. All three fail without this fix.
1 task
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.
Description
Fixes #8338: QUIC connections never relay past the ClientHello when an addon sets
ignore_connectionin thetls_clienthellohook. The connection stalls in both directionsand mitmproxy raises
AssertionError: Unexpected event type at UDPLayer.start.ClientQuicLayer.receive_handshake_dataswaps the QUIC layers for an ignoredUDPLayer, andtwo things go wrong, both because the swap does not account for the layers it replaces.
The parent layer's dispatch is patched twice, and neither assignment holds.
parent_layer._handle_event = replacement_layer._handle_eventcaptures a bound method whilethe UDP layer is still in its
startstate;UDPLayer.startlater setsself._handle_event = self.relay_messageson the replacement layer, which the parent neverre-reads. The accompanying
parent_layer.handle_event = replacement_layer.handle_eventcannot compensate, because an outer layer may already hold a bound reference to the
original —
NextLayer._askcaches exactly that asNextLayer._handlewhen it decides thelayer stack, which happens on the first datagram, before the ClientHello is parsed. Events
arrive through the cached method, find the parent unpaused, and dispatch into the frozen
startstate.This keeps
handle_eventuntouched and replaces only_handle_event, with a dispatcher thatdelegates to the UDP layer's current state function.
Layer.handle_eventre-readsself._handle_eventfor every event, so that is the only assignment a stale cache cannotdefeat. The parent then keeps owning blocking and pause handling, and datagrams that arrive
while
OpenConnectionis pending are relayed in order once it completes.A command issued before the swap can still be in flight, and its reply is routed to the
parent, which now dispatches it into the UDP layer.
RawQuicLayer(force_raw=True)builds adatagram
UDPLayerin its constructor, so--mode reverse:quic://reliably has anoutstanding
UdpStartHookwhen the ClientHello arrives. Such replies are now dropped: theparent resumes the UDP layer's own blocking commands itself, so anything that reaches the
dispatcher answers a command from a layer that no longer exists.
Neither facet is a race, and neither depends on
connection_strategy. Both fire in eitherserver state.
Why the existing test misses this
test_passthrough_from_clienthellopasses today becausetutils.Playbookre-resolvesself.layer.handle_eventfor every event, so it always reaches the patched attribute, andmake_client_tls_layerwires child layers by hand "to avoid NextLayer noise". The addedtests drive events through a reference bound up front, which is what a real server run does,
and feed back a reply the ignored layer never asked for.
Verification
End-to-end, with an
aioquicecho server behindmitmdump --mode reverse:quic://and anaddon setting
ignore_connection, driven by anaioquicclient:mainConnectionError(no handshake)mitmproxy has crashed!Test suite on
main(c69122b), Python 3.13.14:test/mitmproxy/proxy+test/mitmproxy/addons/test_next_layer.py: 546 passed, 4 skipped.end-to-end run produces.
ruff format --check,ruff check, andmypyon the changed module are clean.Checklist