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

[TLS 1.3] Post-Quantum Readiness via Hybrid Key Exchange #2983

Closed
wants to merge 6 commits into from

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented May 31, 2022

Pull-Request Dependencies

Both change sets are currently also displayed in this pull request. Hence, review and merge of those should make this PR fairly small (~700 lines added).

TODO

  • Add a "hybrid" key exchange method to the TLS policy
  • Add a minimum_kyber_group_size() to the TLS policy (??)

Description

This enables the TLS 1.3 implementation to perform hybrid key exchanges using a classical KEX (ECDH or X25519) and a post-quantum KEM (Kyber or Kyber90s). The implementation is based on this IETF draft and the group identifiers for the Key Share extension are taken from OQS.

Demo

./configure.py                 \
    --build-targets=static,cli \
    --minimized-build          \
    --without-documentation    \
    --enable-modules=tls13,tls13_pqc,auto_rng,system_rng,chacha20poly1305,curve25519,kyber

make -j$(nproc) cli

./botan tls_client                                           \
    --policy=src/tests/data/tls-policy/default_tls13_pqc.txt \
    --port=443                                               \
    kms.eu-central-1.amazonaws.com

Using the snippets above, one should obtain a TLS 1.3 connection to Amazon's KMS endpoint (that is already PQC-enabled using their s2n-tls library). Simply typing "GET / HTTP/1.1" [Enter][Enter] should yield an (admittedly useless) "Bad Request" response.

The PQC TLS 1.3 policy file passed to the CLI uses X25519/Kyber512 as the hybrid key exchange scheme.

@lgtm-com
Copy link

lgtm-com bot commented Jun 1, 2022

This pull request introduces 1 alert when merging 7b74a14 into 8bfb00f - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@reneme
Copy link
Collaborator Author

reneme commented Jul 5, 2022

Rebased to master.

@reneme reneme force-pushed the tls13/hybrid_key_exchange branch from 9b38694 to 5bad80a Compare July 6, 2022 12:34
@reneme reneme marked this pull request as draft October 20, 2022 06:32
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

Broadly speaking looks good to me. Needs a rebase to resolve the merge conflicts and I can give a final review.

@reneme
Copy link
Collaborator Author

reneme commented Dec 16, 2022

Rebased to master.

This will certainly need another look and compatibility testing round. Maybe it could even be used as a vehicle to find a better alternative to the tls_dh_agree() callbacks discussed here. After all is basically just another group, of sorts.

I'd suggest to keep this open until most of the the remaining TLS 1.3 work is done and revisit.

@codecov-commenter
Copy link

Codecov Report

Base: 87.98% // Head: 87.69% // Decreases project coverage by -0.29% ⚠️

Coverage data is based on head (f6b4f80) compared to base (4eb304b).
Patch coverage: 3.63% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2983      +/-   ##
==========================================
- Coverage   87.98%   87.69%   -0.30%     
==========================================
  Files         599      600       +1     
  Lines       66333    66544     +211     
  Branches     6610     6635      +25     
==========================================
- Hits        58365    58355      -10     
- Misses       5181     5407     +226     
+ Partials     2787     2782       -5     
Impacted Files Coverage Δ
src/lib/tls/tls13_pqc/composite_public_key.cpp 0.00% <0.00%> (ø)
src/lib/tls/tls_algos.cpp 58.64% <0.00%> (-25.40%) ⬇️
src/lib/tls/tls13/tls_extensions_key_share.cpp 73.29% <19.35%> (-10.95%) ⬇️
src/lib/tls/msg_client_hello.cpp 82.97% <100.00%> (ø)
src/lib/tls/tls13/tls_client_impl_13.cpp 84.28% <100.00%> (ø)
src/lib/entropy/rdseed/rdseed.cpp 18.18% <0.00%> (-63.64%) ⬇️
src/lib/utils/cpuid/cpuid_x86.cpp 45.94% <0.00%> (-7.39%) ⬇️
src/cli/cli_rng.cpp 60.00% <0.00%> (-4.00%) ⬇️
src/lib/utils/cpuid/cpuid.cpp 62.16% <0.00%> (-2.40%) ⬇️
src/lib/pubkey/dl_group/dl_group.cpp 84.42% <0.00%> (-0.68%) ⬇️
... and 12 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

@randombit
Copy link
Owner

There is a specific draft for X25519+Kyber768 which is already implemented by Zig stdlib and also (IIRC) Cloudflare https://github.com/bwesterb/draft-westerbaan-tls-xyber768d00/blob/main/draft-tls-westerbaan-xyber768d00.md

@reneme
Copy link
Collaborator Author

reneme commented Jun 30, 2023

Closing as superseded by: #3609

@reneme reneme closed this Jun 30, 2023
@randombit randombit deleted the tls13/hybrid_key_exchange branch April 20, 2024 12:17
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