Assert on the ALPN extension in test_tunnel_sets_http_11_alpn - #5232
Conversation
The test looked for `http/1.1` and `h2` in the ClientHello as a whole, but the random bytes, the session id and the key shares can spell either one out by accident. Locally that made it fail roughly once in 400 runs, and more often on CI, where the key share carries a post-quantum group and the record is correspondingly larger. Parse the ALPN extension and compare the protocols it offers instead. That is both stable and stricter: it now checks that `http/1.1` is the only protocol offered, rather than that its bytes appear somewhere. The handler also reads until the announced record is complete, so a larger ClientHello split across packets cannot reintroduce the flakiness.
|
The red
While setting up Python 3.13 with OpenSSL 3.5.7 to check that, I got a better number for this PR than the one in the description. On that OpenSSL the ClientHello is 1531 bytes rather than 517, and |
illia-v
left a comment
There was a problem hiding this comment.
Thanks again for looking into this test!
Could we simplify the fix by wrapping the handler socket with an SSL context configured to prefer HTTP/2 over HTTP/1.1, then asserting that ssl.SSLSocket.selected_alpn_protocol returns http/1.1?
That should catch accidentally advertising HTTP/2 without needing to parse the ClientHello.
The handler now completes a TLS handshake with a context that prefers HTTP/2, so the protocol it negotiates comes back as http/1.1 only when the client offered nothing else. That is the guarantee the ClientHello parser was there to give, without the parser.
|
Much better, thanks, done. The handler now completes a real handshake with a context offering Verified with |
test_tunnel_sets_http_11_alpnasserted against the whole ClientHello:self.bufis the entire record, most of which is random, sob"h2"turnsup in it by chance. Measuring 3000 handshakes locally (OpenSSL 3.0.13, a
517-byte ClientHello), 7 of them contained
b"h2"somewhere, about one in430. CI is worse: newer OpenSSL sends a post-quantum key share, so the
record is several times larger and the odds scale with it. I hit it twice
in 300 local runs of the test and once on Windows CI.
This parses the ALPN extension out of the ClientHello and compares the
protocol list:
That is stricter than what it replaces, it checks
http/1.1is the onlyprotocol offered, instead of checking its bytes appear somewhere in the
packet, and the failure message becomes readable:
rather than a hex dump of the ClientHello.
The socket handler now also reads until the record announced by its header
is complete, instead of assuming one packet. A post-quantum ClientHello is
big enough that this is worth not relying on, and an incomplete read would
otherwise just trade one flake for another.
Verification
ALPN_PROTOCOLSto["h2", "http/1.1"]makes it fail, so theassertion still catches the regression it is there to catch.
["http/1.1"],["h2", "http/1.1"]and no ALPN at all, over 3000 handshakes.test_socketlevel.py: 122 passed, 8 skipped.