Repository navigation
Conversation
bryancall
left a comment
There was a problem hiding this comment.
❌ Request changes
Thanks for tracking this down. The inbound fix is right, and the new test proves it: tls_hooks_vconn_start_reject fails on 493bc01 and passes with this patch on my box.
Still blocking: the new check also fires on outbound connections
reenable() sets SSL_HANDSHAKE_ERROR for both directions. On an origin connection, a TS_SSL_VERIFY_SERVER_HOOK plugin that reenables with TS_EVENT_ERROR under verify_server_policy: PERMISSIVE is supposed to log Action=Continue and let the handshake finish (verify_callback returns !enforce_mode). The status stays at ERROR until the handshake completes, so with a TLS 1.2 origin the next net_read_io hits the new check and the origin connect fails.
I reproduced it with a copy of tls/tls_hooks_verify.test.py that adds proxy.config.ssl.client.TLSv1_3.enabled: 0 and proxy.config.ssl.origin_session_cache.enabled: 0, and asserts the "request bad name permissive" run gets a 200:
| bar.com origin handshakes | Result | |
|---|---|---|
| base 493bc01 | 1 | 200 |
| this PR | 4 (1 + 3 retries), each closed by the new check | 502 |
With the origin session cache on, the third attempt resumes a cached session, skips the verify hook and gets a 200, which is why the existing test stays green.
Limiting the check to inbound fixes it:
if (get_context() == NET_VCONNECTION_IN && sslHandshakeStatus == SSLHandshakeStatus::SSL_HANDSHAKE_ERROR) {With that one change, tls_hooks_vconn_start_reject, tls_hooks_verify and both TLS 1.2 variants pass, and the permissive request is back to a single handshake. Adding the TLS 1.2, no-session-cache permissive case as a test would keep this covered.
Non-blocking: deferred rejection at the later hooks
As far as I can tell, a deferred TS_EVENT_ERROR reenable at TS_SSL_CLIENT_HELLO_HOOK or TS_SSL_CERT_HOOK reaches the same read_disable path, so this change fixes those too. A --hook option on vconn_start_reject.cc would cover them cheaply.
Nit: the async run cannot tell the async branch ran
Both plugin log lines match Rejecting connection. Asserting Rejecting connection .* asynchronously for the async run would catch a dropped --async.
Nit: reject_probe.py fails on any received bytes
tls_client_hello.py accepts a TLS alert as a rejection, but reject_probe.py fails on any byte, so a later change that sends handshake_failure on rejection would turn it red. Its comment also calls the payload "the start of a ClientHello", while the record and handshake lengths declare more bytes than are sent. "Truncated ClientHello" would be more accurate.
551ddb0 to
f878699
Compare
|
@bryancall thanks for the thorough review and the repro. All four points are addressed in the force-pushed f878699. Outbound connections. The Deferred rejection at the later hooks. The test plugin is now
The synchronous cert case turned up a separate bug: Async assertion nit. The async runs now require reject_probe.py nit. The probe now does a real handshake with Python's The new tests and the related TLS hook, verify, rate_limit and abuse_shield autests (51 total) pass locally. |
A plugin that rejects an inbound TLS connection with TSVConnReenableEx(TS_EVENT_ERROR) often does not get it closed. The error marks the handshake as failed, but getSSLHandShakeComplete() treats any non-ongoing state as complete. A rejection at TS_VCONN_START_HOOK, or a deferred one at the ClientHello or cert hook, makes the read path skip the handshake, disable the read, and cancel the inactivity timeout, so the socket sits in CLOSE-WAIT until ATS restarts. abuse_shield rejects blocked IPs at TS_VCONN_START_HOOK, and a production fleet using its block action leaked sockets until the hosts ran out of memory. A rejection from within the cert hook was ignored outright and the handshake completed, while one from within the ClientHello hook only worked because the certificate callback mistook the error state for a client renegotiation attempt. This patch makes the inbound SSL read path signal a read error when a plugin has rejected the handshake, and makes the ClientHello and cert callbacks fail the handshake explicitly in that state. Outbound connections are left alone, since a TS_SSL_VERIFY_SERVER_HOOK error under a PERMISSIVE policy must let the origin handshake finish. A new autest rejects connections from each of these hooks, both from within the hook and asynchronously, and checks that every connection is closed. Another covers the permissive origin verification case over TLS 1.2 without the session cache, and the abuse_shield connection rate test now checks that blocked TLS connections are closed. Co-authored-by: Claude Opus 5.5 Medium <noreply@anthropic.com>
f878699 to
38b1a35
Compare
A plugin that rejects an inbound TLS connection with
TSVConnReenableEx(TS_EVENT_ERROR) often does not get it closed. The
error marks the handshake as failed, but getSSLHandShakeComplete()
treats any non-ongoing state as complete. A rejection at
TS_VCONN_START_HOOK, or a deferred one at the ClientHello or cert hook,
makes the read path skip the handshake, disable the read, and cancel
the inactivity timeout, so the socket sits in CLOSE-WAIT until ATS
restarts. abuse_shield rejects blocked IPs at TS_VCONN_START_HOOK, and
a production fleet using its block action leaked sockets until the
hosts ran out of memory. A rejection from within the cert hook was
ignored outright and the handshake completed, while one from within the
ClientHello hook only worked because the certificate callback mistook
the error state for a client renegotiation attempt.
This patch makes the inbound SSL read path signal a read error when a
plugin has rejected the handshake, and makes the ClientHello and cert
callbacks fail the handshake explicitly in that state. Outbound
connections are left alone, since a TS_SSL_VERIFY_SERVER_HOOK error
under a PERMISSIVE policy must let the origin handshake finish.
A new autest rejects connections from each of these hooks, both from
within the hook and asynchronously, and checks that every connection
is closed. Another covers the permissive origin verification case over
TLS 1.2 without the session cache, and the abuse_shield connection rate
test now checks that blocked TLS connections are closed.
Co-authored-by: Claude Opus 5.5 Medium noreply@anthropic.com
🤖 Generated with Claude Code