dalek-cryptography / x25519-dalek

X25519 elliptic curve Diffie-Hellman key exchange in pure-Rust, using curve25519-dalek.
BSD 3-Clause "New" or "Revised" License
328 stars 132 forks source link

Create new types for keys and clear values on drop #15

Closed DebugSteven closed 5 years ago

DebugSteven commented 5 years ago

PR for issue #9

Created Ephemeral & SharedSecret types to use for keys. diffie_hellman, generate_public, & generate_secret are now methods on Ephemeral. Both types use clear in their respective drop implementations & Ephemeral has an implementation for Mul.

hdevalence commented 5 years ago

Could the build failure be related to https://github.com/rust-random/rand/issues/645 ?

hdevalence commented 5 years ago

Moving the protocol flow into types looks great!

One thing I notice is that the ephemeral secret key (a Scalar) is encapsulated in Ephemeral struct, but the public key (a MontgomeryPoint) isn't. It seems like the types would match the protocol better if Ephemeral was renamed to EphemeralSecret, and the MontgomeryPoint was encapsulated in an EphemeralPublic.

Then the EphemeralSecret can have a new(csprng: &mut T) (instead of generate_secret, since the secret is part of the type already), the generate_public can become a From impl (since it's really a conversion, not generating something new), and the SharedSecret type can have an as_bytes to get the secret bytes at the end (right now there's no way to access the bytes of a SharedSecret). Then all the other methods could be removed -- for instance, it's not necessary to inspect the bytes of an EphemeralSecret, or to save it etc.

hdevalence commented 5 years ago

Hey, sorry for not having bandwidth for this until now. I think that the ephemeral DH API looks good. You asked (out-of-band) about whether the function should be provided as in the RFC or whether the same functionality should be provided by an API which is different than the way that the RFC is written.

One point is that the ephemeral DH API, as you've implemented it, forces the user to actually do ephemeral DH, only using the secret key once (since it's consumed by the DH function). This is stricter than what the x25519 function in the RFC allows. So maybe the best thing is to provide two "levels" of functionality: the ephemeral DH API, which is oriented around using Rust types to express protocol invariants (like no reuse of secret keys), as well as a bare, byte-oriented x25519 function that operates on raw bytes (and matches the RFC's spec more closely):

pub fn x25519(k: [u8; 32], u: [u8; 32]) -> [u8; 32] {
    (clamp_scalar(k) * MontgomeryPoint(u)).to_bytes()
}

Then this separation allows a few simplifications:

hdevalence commented 5 years ago

note: with those changes the RFC tests need to be using the bare x25519 function, not the ephemeral API