Skip to content

[TLS 1.3] Refactor: Prepare for TLS 1.3 Server#3062

Merged
reneme merged 16 commits intomasterfrom
refactor/prepare_for_tls13_server
Dec 15, 2022
Merged

[TLS 1.3] Refactor: Prepare for TLS 1.3 Server#3062
reneme merged 16 commits intomasterfrom
refactor/prepare_for_tls13_server

Conversation

@reneme
Copy link
Copy Markdown
Collaborator

@reneme reneme commented Oct 21, 2022

Pull Request Dependencies

This is based on #3064

Description

This applies preparatory refactorings to (hopefully) make the initial pull request towards a TLS 1.3 server easier to review.

Overview of Changes

Client Hello Marshalling

The wire-format of the client hello message did not change between TLS 1.2 and 1.3. I.e. we can reuse the parsing and serialization code. Similarly to the Server_Hello (already merged in TLS 1.3 client) this introduces a "dumb" Client_Hello_Internal class that encapsulates the marshalling.

Client_Hello_13 gains a ::parse() factory method that decides whether it deals with a 1.2 or 1.3 client hello and returns the appropriate type in a std::variant<>. The constructor of Client_Hello_13 already contains some of the local sanity checks required in RFC 8446.

Removal of the Server_Impl Interface

This interface class was introduced way back by the Elektrobit folks. It turned out that it is not needed as all its defined methods are either TLS 1.2-specific or better defined in the generic Channel_Impl class. Similarly, we removed this Client_Impl counter-part during the TLS 1.3 client implementation.

application_protocol() vs. next_protocol()

To the best of my understanding, they have the same semantics. Hence, I removed the next_protocol() method from the lower-level code. The user-facing method (in TLS::Server) is now just a delegate to application_protocol() and is deprecated. Or should we remove it right away, @randombit?

Policy::choose_key_exchange_group()

This callback now has an additional parameter allowing applications to take the peer's offered groups into account. This way an application can define when to fall back to a Hello Retry Request even when mutually acceptable groups were already offered. One potential use case: A client might advertise support for a PQC exchange group but offered standard ECDH only. With this addition, the server could enforce PQ crypto at its own discretion.

Signature_Algorithms and Signature_Algorithms_Cert extensions

While we don't fully honor the "signature_algorithms_cert" semantics just yet, we must be able to parse it. Both extensions share the same schema, hence this tries to unify their marshalling code better. See also #2990.

Cipher State allows to receive data before ingesting the client Finished message

The server may start sending records encrypted under its "application traffic secret" even before receiving the client's Finished message. The Cipher_State must be able to handle this by deriving the server's traffic secrets ahead, while deferring the derivation of the client's secrets until after receiving the client's Finished. This was found via BoGo tests that expect to exchange a protected Alert record in this meta-situation.

Server_Name_Indication can be rendered by the Server

According to RFC 6066, the server may send an empty SNI extension as acknowledgement:

Servers that receive a client hello containing the "trusted_ca_keys"
extension MAY use the information contained in the extension to guide
their selection of an appropriate certificate chain to return to the
client. In this event, the server SHALL include an extension of type
"trusted_ca_keys" in the (extended) server hello. The
"extension_data" field of this extension SHALL be empty.

generalize_to<> Helper

This is a helper function that allows transforming a specific std::variant<> type (or individual concrete type) into a super-set. E.g.

using Pet = std::variant<Cat, Dog>;
using Animal = std::variant<Cat, Dog, Bat, Spider, Snake>;
Pet pet = Cat();
Cat cat;
auto animal = generalize_to<Animal>(pet);
auto other_animal = generalize_to<Animal>(cat);

Protocol Version Downgrade

Downgrading can now be explicitly requested by the concrete TLS client/server implementation class. Before, the Channel took a downgrade decision simply by reacting to a Server_Hello_12. This is not possible anymore, because TLS 1.3 servers must be able to downgrade even when receiving a Client_Hello_13 (that explicitly negotiates TLS 1.2 in its "supported_versions" extension).

tls_stream_integration test

This is an integration test of the TLS asio adapter that sets up a test client and test server (as asio streams) and lets them talk to each other via a local TCP connection. We found some races and roadblocks when trying to extend this test with additional configurations using the TLS 1.3 server. This restructures the test's internal steering logic to better handle timeouts. Also, it adds some machinery to allow running the internal test cases with various TLS policies.

@reneme reneme added this to the Botan 3.0.0 milestone Oct 21, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 21, 2022

Codecov Report

Base: 87.91% // Head: 87.95% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (54e9d3f) compared to base (08c54f5).
Patch coverage: 86.59% of modified lines in pull request are covered.

❗ Current head 54e9d3f differs from pull request most recent head 007c725. Consider uploading reports for the commit 007c725 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3062      +/-   ##
==========================================
+ Coverage   87.91%   87.95%   +0.04%     
==========================================
  Files         599      599              
  Lines       66039    66177     +138     
  Branches     6609     6615       +6     
==========================================
+ Hits        58059    58207     +148     
+ Misses       5190     5181       -9     
+ Partials     2790     2789       -1     
Impacted Files Coverage Δ
src/bogo_shim/bogo_shim.cpp 73.52% <ø> (+0.31%) ⬆️
src/lib/tls/msg_server_hello.cpp 81.30% <ø> (-0.09%) ⬇️
src/lib/tls/tls12/tls_server_impl_12.cpp 79.25% <ø> (+0.39%) ⬆️
src/lib/tls/tls13/tls_handshake_state_13.cpp 90.90% <0.00%> (-9.10%) ⬇️
src/lib/tls/tls_client.cpp 83.92% <33.33%> (-6.08%) ⬇️
src/lib/tls/tls_server.cpp 64.10% <50.00%> (-3.47%) ⬇️
src/lib/tls/tls13/tls_cipher_state.cpp 94.47% <66.66%> (-1.51%) ⬇️
src/lib/tls/tls13/tls_channel_impl_13.cpp 88.96% <80.00%> (+0.42%) ⬆️
src/lib/tls/msg_client_hello.cpp 82.97% <86.14%> (+1.84%) ⬆️
src/lib/tls/tls_extensions.cpp 81.64% <87.50%> (+0.45%) ⬆️
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@reneme reneme force-pushed the refactor/prepare_for_tls13_server branch from 9ad1a98 to e9a3629 Compare October 21, 2022 11:53
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Oct 21, 2022

This pull request introduces 1 alert when merging e9a3629 into 76089a3 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 3, 2022

This pull request introduces 1 alert when merging e1d5c2c into 76089a3 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@reneme reneme force-pushed the refactor/prepare_for_tls13_server branch from e1d5c2c to e65f258 Compare November 4, 2022 09:31
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 4, 2022

This pull request introduces 1 alert when merging e65f258 into 76089a3 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@reneme reneme force-pushed the refactor/prepare_for_tls13_server branch from e65f258 to a65c800 Compare November 4, 2022 16:54
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 4, 2022

This pull request introduces 1 alert when merging a65c800 into 76089a3 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@reneme reneme force-pushed the refactor/prepare_for_tls13_server branch from a65c800 to bdb0b8b Compare November 10, 2022 12:03
@lgtm-com
Copy link
Copy Markdown

lgtm-com bot commented Nov 10, 2022

This pull request introduces 1 alert when merging bdb0b8b into 76089a3 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@reneme reneme force-pushed the refactor/prepare_for_tls13_server branch 3 times, most recently from d99d5b0 to 8b21714 Compare November 17, 2022 14:45
lieser
lieser previously requested changes Nov 18, 2022
catch(const std::exception& ex)
{ m_result.test_failure(std::string("sync client failed with: ") + ex.what()); }
catch(const boost::exception&)
{ m_result.test_failure("sync client failed with boost exception"); }
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not record information about the boost exception like it is done for the std one?

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.

Frankly: Because I couldn't figure out how to get the error message from boost extensions. O.o

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There seems to exist a diagnostic_information function. But would be ok for me to also leave as is

@reneme reneme force-pushed the refactor/prepare_for_tls13_server branch from fadfc21 to 185654f Compare December 15, 2022 07:44
Under load (and after adding additional test configurations for TLS 1.3)
the test started to be flaky, failing or hanging randomly.

This reworks the timeouts and steering of the test client/server
to repair those issues. It also made the test a lot faster.

The main culprit before the rework: if the server ran into a
timeout, the io_context was interrupted and not restarted. This
caused a synchronous client in a different thread to sometimes
deadlock or misbehave randomly.
@reneme reneme force-pushed the refactor/prepare_for_tls13_server branch from 185654f to ed37308 Compare December 15, 2022 09:11
@reneme reneme force-pushed the refactor/prepare_for_tls13_server branch from ed37308 to 54e9d3f Compare December 15, 2022 09:54
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.

Sorry for the long delay on this review. I did not review the tests very carefully but the library changes look good. I left a couple of small questions in the review, but approving to unblock this.

Copy link
Copy Markdown
Collaborator

@lieser lieser left a comment

Choose a reason for hiding this comment

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

My comments seems to be addressed, did not do a full re-review.

@lieser lieser dismissed their stale review December 15, 2022 11:31

My comments seems to be addressed, but did not do a full re-review.

@reneme reneme merged commit 788b576 into master Dec 15, 2022
@randombit randombit deleted the refactor/prepare_for_tls13_server branch December 18, 2022 20:28
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.

4 participants