[TLS 1.3] Hybrid PQ/T key establishment#3609
Conversation
b452104 to
db5c166
Compare
db5c166 to
6d58057
Compare
|
With this policy: We tested successfully against:
We couldn't negotiate against:
|
6d58057 to
5741e18
Compare
136c761 to
a963bea
Compare
I believe we should. See for example Edit: done |
e4af46f to
3f7c9c4
Compare
|
Rebased to |
randombit
left a comment
There was a problem hiding this comment.
This is only a partial review - I haven't reviewed the KEX->KEM adapter at all and only partially reviewed hybrid_public_key.cpp. I will finish the review asap.
|
Thanks for the review. Unfortunately, I won't be able to look into it before August 7th. Perhaps @FAlbertDev wants to give it a go, though. 😃 |
Yeah, I'll give it a try 👍 |
4a159c8 to
745dae8
Compare
|
Thanks for addressing the review comments @FAlbertDev, on a quick review of just your commit things looked good. I want to do another full pass review, I'll get this in sometime before next week. |
745dae8 to
0988317
Compare
0988317 to
fcbf8a6
Compare
|
Rebased to |
| /** | ||
| * This helper determines the length of the agreed-upon value depending | ||
| * on the key agreement public key's algorithm type. It would be better | ||
| * to get this value via PK_Key_Agreement::agreed_value_size(), but | ||
| * instantiating a PK_Key_Agreement object requires a PrivateKey object | ||
| * which we don't have (yet) in the context this is used. | ||
| * | ||
| * TODO: Find a way to get this information without duplicating those | ||
| * implementation details of the key agreement algorithms. | ||
| */ | ||
| size_t kex_shared_key_length(const Public_Key& kex_public_key) { | ||
| return std::visit( | ||
| overloaded{[](const ECDH_PublicKey& ecdh_public_key) { return ecdh_public_key.domain().get_p_bytes(); }, | ||
| [](const DH_PublicKey& dh_public_key) { return dh_public_key.group().p_bytes(); }, | ||
| [](const Curve25519_PublicKey&) { return size_t(32) /* TODO: magic number */; }}, | ||
| as_kex_public_key(kex_public_key)); | ||
| } |
There was a problem hiding this comment.
It would be great if we could access certain basic information of specific algorithms statically.
There was a problem hiding this comment.
Thought I had left a comment on this earlier? Hm.
Statically might be difficult given how DH/ECDH are defined over arbitrary groups, but it seems pretty possible to have something like virtual size_t raw_shared_key_length() = 0 the challenge is on what type to put it since PK_Key_Agreement_Key is only for the private keys ...
Conceivably we could have something on Public_Key like so
/**
* Return the length of the relevant artifact of this key.
*
* Throws Not_Implemented if the key does not support an operation of this type.
*
* For KeyAgreement, returns the raw (non-KDF) output length.
*/
virtual size_t output_length(PublicKeyOperation op) = 0
```
randombit
left a comment
There was a problem hiding this comment.
Looks very good, left a few comments, ptal
1cdf85b to
7394ffb
Compare
|
Done with your latest review comments. I updated the PR description (mainly for future reference) and created #3706 to collect future improvements on the Pubkey-APIs. I committed the review fixes into new commits for easy review. Let me know if I should squash before we merge eventually. |
(draft-ietf-tls-hybrid-design) Co-Authored-By: Fabian Albert <fabian.albert@rohde-schwarz.com>
7394ffb to
4486d90
Compare
|
Rebased to master. |
randombit
left a comment
There was a problem hiding this comment.
Did a full review, left a few minor comments. Approving to unblock since I didn't notice anything major. This certainly points out some nasty holes in our API surface; in principle it should be possible to implement the hybrid and kex->kem adaptors without referencing any algorithm specific details.
512e0ff to
138ee13
Compare
138ee13 to
413f7a6
Compare
Pull Request Dependencies
[TLS 1.3] Use a KEM interface in the Callbacks #3608Chore: Update BoGo tests #3616API modernization PK_KEM_Encryptor/Decryptor #3611Things to do
test_cli.py cli_tls_socket_tests)Description
This (finally) follows up on #2983 and is a more serious attempt in delivering hybrid key exchange based on draft-ietf-tls-hybrid-design-06.
Hybrid key exchange is handled by an adapter class
Hybrid_KEM_PrivateKeythat combines 2 or more KEX/KEM keys as specified in draft-ietf-tls-hybrid-design-06. To upstream users it provides a KEM interface.Internally
Hybrid_KEM_PrivateKeyuses another adapterKEX_to_KEM_Adapter_PrivateKeythat takes a single Key Agreement key (e.g. X25519) and exposes a KEM interface based on it. "KEM-Encaps" is implemented by generating a matching ephemeral key pair and using the ephemeral public key as the "encapsulated secret".There are plenty of
TODOwhere we needed to workaround short-comings of thePublic_KeyAPI. Potential improvements are collected in #3706.IANA did not define code points for PQ and PQ/T key exchange groups in TLS, yet. As a result, different (beta) services define incompatible algorithm identifiers. We're providing a number of algorithm aliases to be compatible to both libOQS and Cloudflare's implementation. This is certainly a moving target that might change in the near future.
To give this a test run and connect to cloudflare.com using PQC (see their blog post -- section "What we deployed" -- for more details):
pqc_tls.txt):You'll see that the exchanged Client Hello and Server Hello messages in the debug output are unusually large. The kyber public key weighs about 800bytes.