Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Nov 4, 2023

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 uses System_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.
  • a convenience constructor for TLS::Stream<> that uses defaults for all aspects of the stream's Context object, namely:
    • AutoSeeded_RNG
    • Default_Credentials_Manager
    • TLS::Session_Manager_In_Memory and
    • TLS::Default_Policy

@reneme reneme requested a review from randombit November 4, 2023 17:11
@reneme reneme self-assigned this Nov 4, 2023
@coveralls
Copy link

coveralls commented Nov 4, 2023

Coverage Status

coverage: 92.004% (-0.02%) from 92.028%
when pulling 63de52d on Rohde-Schwarz:example/minimal_https_client
into bf86f1a on randombit:master.

@reneme
Copy link
Collaborator Author

reneme commented Nov 4, 2023

Mhh, the boost version on CI doesn't seem to be recent enough for this example. I'll look into that later.

* @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>()),
Copy link
Owner

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?

Copy link
Collaborator Author

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))),
    [...]

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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).

@reneme reneme force-pushed the example/minimal_https_client branch 4 times, most recently from 79b76c6 to 618024a Compare February 2, 2024 16:31
@reneme reneme force-pushed the example/minimal_https_client branch from 618024a to 7a9833e Compare April 23, 2024 09:00
@reneme reneme changed the title New Example: Minimal HTTPS client Reduce Example: Minimal HTTPS client Apr 23, 2024
@reneme reneme force-pushed the example/minimal_https_client branch from 7a9833e to c076df7 Compare April 23, 2024 11:39
... to allow creating a simplistic asio stream based client with
minimal Botan-related boilerplate.
@reneme reneme force-pushed the example/minimal_https_client branch from c076df7 to 63de52d Compare April 23, 2024 12:17
@reneme
Copy link
Collaborator Author

reneme commented Apr 23, 2024

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 TLS::Context class used by the ASIO stream. It allows users to get started with a simplistic HTTPS client application with minimal Botan-related boilerplate. (The gist of it now fits on a slide, for instance. Which would come in handy for our contribution to the "BSI IT-Sicherheitskongress" in May 😅.)

Before, the Default_Credentials_Manager was in a public header. But given its slim utility, I degraded it to an implementation detail of said convenience constructor. In the future we could extend this with a similar convenience constructor for a server use case (i.e. just take a cert and a public key).

@reneme reneme requested a review from randombit April 23, 2024 13:13
@reneme reneme marked this pull request as ready for review May 22, 2024 14:19
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.

None yet

3 participants