Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

randombit
Copy link
Owner

@randombit randombit commented Aug 13, 2024

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.

@randombit randombit added this to the Botan 3.6.0 milestone Aug 13, 2024
@randombit randombit requested a review from reneme August 13, 2024 22:23
@coveralls
Copy link

coveralls commented Aug 13, 2024

Coverage Status

coverage: 91.245% (-0.04%) from 91.281%
when pulling ee8f6c0 on jack/enable-pqc
into 244c0cb on master.

Copy link
Collaborator

@reneme reneme left a 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.

src/lib/tls/tls_policy.cpp Outdated Show resolved Hide resolved
src/lib/tls/tls_session.cpp Outdated Show resolved Hide resolved
src/lib/tls/tls_session.cpp Outdated Show resolved Hide resolved
src/lib/tls/tls_session.cpp Outdated Show resolved Hide resolved
src/lib/tls/tls_session.cpp Outdated Show resolved Hide resolved
src/tests/data/tls-policy/default.txt Outdated Show resolved Hide resolved
src/tests/data/tls-policy/default_tls13.txt Outdated Show resolved Hide resolved
src/tests/data/tls-policy/strict.txt Outdated Show resolved Hide resolved
src/tests/data/tls-policy/strict_tls13.txt Outdated Show resolved Hide resolved
src/lib/tls/tls_algos.cpp Outdated Show resolved Hide resolved
@randombit
Copy link
Owner Author

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 ....

@randombit
Copy link
Owner Author

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 google.com and cloudflare.com - we will, if we receive a keyshare for say x25519, we won't ignore it even if the top preference of both the client and the server are Kyber - we just negotiate x25519. So we won't negotiate Kyber with ourselves 😭

@randombit randombit requested a review from reneme August 15, 2024 11:36
@randombit
Copy link
Owner Author

OK I think we just need to adjust the logic in Policy::choose_key_exchange_group.

@reneme
Copy link
Collaborator

reneme commented Aug 15, 2024

OK I think we just need to adjust the logic in Policy::choose_key_exchange_group.

... 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.

@randombit
Copy link
Owner Author

I'll take a look at that BoringSSL and co do here. I expect it looks something like a 2-tier selection

  • If both peers share a PQC algorithm of any kind, then we're using PQC. If the client offered a PQC share, use that (even if it's not our favorite PQC). Otherwise choose a PQC, either clients favorite or servers favorite, depending on server_uses_own_ciphersuite_preferences.

  • If there is no mutually agreed upon PQC group, then use effectively the existing logic.

@reneme
Copy link
Collaborator

reneme commented Aug 15, 2024

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 prefer_pqc_groups_when_possible(). But then again: if one is explicitly offering PQC support at this time, I guess they are also fine with using it anyway.

@randombit
Copy link
Owner Author

prefer_pqc_groups_when_possible() might be worth having as an easy to use toggle for those who are willing to use PQC but would like to save the round trip where possible.

@randombit
Copy link
Owner Author

OTOH we already have a lot of fucking policy toggles 😅

Copy link
Collaborator

@reneme reneme left a 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.

src/lib/tls/tls_policy.cpp Outdated Show resolved Hide resolved
src/lib/tls/tls_session.cpp Outdated Show resolved Hide resolved
src/tests/unit_tls_policy.cpp Outdated Show resolved Hide resolved
src/lib/tls/tls_session.h Outdated Show resolved Hide resolved
src/lib/tls/tls_session.cpp Outdated Show resolved Hide resolved
@randombit randombit modified the milestones: Botan 3.6.0, Botan 3.7.0 Oct 7, 2024
src/lib/tls/tls_policy.cpp Outdated Show resolved Hide resolved
@randombit randombit requested a review from reneme October 24, 2024 09:02
@randombit randombit force-pushed the jack/enable-pqc branch 3 times, most recently from ec0d301 to a354031 Compare October 24, 2024 09:29
@randombit randombit changed the title Enable PQC ciphersuite in TLS 1.3 by default Enable the post-quantum x25519+ML-KEM-768 TLS 1.3 ciphersuite by default Oct 24, 2024
Copy link
Collaborator

@reneme reneme left a 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
Copy link
Collaborator

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

Suggested change
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/tests/data/tls-policy/strict.txt Outdated Show resolved Hide resolved
Comment on lines +218 to +219
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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

@@ -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 {
Copy link
Collaborator

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.

This change also rearranges the default kex list, and removes some of
the most expensive FFDHE groups.

Group selection logic is changed such that if the client supports a
post quantum algorithm, it is always preferred even if the client
sent other (non-PQC) key shares.

Expose which group was used for key exchange in the Session_Summary.

Co-authored-by: René Meusel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants