HTTP/3: reject an unknown ALPN instead of aborting - #13631
Draft
brbzull0 wants to merge 1 commit into
Draft
Conversation
Http3SessionAccept::accept() ended its ALPN dispatch with
ink_abort("Negotiated App Name is unknown"), and an absent or empty ALPN
falls into that branch, so a completed QUIC handshake carrying no ALPN
extension took down traffic_server. 08d1896 removed the earlier
alpn.empty() arm that used to catch it.
Move the dispatch into a switch over a new select_app_type() helper and
return false for the unknown case, which mainEvent() already turns into
do_io_close(). Also initialise the out-pointer in
QUICNetVConnection::negotiated_application_name(), which quiche leaves
untouched when no protocol was negotiated.
Contributor
Author
|
[approve ci autest 2] |
Contributor
Author
|
[approve ci fedora] |
Contributor
Author
|
[approve ci freebsd] |
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.
Http3SessionAccept::accept()dispatched on the negotiated ALPN with anif/else-if chain ending in:
An absent or empty ALPN lands in that final arm, so a QUIC handshake that
completes without an ALPN extension aborts the process rather than closing the
connection.
08d1896d6a64("Reject TLS if client offers alpn with no match",#7981) removed the earlier
alpn.empty()arm that used to catch this casebefore it reached the abort.
That the empty state is expected elsewhere in the same path is visible in both
_start_application()implementations, which already have null-ALPN fallbacks.Change
Http3SessionAccept.h/.cc: add anAppTypeenum and a staticselect_app_type(std::string_view)helper, and turn the dispatch into aswitch. TheUNKNOWNarm logs and returnsfalse;Http3SessionAccept::mainEvent()already converts afalsereturn intonetvc->do_io_close(), so no new teardown path is introduced.QUICNetVConnection.cc:negotiated_application_name()declaredconst uint8_t *name;uninitialised and passed it toquiche_conn_application_proto(), which leaves it untouched when no protocolwas negotiated -- so the
std::string_viewwas constructed over anindeterminate pointer. Initialise to
nullptrand return an emptystring_viewin that case. This also makesalpn.data()non-null for the%.*sin the new log line.Extracting
select_app_type()keeps the tag-to-application mapping in one placeand makes it directly unit-testable, though no unit test is added here (see
below).
Test
Honest summary: no existing test fails without this change, and a targeted new
test is not portable.
An unrecognised tag such as
banananever reachesaccept()-- the TLS layeranswers
no_application_protocolfirst, which is whattests/gold_tests/tls/tls_bad_alpn.test.pyalready covers over TCP. The emptyALPN case only completes a handshake on a backend that does not enforce
RFC 9001 s8.1, so a gold test for it would pass or skip inconsistently across
BoringSSL, QUICTLS and the OpenSSL QUIC-TLS-callbacks shim. Rather than add a
backend-dependent test, this is submitted as hardening.
What was run, 9/9 pass:
h3_active_timeout,h3_flow_control,h3_go_client,h3_proxy_verifier,h3_python_client,h3_sni_check,h3_stream_lifetime,quic_no_activity_timeout,tls_bad_alpn. Three of those assert on the exactstart HTTP/3 app (ALPN=h3)debug line, so they confirm the switch conversiondid not disturb the happy path.
Built on both QUIC backends. Under
ENABLE_QUICHE=ON,src/iocore/net/QUICNetVConnection.cccompiles clean -- that file is only builtunder
elseif(TS_HAS_QUICHE)insrc/iocore/net/CMakeLists.txt, so theOpenSSL-QUIC build does not cover it. The quiche configuration was
compile-verified only; the autests above ran against the OpenSSL-QUIC build.