Skip to content

Refactor: Opportunistically disable TLS downgrade support entirely - #5812

Merged
reneme merged 1 commit into
randombit:masterfrom
Rohde-Schwarz:refactor/disable_tls_downgrade
Aug 7, 2026
Merged

reneme merged 1 commit into
randombit:masterfrom
Rohde-Schwarz:refactor/disable_tls_downgrade

Conversation

@reneme

@reneme reneme commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

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.

@reneme reneme self-assigned this Aug 7, 2026
Copilot AI lite review requested due to automatic review settings August 7, 2026 10:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 both BOTAN_HAS_TLS_12 and BOTAN_HAS_TLS_13 are set) and compiles downgrade-related APIs/logic only under that macro.
  • Updates TLS 1.3 client/server downgrade handling to hard-fail with ProtocolVersion when 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.

Comment thread src/lib/tls/tls13/tls_channel_impl_13.cpp Outdated
Comment thread src/lib/tls/tls_channel_impl.h Outdated
Comment thread src/lib/tls/tls13/tls_client_impl_13.cpp Outdated
@reneme
reneme force-pushed the refactor/disable_tls_downgrade branch from 921f1c5 to 7e1de32 Compare August 7, 2026 11:05
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.
@reneme
reneme force-pushed the refactor/disable_tls_downgrade branch from 7e1de32 to e01aaa2 Compare August 7, 2026 11:10
@reneme
reneme requested a review from randombit August 7, 2026 11:24

@randombit randombit left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@reneme

reneme commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator Author

Regarding the if constexpr suggestion: The next PR will adapt the Downgrade_Information struct to actually hold the Client_Hello_13 object created by a to-be-downgraded TLS 1.3 implementation. This type won't be available if TLS 1.3 is disabled at configure time. That problem sparked the whole exercise...

Let's maybe revisit this after the next PR.

@randombit

Copy link
Copy Markdown
Owner

OK we can wait for the full sequence and then consider the possibilities. Fine to merge.

@reneme
reneme merged commit 0a3d083 into randombit:master Aug 7, 2026
47 checks passed
@reneme
reneme deleted the refactor/disable_tls_downgrade branch August 7, 2026 12:29
shamkhaltyb79-spec referenced this pull request Sep 4, 2026
Add PKCS12::mac_protected to indicate if a MAC was used or not
@reneme reneme mentioned this pull request Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants