Refactor: Disentangle Server_Hello implementations#5292
Refactor: Disentangle Server_Hello implementations#5292reneme merged 1 commit intorandombit:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors TLS ServerHello handling by moving the concrete Server_Hello_12 and Server_Hello_13 implementations into their respective tls12/tls13 modules, while keeping a minimal TLS 1.2 “shim” type available for the TLS 1.3 stack to detect and process protocol downgrades.
Changes:
- Introduce
Server_Hello_12_Shim(minimal TLS 1.2 ServerHello interface) in the common TLS messages header and update the TLS 1.3 handshake message variants to use it for downgrade detection. - Extract
Server_Hello_Internalinto a dedicated internal header and keep version-agnostic parsing centralized (now used from module-specific message code). - Move the TLS 1.2
Server_Hello_12implementation intotls12/and TLS 1.3Server_Hello_13/ HRR logic intotls13/, updating tests accordingly.
Detailed report: logic/behavior changes (beyond “just moved”)
- Type/API surface change for TLS 1.3 downgrade path: TLS 1.3 parsing now yields
Server_Hello_12_Shim(instead of the fullServer_Hello_12) when a pre–TLS 1.3 ServerHello is detected. This intentionally restricts what the TLS 1.3 code can access (only version selection + downgrade signaling), reducing coupling between TLS 1.3 and TLS 1.2 message features. - Method used in tests changed: tests that previously compared
pv == msg.legacy_version()now comparepv == msg.selected_version()for the shim case (equivalent for the shim, but matches the new public surface). - Constants relocation: downgrade markers and the HelloRetryRequest marker moved into
tls_magic.has shared constants instead of being local tomsg_server_hello.cpp. (No intended semantic change, but it changes include/visibility requirements.) - ServerHello-internal parsing implementation detail: HRR detection now uses
CT::is_equal<uint8_t>(m_random, HELLO_RETRY_REQUEST_MARKER)directly rather than a helper that compared raw pointers. This should be functionally equivalent.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/test_tls_messages.cpp | Adjust TLS 1.3 ServerHello parsing tests to expect Server_Hello_12_Shim and use selected_version() |
| src/tests/test_tls_handshake_layer_13.cpp | Update handshake-layer test to expect Server_Hello_12_Shim |
| src/lib/tls/tls_messages_internal.h | New internal header for Server_Hello_Internal container |
| src/lib/tls/tls_messages.h | Replace public Server_Hello_12 with Server_Hello_12_Shim and narrow exposed interface |
| src/lib/tls/tls_magic.h | Add shared downgrade/HRR marker constants |
| src/lib/tls/tls13/tls_messages_13.h | Update TLS 1.3 message variants and parse() return type to use Server_Hello_12_Shim |
| src/lib/tls/tls13/tls_handshake_state_13.h / .cpp | Store downgrade ServerHello as Server_Hello_12_Shim |
| src/lib/tls/tls13/tls_client_impl_13.h / .cpp | Handle downgrade ServerHello via Server_Hello_12_Shim |
| src/lib/tls/tls13/msg_server_hello_13.cpp | New TLS 1.3 ServerHello/HRR implementation file |
| src/lib/tls/tls12/tls_messages_12.h | Reintroduce full TLS 1.2 Server_Hello_12 in tls12 module, inheriting from the shim |
| src/lib/tls/tls12/msg_server_hello_12.cpp | New TLS 1.2 ServerHello implementation file |
| src/lib/tls/msg_server_hello.cpp | Centralize Server_Hello_Internal parsing + implement shim methods; remove module-specific code |
| src/lib/tls/info.txt | Register tls_messages_internal.h as internal header |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
27b639d to
f6d323c
Compare
|
This should be ready for prime time, but it will likely conflict with #5296. I would suggest merging the header patrol first, as this mostly moves stuff around anyway, it should be fairly easy to resolve conflicts on a rebase. |
👍 I'll merge 5296 now and then try to avoid messing with TLS futher until these can land |
This moves the implementation of Server_Hello_12 into the tls12 module and Server_Hello_13 to tls13 likewise. To support downgrading the protocol version, the TLS 1.3 implementation needs to have a rudimentary understanding of the TLS 1.2 Server Hello. This is now implemented by the Server_Hello_12_Shim base class.
f6d323c to
a8ca18f
Compare
|
@randombit The conflicts caused by #5296 were quite minimal. This is now rebased and ready for review. As stated above, I made sure to leave actual semantic code changes to a minimum and just moved stuff around into the respective submodules. |
See here for an overview of the related pull requests to disentangle TLS 1.2 and 1.3.
This moves the implementation of
Server_Hello_12into the tls12 module andServer_Hello_13to tls13 likewise. To support downgrading the protocol version, the TLS 1.3 implementation needs to have a rudimentary understanding of the TLS 1.2 Server Hello. This is now implemented by theServer_Hello_12_Shimbase class.@copilot Most of this pull request is just moving code around without changing the implementations of the individual methods. Sometimes, small adjustments had to be done when certain aspects needed a different scope. Please create a detailed report on implementations and logic that didn't just move to make it easier for a human reviewer to follow what actually changes in this PR.