Conversation
Codecov ReportBase: 92.55% // Head: 91.79% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2989 +/- ##
==========================================
- Coverage 92.55% 91.79% -0.77%
==========================================
Files 601 599 -2
Lines 70608 72086 +1478
Branches 6666 6603 -63
==========================================
+ Hits 65353 66172 +819
- Misses 5225 5914 +689
+ Partials 30 0 -30
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. |
|
This pull request introduces 1 alert when merging 72f44f0 into db89870 - view on LGTM.com new alerts:
|
0c8c29a to
80408a4
Compare
17c2e54 to
26b593c
Compare
randombit
left a comment
There was a problem hiding this comment.
Similar comment re the callbacks PR - for EMS support, if it is always enabled and the policy does nothing, we might as well just remove it entirely in 3.0. Otherwise LGTM
src/lib/tls/tls_policy.h
Outdated
| * extended master secret is always enabled by the implementation. | ||
| */ | ||
| BOTAN_DEPRECATED("extended master secret is always enabled for TLS 1.2 and not applicable for TLS 1.3") | ||
| virtual bool use_extended_master_secret() const; |
There was a problem hiding this comment.
This being the case we should just remote it as a toggle
TLS 1.2 enables this by default, TLS 1.3 doesn't have an explicit notion of EMS anymore.
Policy Methods that might need attention
Cipher suite related:
allowed_ciphers(),allowed_macs(), ...My current working assumption: TLS 1.3 cipher suites are simply subject to those policy configurations as well.
allowed_key_exchange_methods()needs to be extended for hybrid key exchange for PQC.only_resume_with_exact_version()Sessions that originally negotiated TLS 1.2 cannot be resumed as TLS 1.3 and vice versa in the current implementation. I'm not sure whether this would even be technically possible. Since TLS protocol versions prior to 1.2 were removed, this will not have an effect.
record_size_limit()Currently, the record size limit extension is supported by the TLS 1.3 implementation only. Hence, it cannot be offered if our Botan-client signals willingness to negotiate either 1.2 or 1.3. Otherwise a peer might choose to negotiate TLS 1.2 and use the record size limit extension which would break. That's a bit of a bummer (and a non-obvious cause for user frustration), but the extension is fairly niche anyway. We mainly implemented it for compliance with the RFC 8448 tests that use it.
Modifications at a Glance
Removed
use_extended_master_secret()TLS 1.2 enables this extension by default (and ignores the setting of this policy config). TLS 1.3 connections don't have a notion of "extended master secret" anymore. Hence the removal.
key_exchange_groups_to_offer()TLS 1.3 client specific: the groups (in order of preference) that should be offered in the initial Client Hello's KeyShare extension. By default, this will offer the first supported group (as returned by
Policy::key_exhange_groups()) but applications may adjust this. Somewhat obvious: only groups that are supported can be offered.choose_key_exchange_group()has a new parameterThis method existed before. It is responsible for deciding which key exchange group a peer should use given the counter-party's supported groups. Now, it additionally obtains a list of groups that the counter-party send TLS 1.3 key share offers for. When called from the TLS 1.2 implementation the new parameter is always empty.
With that, it is up to the application whether a Hello Retry Request should be issued (e.g. to choose a PQ-safe group over an offered conventional group). By default, this will try to avoid Hello Retry Requests by choosing a group that was offered by the counter-party.
tls_13_middlebox_compatibility_mode()Lets a TLS 1.3 client behave as specified in RFC 8446 Appendix D.4. TL;DR: Make the TLS 1.3 handshake appear to be a TLS 1.2 session resumption to fool non-compliant middleboxes. This is on by default and (obviously) has no effect on the TLS 1.2 implementation. Not added in this PR.
hash_hello_random()Whether or not the random string in client/server hello should be hashed with SHA-256 after being drawn from the RNG. This is enabled by default and was added to allow deterministic testing (i.e. RFC 8448). Not added in this PR.
Documentation
Added
@noteparagraphs for policy configs that have no effect for either TLS 1.2 or 1.3 respectively.