[TLS 1.3] Refactor: Prepare for TLS 1.3 Server#3062
Conversation
Codecov ReportBase: 87.91% // Head: 87.95% // Increases project coverage by
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
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. |
9ad1a98 to
e9a3629
Compare
|
This pull request introduces 1 alert when merging e9a3629 into 76089a3 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging e1d5c2c into 76089a3 - view on LGTM.com new alerts:
|
e1d5c2c to
e65f258
Compare
|
This pull request introduces 1 alert when merging e65f258 into 76089a3 - view on LGTM.com new alerts:
|
e65f258 to
a65c800
Compare
|
This pull request introduces 1 alert when merging a65c800 into 76089a3 - view on LGTM.com new alerts:
|
a65c800 to
bdb0b8b
Compare
|
This pull request introduces 1 alert when merging bdb0b8b into 76089a3 - view on LGTM.com new alerts:
|
d99d5b0 to
8b21714
Compare
| 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"); } |
There was a problem hiding this comment.
Why not record information about the boost exception like it is done for the std one?
There was a problem hiding this comment.
Frankly: Because I couldn't figure out how to get the error message from boost extensions. O.o
There was a problem hiding this comment.
There seems to exist a diagnostic_information function. But would be ok for me to also leave as is
1d134f0 to
fadfc21
Compare
fadfc21 to
185654f
Compare
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.
185654f to
ed37308
Compare
ed37308 to
54e9d3f
Compare
randombit
left a comment
There was a problem hiding this comment.
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.
lieser
left a comment
There was a problem hiding this comment.
My comments seems to be addressed, did not do a full re-review.
My comments seems to be addressed, but did not do a full re-review.
Pull Request Dependencies
This is based on #3064Description
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_Internalclass that encapsulates the marshalling.Client_Hello_13gains a::parse()factory method that decides whether it deals with a 1.2 or 1.3 client hello and returns the appropriate type in astd::variant<>. The constructor ofClient_Hello_13already contains some of the local sanity checks required in RFC 8446.Removal of the
Server_ImplInterfaceThis 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_Implclass. Similarly, we removed thisClient_Implcounter-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 (inTLS::Server) is now just a delegate toapplication_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_AlgorithmsandSignature_Algorithms_CertextensionsWhile 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_Statemust 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_Indicationcan be rendered by the ServerAccording to RFC 6066, the server may send an empty SNI extension as acknowledgement:
generalize_to<>HelperThis is a helper function that allows transforming a specific
std::variant<>type (or individual concrete type) into a super-set. E.g.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 aClient_Hello_13(that explicitly negotiates TLS 1.2 in its "supported_versions" extension).tls_stream_integrationtestThis 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.