[TLS 1.3] Limited TLS 1.3 Client Implementation

[hrantzsch]

Limited TLS 1.3 Client

Pull Request Dependencies

• The RFC 8448 based test uses the Test::Result consolidation constructor from this pull request. For simplicity the required commit is duplicated here and should be removed after the consolidation constructor is finalized and merged.

Follow-up Pull Requests

The number of open pull requests that are based on this are getting somewhat out of hand. Below is a list to keep track of the dependencies:

• THIS ONE: [TLS 1.3] Limited TLS 1.3 Client Implementation #2922
  • [TLS 1.3] Client Authentication in the main Handshake #2957
  • [TLS 1.3] Session Resumption #2974
  • [TLS 1.3] Fuzz Target for Handshake Message Parsing #2977
  • [TLS 1.3] Post-Quantum Readiness via Hybrid Key Exchange #2983
  • [TLS 1.3] Overhaul TLS::Callbacks #2988
  • [TLS 1.3] Overhaul TLS::Policy #2989
  • INFORMATIVE: [TLS 1.3] Full Integration #2981
  • INFORMATIVE: Post-Quantum TLS experiments based on KEMTLS #2954

Potential Future Work

This is a non-exhaustive list of potential improvements or further development. Note that those points are not tackled by the follow-up PRs. I'll leave this here for future reference.

• Overhaul of TLS_Data_Reader
  • Some of its methods leave room for performance improvements
  • The interface could be easier to use
  • More invariant checks (e.g. assert !has_remaining() on destruction).
  • Also a "sub-reader" concept: i.e. a new ::get_tls_length_value_as_reader() could return a sub-reader that can then be passed into a sub-parser (e.g. Client Hello parsing its extensions)
  • Use it consistently for parsing
• Allow disabling TLS 1.2 at compile time
• std::span to avoid copying
  • C++17 doesn't actually provide it, but a custom implementation should be straight forward
• Separate TLS 1.2 and 1.3 cipher suites
  • TLS 1.3 suites are more focussed
    (some protocol parameters are negotiated differently)
  • the Ciphersuite class provides methods that are strictly not defined for TLS 1.3 (potential cause for bugs)
  • cipher suite handling code becomes more convoluted that necessary
    (e.g. Policy::ciphersuite_list(), generation via tls_suite_info.py)

This PR adds limited TLS 1.3 support. Available functionality:

• functional TLS 1.3 client
  • automatically downgrades to TLS 1.2 if allowed and required by the server
  • usable via Botan-CLI tls_client
• additional extensions as required per RFC 8446:
  • Key_Share
  • Supported_Versions
  • Cookie
  • Signature Algorithms
  • Signature Algorithms Certificate
  • Negotiated Groups
• additional extensions (not strictly required by RFC 8446)
  • Certificate Status Request (OCSP stapling)
  • Record_Size_Limit (RFC 8449)
  • Application Layer Protocol Notification (RFC 7301)
  • Server Name Indication
• Utility and Misc
  • ./botan tls_client --debug to print raw TLS traffic
  • Fixed_Output_RNG can optionally fall back to another RNG when its pool is empty
• Tests integrated or performed
  • New infrastructure has unit tests
  • Integration tests using the test vectors from RFC 8448
  • BoGo tests that do not depend on not-yet-implemented protocol features
    (see here for a list of issues found by BoGo)
  • Handshake and data exchange works using ./botan tls_client with the big players (google.com, cloudflare.com, ...)

Limitations:

• TLS 1.3 support cannot be compiled without enabling TLS 1.2
  (TLS 1.2 can be disabled in the TLS policy though)
• no TLS 1.3 compatible server
• no client authentication (added in: [TLS 1.3] Client Authentication in the main Handshake #2957)
• no session resumption via Tickets (added in: [TLS 1.3] Session Resumption #2974)
• no Pre-Shared-Keys (technical foundation added in: [TLS 1.3] Session Resumption #2974)
• no 0-RTT support (early data) (technical foundation added in: [TLS 1.3] Session Resumption #2974)
• no DTLS
• no fuzzing of the TLS 1.3 implementation (basic fuzz target for parser: [TLS 1.3] Fuzz Target for Handshake Message Parsing #2977)

Implementation Overview

To make this large PR a bit more approachable, we'll provide an overview to the newly introduced components and their responsibilities. Note that this work-in-progress pull request strives towards a usable implementation of "Minimal Viable Product" as outlined in the ToDo-List of this issue comment.

TODO

• server certificate validation
  • OCSP stapling
• Hello Retry Request
• handle CLOSE_NOTIFY properly (difference between TLS 1.2 and 1.3)
• protocol version downgrade
• Key Update
• Key Material Export (RFC 8446 7.5 / RFC 5705)
• review TLS callbacks invocations
• bogo test suite integration
• check occurrences of extensions as indicated in the table in RFC 8446 4.2
• configure ClientHello settings (e.g. ALPN)
• honor the record size limit extension
• Session Resumption (PSK w/o 0-RTT) not part of this PR

Intro

The descriptions mostly refer to the components in the tls13 module, as the TLS 1.2 implementation remains unchanged as far as possible. As initiated in the refactoring by Elektrobit Automotive GmbH, the user-facing interface of TLS::Client and TLS::Server also remains unchanged by means of a Pimpl-pattern.

We nevertheless anticipate some API changes in the public API. Please refer to this issue comment for further details.

Components

https://excalidraw.com/#json=uJjsg_me6QrHsybCbkgxs,Ql21IMba1ZERgxfIiFElKg

Channel

The Channel acts as "composition root" of the entire TLS 1.3 implementation. It implements the library's public APIs via a Pimpl-construction as detailed in this issue comment.
This class implements the client/server agnostic parts of the protocol. Hence, orchestrating the Record_Layer and Handshake_Layer to transform received bytes from the network into handshake messages to be interpreted by the Client (and later Server) implementations or application data to be passed to the using code. Furthermore, it takes care of handling TLS alerts.

Record_Layer

This layer implements the record layer level of the protocol. It parses raw data from the wire to individual records and decrypts protected records using Cipher_State. This corresponds to tasks performed by the Channel (Channel_Impl_12) and the Record_Header class in the 1.2 implementation.
When sending data it add record headers and encryption to the records.

Handshake_Layer

This class takes care of parsing and marshalling of Handshake_Messages as well as equipping them with the appropriate handshake protocol headers. As this class handles the byte-representation of all TLS handshake messages (i.e. before parsing or after serialization), it is responsible for updating the Transcript_Hash_State appropriately.

In TLS 1.2 the task of the Handshake_Layer were mostly implemented in the Handshake_IO class. Note that Handshake_Layer does not perform any handshake state validations. It simply parses/serializes messages as they come in from the wire or the downstream protocol implementation.

Cipher_State

This class implments the Key Schedule mechanism. Most importantly, it derives and holds traffic secrets and provides interfaces for the Record_Layer protect and deprotect records. The Client advances the Cipher_State through the Key Schedule "state machine".

Transcript_Hash_State

This class keeps track of the transcript hash while sending/receiving messages in the Handshake_Layer. Client (and later Server) consult the Transcript_Hash_State for relevant hash-data when updating the Cipher_State appropriately.

Client

The Client's main responsibility continues to lie in process_handshake_message, implemented via individual handle-methods for each specific message type. Hence, the client's TLS state machine is implemented here. During message processing, it updates the state of other components:

• advancing the Key Schedule in Cipher_State
• consult the Transcript_Hash_State
• manage expected next messages in Handshake_Transitions

Handshake_Transitions

This class aids the Client implementation in validating state transitions. It is merely an extraction of the confirm_transition_to and set_expected_next functionality from TLS 1.2's Handshake_State.

Handshake_State

HandshakeState manages the incoming and outgoing messages and keeps a record of them for later reference. Before storing the messages it filters them regarding the Outbound_Message and Inbound_Message variants. Hence, a misuse of Handshake_State will fail to compile (e.g. when a client implementation tries to send a Server_Hello_13). Similarly, the compiler can check that Client implements handle() methods for exactly the relevant handshake messages.

Handshake_Messages

The class hierarchy of Handshake_Messages implements the specifics of individual handshake messages. In the TLS 1.3 implementation we don't rely on the actual polymorphic nature of Handshake_Message but replace it with std::variant constructions to specifically define which handshake message types are expected where.

Each handshake message takes care of "local" protocol semantic checks. I.e. all validations that can be performed without contextual information is happening right in the parsing code. The same holds true for handshake message extensions (e.g. Key_Share, Supported_Groups, ...). Protocol logic that is tied to specific messages or extensions is implemented locally in those classes. For instance, Key_Share implements Diffie-Hellman in its exchange method and Client_Hello_13::retry() implements necessary updates and validations for handling "hello retry requests".

This is a different paradigm compared to the messages in TLS 1.2, where many messages where mere "Data Transfer Objects" without much logic. Hence, messages derive a _12 and _13 sub-class and share the parsing code in the parent class where possible. This is also useful in situations where the interface and usage of the same message type differs between TLS 1.2 and TLS 1.3, but the wire representation is unchanged.

[lgtm-com]

This pull request introduces 1 alert when merging 6b19ed4 into 8c61154 - view on LGTM.com

new alerts:

• 1 for Futile conditional

[lgtm-com]

This pull request introduces 1 alert when merging 4229dff into 8c61154 - view on LGTM.com

new alerts:

• 1 for Futile conditional

[codecov-commenter]

Codecov Report

Merging #2922 (ebc62be) into master (8739125) will increase coverage by 0.04%.
The diff coverage is 92.97%.

❗ Current head ebc62be differs from pull request most recent head a89c539. Consider uploading reports for the commit a89c539 to get more accurate results

@@            Coverage Diff             @@
##           master    #2922      +/-   ##
==========================================
+ Coverage   92.55%   92.59%   +0.04%     
==========================================
  Files         577      596      +19     
  Lines       66780    69729    +2949     
  Branches     6401     6609     +208     
==========================================
+ Hits        61806    64568    +2762     
- Misses       4941     5128     +187     
  Partials       33       33              

Impacted Files	Coverage Δ
src/lib/tls/tls12/msg_certificate_12.cpp	87.80% <ø> (ø)
src/lib/tls/tls12/tls_channel_impl_12.cpp	89.04% <0.00%> (-0.81%)	⬇️
src/lib/tls/tls_algos.cpp	86.59% <0.00%> (+2.66%)	⬆️
src/lib/tls/tls_server.cpp	65.90% <0.00%> (-4.83%)	⬇️
src/lib/tls/tls_suite_info.cpp	100.00% <ø> (ø)
src/cli/tls_utils.cpp	77.64% <50.00%> (ø)
src/lib/tls/msg_session_ticket.cpp	76.92% <50.00%> (-4.90%)	⬇️
src/cli/tls_client.cpp	74.41% <66.66%> (-0.59%)	⬇️
src/lib/tls/tls_extensions_cert_status_req.cpp	90.32% <71.42%> (-2.28%)	⬇️
src/lib/tls/tls_signature_scheme.cpp	73.84% <73.84%> (ø)
... and 65 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4bbe0a...a89c539. Read the comment docs.

[randombit]

@hrantzsch @reneme Guys so sorry about how long review is taking on my side, work is kind of a death march at the moment. I will see what I can do over this weekend.

[hrantzsch]

Status update: the "MVP" is now pretty much done and even usable. We started testing using the current (TLS 1.2) Bogo configuration. Some of those tests downgrade to TLS 1.2 and continue to (successfully) test the 1.2 implementation, some succeed without downgrading, and a number of tests (70/1160, specifically those testing NYI features like resumption) will fail.

Next week we will look into the remaining Bogo test failures and tick the other minor items in the TODO list.

[lgtm-com]

This pull request introduces 1 alert when merging d8509a4 into 6ada2f3 - view on LGTM.com

new alerts:

• 1 for Futile conditional

[randombit]

OK did a second pass review now. I think past addressing the comments I left, all of which are just small style nits, we should go ahead and get this merged onto master.

[lgtm-com]

This pull request introduces 1 alert when merging 37eaa89 into 8bfb00f - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same

[lgtm-com]

This pull request introduces 1 alert when merging 574b8f7 into 8bfb00f - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same

[reneme]

@randombit Thanks for looking deeper into this! Your review is much appreciated.
I added your proposed changes up to this point.

Apart from the last unresolved tasks and discussions (#2922 (comment), #2922 (comment), #2922 (comment)) I'd like to draw your attention to the list of "Potential Future Work" in this PR's description. Albeit uncritical, I believe those improvements would be beneficial for both performance and maintainability. Should we open a separate issue to track those?

Further, I feel we should invest some time into reviewing the application-facing interfaces (particularly TLS::Policy and TLS::Callbacks). Some of the callbacks don't make sense for TLS 1.3 or lack information to be useful. We started assembling a list a while back: #2714 (comment) (probably not complete). Obviously, most of those potential API adaptions could also be tackled when the need comes up. Though, some might require breaking API changes.

Lastly, I would really appreciate a beta-test by a potential future consumer of the TLS 1.3 implementation. Maybe @pist-eb is still interested?

[randombit]

Should we open a separate issue to track those?

I saw these and don't think any should block merging this PR. We could track the remaining work either as distinct issues, or (maybe simpler) by opening a single issue that contains a checklist of all of the todos and fixmes that would otherwise block releasing a "final" 1.3 implementation.

Further, I feel we should invest some time into reviewing the application-facing interfaces (particularly TLS::Policy and TLS::Callbacks).

I agree on this. Some may already be obsolete with the removal of TLS 1.0/1.1 also.

For the key agreement callbacks I think we can just remove or change them suitably to cover both 1.2 and 1.3. In general a gradual deprecation is nicer but those particular callbacks are very niche and IMO it's simpler for us to just document what changed and give guidance on how to adapt to whatever we come up with that works for both 1.2 and 1.3

[reneme]

Further, I feel we should invest some time into reviewing the application-facing interfaces (particularly TLS::Policy and TLS::Callbacks).

I agree on this. Some may already be obsolete with the removal of TLS 1.0/1.1 also.

I'm working on a TLS::Callbacks(#2988) and TLS::Policy (#2989) overhauls: Please feel free to put in your feedback on the go.

[lgtm-com]

This pull request introduces 1 alert when merging ebc62be into db89870 - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same

[lgtm-com]

This pull request introduces 1 alert when merging a89c539 into db89870 - view on LGTM.com

new alerts:

• 1 for Comparison result is always the same

[randombit]

@reneme As far as I'm concerned this is fine to merge to master, do you see any issue with that?

[reneme]

No issues with merging it. Thanks for the time reviewing all of this. 😊

[reneme]

@randombit we're waiting for your final approval. :)

[hrantzsch]

Thank you so much @randombit and @reneme. Really happy about this 🥳