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

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

Merged
merged 21 commits into from
Jan 10, 2019

Conversation

DebugSteven
Copy link
Collaborator

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
Copy link
Contributor

Could the build failure be related to rust-random/rand#645 ?

src/lib.rs Outdated Show resolved Hide resolved
@hdevalence
Copy link
Contributor

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.

src/x25519.rs Outdated
fn mul(self, secret: &'b EphemeralSecret) -> EphemeralPublic {
EphemeralPublic(self.0 * secret.0)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to expose this trait impl if the diffie_hellman function exists? My guess is that it's probably not necessary.

src/x25519.rs Outdated

/// A DH SharedSecret
#[repr(C)]
#[derive(Default)] // we derive Default in order to use the clear() method in Drop
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really necessary to derive Default (instead of just calling self.0.clear() in the Drop impl)?

It seems like it would be better not to implement Default, so that the only way to create a SharedSecret is as the output of a DH function.

src/x25519.rs Outdated

/// View this shared secret key as a byte array.
#[inline]
pub fn as_bytes<'a>(&'a self) -> &'a [u8; 32] {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think these lifetime specifiers might be redundant

src/x25519.rs Outdated Show resolved Hide resolved
src/x25519.rs Outdated
/// an ephemeral secret key and montegomery point as input and
/// a shared secret as the output.
pub fn diffie_hellman(&self, their_public: &EphemeralPublic) -> SharedSecret {
SharedSecret(x25519(&self.0, &MontgomeryPoint(*their_public.0.as_bytes())))
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that there's an EphemeralPublic type (which already holds a MontgomeryPoint), there's no need to copy it into a new point and call the x25519 function ... in fact the entire x25519 function could be removed and combined into the diffie_hellman function.

Also, if diffie_hellman takes self rather than &self, then an EphemeralSecret can't be used more than once.

Cargo.toml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
benches/x25519.rs Outdated Show resolved Hide resolved
src/x25519.rs Outdated Show resolved Hide resolved
@hdevalence
Copy link
Contributor

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:

  • Since EphemeralSecrets can only be created by their constructor, the bit-clamping invariant can be enforced by calling clamp_scalar in new, and the diffie_hellman implementation can be simplified to
pub fn diffie_hellman(self, their_public: &EphemeralPublic) -> SharedSecret {
    SharedSecret(self.0 * their_public.0)
}
  • The repr(C)s on the ephemeral types can be eliminated (since if someone wants FFI, they can use the "bare" byte-oriented function);

@hdevalence
Copy link
Contributor

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

hdevalence and others added 5 commits December 15, 2018 17:41
Co-Authored-By: DebugSteven <debugsteven@gmail.com>
Co-Authored-By: DebugSteven <debugsteven@gmail.com>
Co-Authored-By: DebugSteven <debugsteven@gmail.com>
@hdevalence hdevalence added this to the 0.4.0 milestone Dec 29, 2018
Copy link
Contributor

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

LGTM

@hdevalence hdevalence merged commit 1dcab60 into dalek-cryptography:develop Jan 10, 2019
@DebugSteven DebugSteven deleted the develop branch January 10, 2019 19:50
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.

2 participants