Conversation
Codecov ReportBase: 92.59% // Head: 92.57% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2974 +/- ##
==========================================
- Coverage 92.59% 92.57% -0.02%
==========================================
Files 600 601 +1
Lines 70092 70637 +545
Branches 6628 6672 +44
==========================================
+ Hits 64903 65394 +491
- Misses 5156 5210 +54
Partials 33 33
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. |
00bd2d2 to
9fc97c1
Compare
| "TLS12SessionID-TLS13": "We don't offer TLS 1.3 when a TLS 1.2 session was found", | ||
| "Ticket-Forbidden-TLS13": "We don't offer TLS 1.3 when a TLS 1.2 session was found", | ||
| "Resume-Client-NoResume-TLS12-TLS13-TLS": "We don't offer TLS 1.3 when a TLS 1.2 session was found", | ||
| "Resume-Client-Mismatch-TLS12-TLS13-TLS": "We don't offer TLS 1.3 when a TLS 1.2 session was found", |
There was a problem hiding this comment.
We basically query the Session_Manager before instantiating the TLS 1.2 or TLS 1.3 implementation. If we find a TLS 1.2 session we don't even bother offering TLS 1.3 in the first place. Hence, we cannot fulfil those tests. Though, I think it's worth discussing whether the described behaviour is actually wise.
There was a problem hiding this comment.
Would of course be nice to switch to TLS 1.3. even than an older session exist. But unless you think the new some API change for that I don't think it is important to have it in the first version.
| // buffer without re-parsing it. | ||
| // | ||
| // Finds the truncation offset in a serialization of Client Hello as defined in | ||
| // RFC 8446 4.2.11.2 used for the calculation of PSK binder MACs. |
There was a problem hiding this comment.
... yeah. The binder MACs are unfortunately a major cross-cutting concern through our layered implementation architecture. Probably it would be a good idea to see how other implementations are handling this. Maybe I missed a short cut. The current implementation works, but its sad to see this.
|
|
||
| // Check whether we should generate a truncated hash for supporting PSK | ||
| // binder calculation or verification. See RFC 8446 4.2.11.2. | ||
| if(serialized_message_length > 0 && *serialized_message == CLIENT_HELLO) |
There was a problem hiding this comment.
Same as find_client_hello_truncation_mark, this is somewhat "magic" and an ugly breach of abstraction. Before, the Transcript_Hash_State was a dumb class that just "hashed the transcript" as it was passed into it. Now it automagically detects whether it is hashing a Client Hello message, whether that Client Hello message contains a PSK extension (in find_client_hello_truncation_mark) and creates a truncated() hash in that case.
Apart from the level-of-abstraction ugliness, this introduces overhead due to additional memory copies and repeated marshalling and parsing of the data structure.
For now, this is the least intrusive implementation I could come up with. Though, I'm very much open to discussing a better approach to this.... @hrantzsch :)
|
Rebased and retargeted to master. |
* New_Session_Ticket * PSK extension * EarlyDataIndication extension
af88496 to
defa8ac
Compare
4177f4b to
a66393b
Compare
7571118 to
410bd69
Compare
This adds session resumption to the TLS 1.3 implementation.
Central changes in this pull request:
TLS::Sessionclass to allow storing TLS 1.3 sessionsinit_with_psk()Client_Hello_13to calculate PSK bindersTruncate(Client_Hello)function as outlined in RFC 8446 4.2.11.2Provide(now extracted here)TLS::Callbacks::tls_current_timestamp()for deterministic timestamps for testingCurrently, the TLS 1.3 PSK mechanism is exclusively used for session resumption. It should be fairly straight forward to extend it for out-of-band PSKs. Furthermore, this pull request leaves support for early data and early key export to future work.
Caveat: RFC 8446 suggests to not reuse session tickets to prevent passive client tracking. This is not implemented by the client implementation (neither in TLS 1.2). I suppose this is meant to be implemented by a custom
Session_Managerif needed?!