Repository navigation
Refactor: Opportunistically disable TLS downgrade support entirely - #5812
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors TLS 1.3 ↔ TLS 1.2 downgrade mechanics so they are compiled only when both TLS 1.2 and TLS 1.3 are available, and otherwise fully removed from the build. This aligns with the goal of reducing dead/unused downgrade code paths in “TLS12-only” or “TLS13-only” builds.
Changes:
- Introduces
BOTAN_HAS_TLS_DOWNGRADE_SUPPORT(defined when bothBOTAN_HAS_TLS_12andBOTAN_HAS_TLS_13are set) and compiles downgrade-related APIs/logic only under that macro. - Updates TLS 1.3 client/server downgrade handling to hard-fail with
ProtocolVersionwhen downgrade support is not compiled in. - Updates bogo shim error mapping to match new error strings.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/tls/tls13/tls_server_impl_13.h | Guards TLS 1.3 server downgrade API behind BOTAN_HAS_TLS_DOWNGRADE_SUPPORT. |
| src/lib/tls/tls13/tls_server_impl_13.cpp | Guards downgrade implementation and adds “unsupported Client Hello” failure when downgrade support is disabled. |
| src/lib/tls/tls13/tls_client_impl_13.cpp | Guards downgrade/resumption downgrade paths and adds “unsupported Server Hello” failure when downgrade support is disabled. |
| src/lib/tls/tls13/tls_channel_impl_13.h | Guards expect_downgrade() declaration behind downgrade-support macro. |
| src/lib/tls/tls13/tls_channel_impl_13.cpp | Guards transcript/downgrade logic and tweaks alert handling for nonfatal error alerts. |
| src/lib/tls/tls12/tls_server_impl_12.h | Guards downgrade ctor behind downgrade-support macro. |
| src/lib/tls/tls12/tls_server_impl_12.cpp | Guards downgrade ctor implementation behind downgrade-support macro. |
| src/lib/tls/tls12/tls_client_impl_12.h | Guards downgrade ctor behind downgrade-support macro. |
| src/lib/tls/tls12/tls_client_impl_12.cpp | Guards downgrade ctor implementation behind downgrade-support macro. |
| src/lib/tls/tls_server.cpp | Uses downgrade-support macro for downgrade path and IO buffer sizing. |
| src/lib/tls/tls_client.h | Guards Client::downgrade() declaration behind downgrade-support macro. |
| src/lib/tls/tls_client.cpp | Guards downgrade implementation and downgrade triggers behind downgrade-support macro. |
| src/lib/tls/tls_channel.h | Defines BOTAN_HAS_TLS_DOWNGRADE_SUPPORT when TLS 1.2 and TLS 1.3 are both enabled. |
| src/lib/tls/tls_channel_impl.h | Makes downgrade info accessors conditional and adjusts downgrade state query APIs. |
| src/bogo_shim/bogo_shim.cpp | Adds mappings for the new “unsupported Client/Server Hello” error messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
921f1c5 to
7e1de32
Compare
Lots of the downgrade mechanics were compiled into the binary even when either TLS 1.2 or TLS 1.3 was disabled at configure- time. Now, it is only enabled if the compiled library contains support for both protocol versions, otherwise it is fully removed.
7e1de32 to
e01aaa2
Compare
randombit
left a comment
There was a problem hiding this comment.
Definitely like the concept, not such a big fan of the impl.
Like you can look at this
#if defined(BOTAN_HAS_TLS_DOWNGRADE_SUPPORT)
if(expects_downgrade()) {
preserve_peer_transcript(data);
}
#endif
and wonder - should there be an #else block that asserts !expects_downgrade(). But then a further macro mess...
Feels like a lot of this could be done with a constexpr bool Tls13NeedsDowngradeSupport (which itself would need of course a bit of preprocessor since we don't expose feature as constexpr at the moment..) plus if constexpr blocks, to the same effect. OK to merge as is, if you did the constexpr rework I'd like it more, otherwise I'll look at that part next week.
|
Regarding the Let's maybe revisit this after the next PR. |
|
OK we can wait for the full sequence and then consider the possibilities. Fine to merge. |
Add PKCS12::mac_protected to indicate if a MAC was used or not
Lots of the downgrade mechanics were compiled into the binary even when either TLS 1.2 or TLS 1.3 was disabled at configure- time. Now, it is only enabled if the compiled library contains support for both protocol versions, otherwise it is fully removed.
This is essentially an extension of #5318 and preparatory work for #5000.