-
Notifications
You must be signed in to change notification settings - Fork 570
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
Enable the post-quantum x25519+ML-KEM-768 TLS 1.3 ciphersuite by default #4305
base: master
Are you sure you want to change the base?
Conversation
503d8df
to
ee8f6c0
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.
🚀 Nice! The PQ-era is here!
As a side note: This points at an open tech debt regarding the management of ciphersuites (also noted for a long time in #2990). As it is now, there's a lot of if {} else if {} else
scattered around many places in the TLS implementation. Whenever algorithms are added, there's a lot of potential to miss something.
Something is wrong - I know I tested that our client connecting to our server would negotiate Kyber (highly relevant since Botan being on both sides of the connection is very common) but now this doesn't work .... |
Oh I see. I tested us<->us in my initial attempt which put Kyber at the absolute top of the preference list. However I realized later this caused us to send a large keyshare that is ignored much of the time, which is non-optimal. But it seems - unlike the stacks in |
OK I think we just need to adjust the logic in |
... I was about to say that. Currently, the code there optimizes for round-trips and avoids sending a HelloRetryRequest whenever it can. I.e.: it will go for the non-PQ group if it is offered and fits with our supported groups. |
I'll take a look at that BoringSSL and co do here. I expect it looks something like a 2-tier selection
|
Sounds reasonable to me as a default policy. I was thinking to propose an additional policy setting like |
|
OTOH we already have a lot of fucking policy toggles 😅 |
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.
BoGo isn't happy about our new cleverness in selecting KEX algos, I guess. Probably, we'll just have to override the policy with the old logic there.
18b58a2
to
83a05cc
Compare
ec0d301
to
a354031
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.
Let's not start offering Kyber by default anymore.
allow_dtls12 = true | ||
ciphers = ChaCha20Poly1305 AES-256/GCM AES-128/GCM | ||
macs = AEAD SHA-256 SHA-384 SHA-1 | ||
signature_hashes = SHA-512 SHA-384 SHA-256 | ||
signature_methods = ECDSA RSA | ||
key_exchange_methods = ECDH DH | ||
key_exchange_groups = x25519 x448 secp256r1 brainpool256r1 secp384r1 brainpool384r1 secp521r1 brainpool512r1 ffdhe/ietf/2048 ffdhe/ietf/3072 ffdhe/ietf/4096 ffdhe/ietf/6144 ffdhe/ietf/8192 | ||
key_exchange_groups = x25519 secp256r1 x25519/Kyber-768-r3 x448 secp384r1 secp521r1 brainpool256r1 brainpool384r1 brainpool512r1 ffdhe/ietf/2048 ffdhe/ietf/3072 |
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.
That should use ML-KEM now, no? Just like the default_tls13.txt
key_exchange_groups = x25519 secp256r1 x25519/Kyber-768-r3 x448 secp384r1 secp521r1 brainpool256r1 brainpool384r1 brainpool512r1 ffdhe/ietf/2048 ffdhe/ietf/3072 | |
key_exchange_groups = x25519 secp256r1 x25519/ML-KEM-768 x448 secp384r1 secp521r1 brainpool256r1 brainpool384r1 brainpool512r1 ffdhe/ietf/2048 ffdhe/ietf/3072 |
src/lib/tls/tls_policy.cpp
Outdated
By default, we offer a key share for the most-preferred pure ECC group | ||
by default, if any pure ECC group is enabled in the policy. |
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.
By default, we offer a key share for the most-preferred pure ECC group | |
by default, if any pure ECC group is enabled in the policy. | |
By default, we offer a key share for the most-preferred pure ECC group | |
if any pure ECC group is enabled in the policy. |
src/lib/tls/tls_algos.h
Outdated
@@ -198,7 +203,7 @@ class BOTAN_PUBLIC_API(3, 2) Group_Params final { | |||
m_code == Group_Params_Code::FFDHE_8192; | |||
} | |||
|
|||
constexpr bool is_pure_kyber() const { | |||
constexpr bool is_pure_kyber_r3() const { |
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.
This is a public API. I think we'll have to go with deprecation of is_pure_kyber()
here.
7d241d6
to
9a1a4d8
Compare
This enables the Cloudflare X25519+Kyber-r3 ciphersuite by default. It also rearranges the default list, and removes some of the most expensive FFDHE groups. Expose which group was used for key exchange in the Session_Summary. Fix a bug in TLS_Text_Policy; it printed reuse_session_tickets as an integer rather than a bool. This was caught by the need to update some of the policy text files.
9a1a4d8
to
2ea7092
Compare
Tested with cloudflare.com, google.com and ourselves.
This adjusts the default logic for both which groups to offer and which group to select during negotiation.
For offering, we send the first pure ECC group in the preference list. This avoids sending large PQ shares to servers that don't support them. If the client for whatever reason does not have any pure ECC groups, then we send a share of whatever their top preference is.
On the server side, if the client indicated support for any mutually supported PQ algorithm, we always select it, even if the client sent some other type of keyshare. Previously we would always prefer to select a group that the client sent a share of, to reduce round trips.