-
Notifications
You must be signed in to change notification settings - Fork 564
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
[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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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::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:
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::span
to avoid copying(some protocol parameters are negotiated differently)
Ciphersuite
class 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 --debug
to print raw TLS trafficFixed_Output_RNG
can optionally fall back to another RNG when its pool is empty(see here for a list of issues found by BoGo)
./botan tls_client
with 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_NOTIFY
properly (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
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 ofTLS::Client
andTLS::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
andHandshake_Layer
to 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_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 theTranscript_Hash_State
appropriately.In TLS 1.2 the task of the
Handshake_Layer
were mostly implemented in theHandshake_IO
class. Note thatHandshake_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. TheClient
advances theCipher_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 laterServer
) consult theTranscript_Hash_State
for relevant hash-data when updating theCipher_State
appropriately.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_State
Transcript_Hash_State
Handshake_Transitions
Handshake_Transitions
This class aids the Client implementation in validating state transitions. It is merely an extraction of the
confirm_transition_to
andset_expected_next
functionality from TLS 1.2'sHandshake_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 theOutbound_Message
andInbound_Message
variants. Hence, a misuse ofHandshake_State
will fail to compile (e.g. when a client implementation tries to send aServer_Hello_13
). Similarly, the compiler can check thatClient
implementshandle()
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 ofHandshake_Message
but replace it withstd::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 itsexchange
method 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
_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.