-
Notifications
You must be signed in to change notification settings - Fork 582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce Example: Minimal HTTPS client #3799
Reduce Example: Minimal HTTPS client #3799
Conversation
Mhh, the boost version on CI doesn't seem to be recent enough for this example. I'll look into that later. |
src/lib/tls/asio/asio_context.h
Outdated
* @param server_info Basic information about the host to connect to (SNI) | ||
*/ | ||
Context(Server_Information server_info = Server_Information()) : | ||
m_credentials_manager(std::make_shared<Default_Credentials_Manager>()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Default_Credentials_Manager
seems odd to me tbh. If the system certs are not available, it seems like nothing will work anyway. Why not just require the system certs for the example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the Credentials_Manager
base class is mostly an interface without useful default implementations. Therefore, users are forced to provide their own Credentials_Manager
subclass, even if they just want to use system certificates, anyway.
In my opinion that adds quite a bit of friction. Especially, for people that want to try the ASIO stream as replacement for the OpenSSL stream shipping with Boost. Hence, this is an attempt to have a functional default for the TLS::Context
object with minimal parameters.
Admittedly, the Default_Credentials_Manager
was just the shortest path to get this. Perhaps, it could be more generic and (optionally) accept user-provided roots. If no root is provided, it could default to the system certs (when available). Like so:
/**
* @param server_info the hostname you're trying to connect to
* @param root_certs the root certificates you're willing to trust
* (defaults to the operating system's certificates)
*/
Context(Server_Information server_info = Server_Information(),
std::vector<X509_Certificate> root_certs = {}) :
m_credentials_manager(std::make_shared<Default_Credentials_Manager>(std::move(root_certs))),
[...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I rebased this onto #3901 which adds the very same example but without the convenience additions in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the Default_Credentials_Manager
out of the public interface and demoted it to an implementation detail. See here: #3799 (comment).
79b76c6
to
618024a
Compare
618024a
to
7a9833e
Compare
7a9833e
to
c076df7
Compare
... to allow creating a simplistic asio stream based client with minimal Botan-related boilerplate.
c076df7
to
63de52d
Compare
Let me dig this up again. The actual example that was initially part of this PR is already merged in #3901. This now just reduces this example by introducing a convenience constructor for the Before, the |
Description
This reduces the example introduced in #3901 by introducing some convenient defaults in the ASIO stream. I think it is making a relevant point, that one can write a basic HTTPS client using Botan's TLS in just a few lines of C++20 that works out of the box.
In detail, I added:
Default_Credentials_Manager
that usesSystem_Certificate_Store
as the default trust store (if available)The existing example had to provide this overload (
System_Credentials_Manager
). If no interface to the system's certificate store is implemented on the target system, the client won't be able to establish trust. We need to document this well enough.TLS::Stream<>
that uses defaults for all aspects of the stream'sContext
object, namely:AutoSeeded_RNG
Default_Credentials_Manager
TLS::Session_Manager_In_Memory
andTLS::Default_Policy