Changes to reduce dependencies in TLS sources/headers by randombit · Pull Request #5347 · randombit/botan

randombit · 2026-02-16T12:25:56Z

No description provided.

Copilot

Pull request overview

Reduces compile-time dependencies in TLS sources/headers by replacing includes with forward declarations and moving type-dependent code into .cpp files.

Changes:

Switched several TLS headers from including x509cert.h / PKIX headers to forward declarations.
Moved/despecialized TLS session and TLS 1.3 certificate entry implementations to avoid requiring full X.509 type definitions in headers.
Updated TLS tests and some TLS implementation .cpp files to explicitly include botan/x509cert.h.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/tests/test_tls_session_manager.cpp	Adds explicit `x509cert` include required after header dependency reductions.
src/tests/test_tls_rfc8448.cpp	Adds explicit `x509cert` include required after header dependency reductions.
src/tests/test_tls.cpp	Adds explicit `x509cert` include required after header dependency reductions.
src/lib/tls/tls_signature_scheme.cpp	Removes an include to reduce dependencies.
src/lib/tls/tls_session.h	Replaces `x509cert` include with forward decls; moves special members out-of-line; changes constructor parameter types.
src/lib/tls/tls_session.cpp	Adds `x509cert` include and defines moved special members/constructors out-of-line.
src/lib/tls/tls_server.cpp	Adds explicit `x509cert` include required after header dependency reductions.
src/lib/tls/tls_handshake_transitions.h	Reorders/cleans up `vector` include placement.
src/lib/tls/tls_client.cpp	Adds explicit `x509cert` include required after header dependency reductions.
src/lib/tls/tls_callbacks.h	Replaces PKIX enums include with forward declarations and adds TODO about future dependency removal.
src/lib/tls/tls13/tls_server_impl_13.cpp	Adds explicit `x509cert` include required after header dependency reductions.
src/lib/tls/tls13/tls_messages_13.h	Removes `x509cert` include; reworks certificate storage to avoid requiring complete type in header.
src/lib/tls/tls13/tls_client_impl_13.cpp	Adds explicit `x509cert` include required after header dependency reductions.
src/lib/tls/tls13/tls_channel_impl_13.cpp	Adds explicit `x509cert` include required after header dependency reductions.
src/lib/tls/tls13/msg_certificate_13.cpp	Updates implementation for new certificate storage representation and defines special members out-of-line.
src/lib/tls/tls12/tls_handshake_state.h	Removes pubkey/pk_keys includes; adds forward decls.
src/lib/tls/tls12/msg_server_kex.cpp	Removes unused/avoidable includes to reduce dependencies.
src/lib/tls/tls12/msg_cert_verify_12.cpp	Removes pk_keys include to reduce dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/lib/tls/tls_session.h

src/lib/tls/tls_session.cpp

src/lib/tls/tls13/tls_messages_13.h

Copilot · 2026-02-16T12:32:39Z

src/lib/tls/tls13/tls_messages_13.h

+            /* This is always either empty or not; would have been std::optional but that requires
+               that the declaration be visible, which we are trying to avoid here
+            */
+            std::vector<X509_Certificate> m_certificate;


Using std::vector<X509_Certificate> as a 0/1 optional surrogate makes the invariant non-obvious and introduces indexing ([0]) in multiple places. This is harder to maintain and easier to misuse (e.g., accidentally pushing more than one cert). Consider a representation that encodes the intent more directly while still supporting incomplete types in the header, such as std::unique_ptr<X509_Certificate> (null vs non-null), and/or enforce the 0/1 invariant explicitly (e.g., assertions) where the value is constructed/modified.

I'm with Copilot here.

Isn't that taking it a little far? Maybe use a unique_ptr for such things so that the extra allocation becomes obvious at least. Also, this won't give the false impression of a 0..n relation.

Yeah I guess that works too

Switched to unique_ptr

coveralls · 2026-02-16T14:42:30Z

coverage: 90.029% (+0.001%) from 90.028%
when pulling f980244 on jack/tls-header-patrol-2
into 7b1bee2 on master.

reneme · 2026-02-16T16:17:40Z

src/lib/tls/tls13/tls_messages_13.h

+            /* This is always either empty or not; would have been std::optional but that requires
+               that the declaration be visible, which we are trying to avoid here
+            */
+            std::vector<X509_Certificate> m_certificate;


I'm with Copilot here.

Isn't that taking it a little far? Maybe use a unique_ptr for such things so that the extra allocation becomes obvious at least. Also, this won't give the false impression of a 0..n relation.

src/lib/tls/tls_callbacks.h

src/lib/tls/tls12/tls_record.h

KaganCanSit

I noticed a few things while checking. Although I'm not entirely sure, they can be checked and added to this branch.

I hope this helps.

src/lib/tls/tls_client.cpp

src/tests/test_tls_rfc8448.cpp

src/tests/test_tls.cpp

@@ -17,10 +17,13 @@
   #include <botan/tls_session.h>
   #include <botan/tls_signature_scheme.h>


randombit · 2026-02-16T21:03:50Z

Thanks @KaganCanSit

randombit requested review from Copilot and reneme February 16, 2026 12:25

Copilot started reviewing on behalf of randombit February 16, 2026 12:30 View session

Copilot AI reviewed Feb 16, 2026

View reviewed changes

randombit force-pushed the jack/tls-header-patrol-2 branch 7 times, most recently from 281ca5d to 59012ac Compare February 16, 2026 13:56

reneme reviewed Feb 16, 2026

View reviewed changes

reneme mentioned this pull request Feb 16, 2026

Refactor: Organize TLS Extensions into TLS 1.2 and 1.3 Modules #5303

Merged

randombit force-pushed the jack/tls-header-patrol-2 branch 2 times, most recently from 92ef4e9 to ec96b52 Compare February 16, 2026 17:07

KaganCanSit reviewed Feb 16, 2026

View reviewed changes

src/lib/tls/tls12/tls_record.h Outdated

This comment was marked as resolved.

Sign in to view

KaganCanSit reviewed Feb 16, 2026

View reviewed changes

src/lib/tls/tls_client.cpp Show resolved Hide resolved

src/tests/test_tls_rfc8448.cpp Show resolved Hide resolved

src/tests/test_tls.cpp

@@ -17,10 +17,13 @@

#include <botan/tls_session.h>

#include <botan/tls_signature_scheme.h>

This comment was marked as resolved.

Sign in to view

randombit force-pushed the jack/tls-header-patrol-2 branch from ec96b52 to 9b32fa2 Compare February 16, 2026 21:04

reneme approved these changes Feb 16, 2026

View reviewed changes

Changes to reduce dependencies in TLS sources/headers

f980244

randombit force-pushed the jack/tls-header-patrol-2 branch from 9b32fa2 to f980244 Compare February 17, 2026 07:23

randombit merged commit 481736b into master Feb 17, 2026
46 checks passed

randombit deleted the jack/tls-header-patrol-2 branch February 17, 2026 10:00

		@@ -17,10 +17,13 @@
		#include <botan/tls_session.h>
		#include <botan/tls_signature_scheme.h>

Conversation

randombit commented Feb 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

reneme Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

randombit Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

randombit Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

coveralls commented Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reneme Feb 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

KaganCanSit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

randombit commented Feb 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Feb 16, 2026 •

edited

Loading