Skip to content

Refactor: Modernize asio TLS Stream#3764

Merged
reneme merged 4 commits intorandombit:masterfrom
Rohde-Schwarz:refactor/modernize_asio_stream
Oct 18, 2023
Merged

Refactor: Modernize asio TLS Stream#3764
reneme merged 4 commits intorandombit:masterfrom
Rohde-Schwarz:refactor/modernize_asio_stream

Conversation

@reneme
Copy link
Copy Markdown
Collaborator

@reneme reneme commented Oct 18, 2023

This updates the asio stream for C++20:

  • using boost-provided concepts instead of macros
  • preparing its async methods to be used with C++20 co-routines
  • reflects the need for at least boost 1.73.0 (released in April 2020) in the code base.

Additionally, it now allows applications to (optionally) override the TLS::Callbacks object. This wasn't possible before and therefore limited the customizability of the asio stream significantly.

Re: co-routines: Given a recent-enough toolchain and boost version, one can actually do:

   auto tcp_stream = /*  some boost-provided TCP stream wrapper */
   Botan::TLS::Stream<tcp_stream> tls_stream(std::move(tcp_stream), std::move(tls_ctx));
   co_await tls_stream.async_handshake(Botan::TLS::Connection_Side::Server);
   co_await tls_stream.write_some(...);

reneme and others added 4 commits October 18, 2023 08:59
Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>
This is the first version of boost asio to handle C++20 concepts
correctly.
Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>
Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>
@reneme reneme added the enhancement Enhancement or new feature label Oct 18, 2023
@reneme reneme requested a review from randombit October 18, 2023 08:00
@reneme reneme self-assigned this Oct 18, 2023
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 91.709% (+0.005%) from 91.704% when pulling f2ab1c9 on Rohde-Schwarz:refactor/modernize_asio_stream into 5657746 on randombit:master.


#include <boost/version.hpp>
#if BOOST_VERSION >= 106600
#if BOOST_VERSION >= 107300
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.

My main concern here is this increases the required Boost version (which is fine) but also it's hard for downstream users to know if the asio integration works for their botan+boost combination. They can test if it's available, using the usual feature macro - but they don't know if it will work if they include the header.

Maybe we should define somewhere (in an existing or new header) what our minimum boost version is, wrt this asio integration, which would allow applications to check for availability. (Or maybe you have some better idea of how to address this)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That's indeed a valid point. I'll make a proposal in a follow-up PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

lgtm modulo this thing with the versioning; doesn't have to be addressed here - it was always an issue and increasing the Boost version just makes the issue more immediate.

@reneme reneme merged commit 0981814 into randombit:master Oct 18, 2023
@reneme reneme deleted the refactor/modernize_asio_stream branch October 18, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Enhancement or new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants