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

dhkem: add KEM providers for common ECDH schemes #16

Merged
merged 14 commits into from
Jul 11, 2024

Conversation

incertia
Copy link
Contributor

This replaces RustCrypto/traits#1556 and puts the Encapsulate and Decapsulate implementations into their own newtype. It probably needs some actual documentation at some point as well.

As an extension, maybe the DhKem trait should be promoted traits/kem as KEM, as KeyGen() -> (PrivateKey, PublicKey) is also part of the standard KEM model and provides a central trait for impl<k1, k2> KEM for TlsKemCombiner<k1, k2> where k1, k2: KEM. That way, we can just write type X25519Kyber768Draft00 = TlsKemCombiner<dhkem::X25519, MlKem768> once the ml-kem crate gets updated.

@tarcieri
Copy link
Member

This should document the scheme being implemented (there are at least two that are applicable: HPKE and TLS), and include test vectors for at least one curve.

For the encapsulator, you can test it decapsulates successfully.

I’m also not wild about names like *Proxy - we generally use Encapsulator and Decapsulator.

@incertia
Copy link
Contributor Author

Is there an elegant way of handling the case where no features are enabled? I could theoretically write

#[cfg(any(feature = "a", feature = "b", ...))]

but this is a pain to extend. I also don't think disabling tests for no features is a viable option as well.

@incertia
Copy link
Contributor Author

I think ultimately the DhKem trait should also be adjusted to be more generic if it is to be included into the kem crate, but this makes the trait slightly harder to use.

The main modification would be the addition of a PublicKey associated type, and the modification random_keypair() -> (Self::DecapsulatingKey, Self::PublicKey). The user would ultimately have to promote the DhKem::PublicKey to the DhKem::EncapsulatingKey either by directly wrapping it with dhkem::Encapsulator (or perhaps it would be ok to have PublicKey = EncapsulatingKey = Encapsulator<EcdhPublicKey> in these cases) but it would allow for X3DH to directly be specified as a KEM model

Comment on lines +84 to +106
/// This is a trait that all KEM models should implement, and should probably be
/// promoted to the kem crate itself. It specifies the types of encapsulating and
/// decapsulating keys created by key generation, the shared secret type, and the
/// encapsulated key type
pub trait DhKem {
/// The type that will implement [`Decapsulate`]
type DecapsulatingKey: Decapsulate<Self::EncapsulatedKey, Self::SharedSecret>;

/// The type that will implement [`Encapsulate`]
type EncapsulatingKey: Encapsulate<Self::EncapsulatedKey, Self::SharedSecret>;

/// The type of the encapsulated key
type EncapsulatedKey;

/// The type of the shared secret
type SharedSecret;

/// Generates a new (decapsulating key, encapsulating key) keypair for the KEM
/// model
fn random_keypair(
rng: &mut impl CryptoRngCore,
) -> (Self::DecapsulatingKey, Self::EncapsulatingKey);
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems somewhat similar to the EncappedKey trait from kem v0.2 which was removed in the latest prerelease: https://docs.rs/kem/0.2.0/kem/trait.EncappedKey.html

cc @rozbb

@incertia
Copy link
Contributor Author

incertia commented Apr 19, 2024

From my POV, I think the remaining TODOs are as follows:

  1. Merge Add SimpleKEM and FullKEM traits to kem traits#1559 once the API becomes somewhat finalized
  2. Add struct DhKem<X> and impl<X> SimpleKem for DhKem<X>. That way, usage for DHKEM is as simple as DhKem<NistP256>::encapsulate(...)
  3. Add some test vectors from HPKE. This probably involves creating a dummy rng that impls CryptoRngCore because I don't think the current EphemeralSecret API allows deserialization.

Regarding ZeroizeOnDrop, I don't think we need custom Drop implementations because the default one should just drop the inner value which will zeroize itself if I'm correctly understanding how drop semantics work.

@tarcieri
Copy link
Member

Going to merge this as a spike and make some follow-up changes

@tarcieri tarcieri merged commit 50d6017 into RustCrypto:master Jul 11, 2024
19 checks passed
@tarcieri tarcieri changed the title Add KEM providers for common ECDH schemes dhkem: add KEM providers for common ECDH schemes Aug 6, 2024
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.

None yet

2 participants