[TLS 1.3] Limited TLS 1.3 Client Implementation#2922
Conversation
9818f99 to
d9a11ad
Compare
b528815 to
6b19ed4
Compare
|
This pull request introduces 1 alert when merging 6b19ed4 into 8c61154 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 4229dff into 8c61154 - view on LGTM.com new alerts:
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
@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. |
98b49ff to
cd1db3c
Compare
9e7242c to
aa7ab3c
Compare
|
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. |
|
This pull request introduces 1 alert when merging d8509a4 into 6ada2f3 - view on LGTM.com new alerts:
|
2781c42 to
ac6b85e
Compare
randombit
left a comment
There was a problem hiding this comment.
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.
|
This pull request introduces 1 alert when merging 37eaa89 into 8bfb00f - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 574b8f7 into 8bfb00f - view on LGTM.com new alerts:
|
|
@randombit Thanks for looking deeper into this! Your review is much appreciated. 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 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? |
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.
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 |
I'm working on a |
|
This pull request introduces 1 alert when merging ebc62be into db89870 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging a89c539 into db89870 - view on LGTM.com new alerts:
|
|
@reneme As far as I'm concerned this is fine to merge to master, do you see any issue with that? |
|
No issues with merging it. Thanks for the time reviewing all of this. 😊 |
|
@randombit we're waiting for your final approval. :) |
|
Thank you so much @randombit and @reneme. Really happy about this 🥳 |
Limited TLS 1.3 Client
Pull Request Dependencies
Test::Resultconsolidation 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:
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.
TLS_Data_Reader!has_remaining()on destruction).::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)std::spanto avoid copying(some protocol parameters are negotiated differently)
Ciphersuiteclass provides methods that are strictly not defined for TLS 1.3 (potential cause for bugs)(e.g.
Policy::ciphersuite_list(), generation viatls_suite_info.py)This PR adds limited TLS 1.3 support. Available functionality:
tls_client./botan tls_client --debugto print raw TLS trafficFixed_Output_RNGcan optionally fall back to another RNG when its pool is empty(see here for a list of issues found by BoGo)
./botan tls_clientwith the big players (google.com, cloudflare.com, ...)Limitations:
(TLS 1.2 can be disabled in the TLS policy though)
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
CLOSE_NOTIFYproperly (difference between TLS 1.2 and 1.3)Session Resumption (PSK w/o 0-RTT)not part of this PRIntro
The descriptions mostly refer to the components in the
tls13module, 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 ofTLS::ClientandTLS::Serveralso 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,Ql21IMba1ZERgxfIiFElKgChannel
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_LayerandHandshake_Layerto transform received bytes from the network into handshake messages to be interpreted by theClient(and laterServer) 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 theRecord_Headerclass 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_Messagesas 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 theTranscript_Hash_Stateappropriately.In TLS 1.2 the task of the
Handshake_Layerwere mostly implemented in theHandshake_IOclass. Note thatHandshake_Layerdoes 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_Layerprotect and deprotect records. TheClientadvances theCipher_Statethrough 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 laterServer) consult theTranscript_Hash_Statefor relevant hash-data when updating theCipher_Stateappropriately.Client
The Client's main responsibility continues to lie in
process_handshake_message, implemented via individualhandle-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:Cipher_StateTranscript_Hash_StateHandshake_TransitionsHandshake_Transitions
This class aids the Client implementation in validating state transitions. It is merely an extraction of the
confirm_transition_toandset_expected_nextfunctionality from TLS 1.2'sHandshake_State.Handshake_State
HandshakeStatemanages the incoming and outgoing messages and keeps a record of them for later reference. Before storing the messages it filters them regarding theOutbound_MessageandInbound_Messagevariants. Hence, a misuse ofHandshake_Statewill fail to compile (e.g. when a client implementation tries to send aServer_Hello_13). Similarly, the compiler can check thatClientimplementshandle()methods for exactly the relevant handshake messages.Handshake_Messages
The class hierarchy of
Handshake_Messagesimplements the specifics of individual handshake messages. In the TLS 1.3 implementation we don't rely on the actual polymorphic nature ofHandshake_Messagebut replace it withstd::variantconstructions 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_Shareimplements Diffie-Hellman in itsexchangemethod andClient_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
_12and_13sub-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.