DHKEM from ECDH trait #1186
DHKEM from ECDH trait #1186
Conversation
jschneider-bensch
force-pushed
the
jonas/kem-from-ecdh
branch
from
8d33cf9 to
d76a50f
Compare
|
Rebased onto |
|
I think the approach here makes sense! Regarding how to abstract this: Since the blanket impl (had it worked) would likely have lived in libcrux-ecdh, maybe that would be a good place for the impl macro, and then we call it everywhere where we also implement ECDH? I agree it doesn't make this more readable but (a) I don't have an idea for how we could get it to work with blanked impls and (b) at least personally, I don't find macros that much worse than blanket impls from a readability perspective :) Regarding the extract-then-expand function: Is that exactly one of the HPKE key schedules? Maybe we could call it that. |
| let ek_x: &mut [u8; X25519_EK_LEN] = ek_x.try_into().unwrap(); | ||
|
|
||
| let (dk_m, dk_x) = dk.split_at_mut(MLKEM768_DK_LEN); | ||
| let dk_m: &mut [u8; MLKEM768_DK_LEN] = dk_m.try_into().unwrap(); |
There was a problem hiding this comment.
These have been here before, but we really shouldn't unwrap here. The first one would be safe if we checked the length of the thing we split. But the second one would need checking as well.
| libcrux-hkdf = { version = "=0.0.3", path = "../libcrux-hkdf", optional = true } | ||
|
|
||
| [features] | ||
| kem-api = ["libcrux-hkdf"] |
There was a problem hiding this comment.
Do we have naming convention for these? It could also just be kem. I'm fine either way.
curve25519/src/kem_api.rs
Outdated
| .map_err(|_| libcrux_traits::kem::arrayref::EncapsError::Unknown)?; | ||
|
|
||
| let mut derived_ecdh = [0u8; EK_LEN]; | ||
| <X25519 as EcdhArrayref<DK_LEN, DK_LEN, EK_LEN>>::derive_ecdh( |
There was a problem hiding this comment.
Wouldn't X25519:: be enough? Same for the other calls.
curve25519/src/kem_api.rs
Outdated
|
|
||
| pub const SHARED_SECRET_LEN: usize = 32; | ||
|
|
||
| impl libcrux_traits::kem::arrayref::Kem<DK_LEN, EK_LEN, EK_LEN, SHARED_SECRET_LEN, DK_LEN, DK_LEN> |
There was a problem hiding this comment.
If we import arrayref this entire thing gets a little more readable.
curve25519/src/kem_api.rs
Outdated
| .map_err(|_| libcrux_traits::kem::arrayref::EncapsError::InvalidEncapsKey)?; | ||
|
|
||
| extract_and_expand(ct, ss, ek, derived_ecdh) | ||
| .map_err(|_| libcrux_traits::kem::arrayref::EncapsError::Unknown) |
There was a problem hiding this comment.
Why do we do additional things with the secret? This will make this API unusable in most usecases.
There was a problem hiding this comment.
You mean, why do we do the HKDF?
The KEM implementation we had before gave out the raw DH shared secret, but we warn against doing that in libcrux-ecdh. This here provides specifically an implementation of DHKEM(X25519, HKDF-SHA256) under the KEM traits, which requires that round of HKDF to make the shared secret safe to use. If you need the raw DH shared secret, like in XWing for example, you can use the ECDH API.
Or do you mean something else?
There was a problem hiding this comment.
But the point of having a KEM API for ECDH as well, is to make it possible to have a uniform way of calling KEMs, if that's an ECC based one or a PQ one. Like we do for example in Bertie. That won't be possible with this API.
When should people use this then?
There was a problem hiding this comment.
Okay, I think we had a misunderstanding of what this should achieve then.
For me, this was a way to deal with the warning I linked above and provide a DHKEM implementation under the KEM API. It would be used when you need a DHKEM.
The previous KEM API did what you want already, it just did so without using the ECDH trait. Doing that on top of the ECDH trait should be easy, however: Just remove the HKDF-related bits here. In fact, removing the HKDF bits makes it possible to have a blanket impl in libcrux-traits.
Shall I do that instead?
|
As discussed offline, this doesn't quite go in the right direction. We do want the EC KEMs to output the raw secret for uniformity of use with PQ-KEMs in protocol implementations. Perhaps this can be salvaged into an easy fix for users that shouldn't use the raw secret, but it's not an immediate priority at the moment. |
|
This PR has been marked as stale due to a lack of activity for 60 days. If you believe this pull request is still relevant, please provide an update or comment to keep it open. Otherwise, it will be closed in 7 days. |
3 participants
This PR implements an RFC 9180-style DHKEM(X25519, HKDF-SHA256), sans Auth, on top of the ECDH trait in
libcrux_curve25519(only that one, for now).The implementation is behind a
kem-apifeature, since users may want to implement their own KEM on top of the ECDH trait, e.g. with a different KDF.There are some things I'm not quite satisfied with:
arrayref::Kemimplementation generic over someconst SHARED_SECRET_LEN: usize, it is not possible to make the macro-based implementation ofslice::Kemgeneric. For this reason, I've fixed theSHARED_SECRET_LEN == 32in both for now.p256. Some things I've thought about/tried:libcrux_traits: Does not work because,libcrux_hkdfdepends onlibcrux_traitsvialibcrux_sha2. ❌libcrux_kem: I think this does not work because of the orphan rule, since both traits are foreign. ❌libcrux_traits::kem: Maybe this could work, but I dislike it, because it makes it harder to understand how things work, and because the macro would have to assume a crate using it has the HKDF dependency, which is not present inlibcrux_traits(forbidden there, actually!).❓I've also had to touch the XWing implementation in(Errors handling can also be improved.)libcrux-kem, since it was using the KEM trait API for X25519, assuming it gave you the raw ECDH shared secret. Instead it now uses the ECDH API, which one the one side is what you want to use there. On the other hand the usage ofEcdhArrayrefis a bit inconvienient I found.