-
Notifications
You must be signed in to change notification settings - Fork 569
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] Hybrid PQ/T key establishment #3609
[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 |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we could access certain basic information of specific algorithms statically.
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.
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
```
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.
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 <[email protected]>
7394ffb
to
4486d90
Compare
Rebased to master. |
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.
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_PrivateKey
that 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_PrivateKey
uses another adapterKEX_to_KEM_Adapter_PrivateKey
that 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
TODO
where we needed to workaround short-comings of thePublic_Key
API. 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.