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

Improve API for hash2curve functions #876

Merged
merged 7 commits into from
Jan 11, 2022

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jan 7, 2022

  • Allow methods to accept multiple data/msgs by using &[&[u8]], á la Hkdf::expand_multi_info().
  • Implement hash_to_scalar, got an implementation for P-256 already working with test vectors.
  • Remove 'static requirement for dst.
    Maybe we don't want hash_to_scalar on GroupDigest, because it also pulls in arithmetic for no real reason.

The goal here is to make this usable for voprf. VOPRF sometimes builds complex msgs that are impossible to concat without allocation, this is why the multi-message API was necessary. I would even prefer just to take an Iterator<Item = &[u8]>.

The 'static requirement doesn't work for VOPRF because it needs to build the dst from multiple parts that are party derived from associated types from traits, this can't be used in const to build a &'static [u8] in Rust right now.

CC @tarcieri @mikelodder7

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 7, 2022

I came up with three solutions how to remove the 'static requirement from dst:

  1. Instead of letting ExpandMsgXmd hold the dst, we can pass it back in again every time we call fill_bytes.
  2. Split ExpandMsg and it's data into two separate traits.
  3. Instead of &'static [u8], pass a GenericArray<u8>.

I'm leaning towards the second solution, it seems to be the friendliest one for the user.

elliptic-curve/Cargo.toml Outdated Show resolved Hide resolved
@tarcieri
Copy link
Member

tarcieri commented Jan 8, 2022

This seems like an improvement to me but with the caveat I haven't carefully read the spec

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 8, 2022

https://www.ietf.org/archive/id/draft-irtf-cfrg-voprf-08.html#section-3.3.1.1-1
For example, takes input, which has a user defined size, and needs to concatenate multiple things to build the context, no way to do that without allocation really.

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 8, 2022

I implemented the second solution, as explained above, for removing the 'static requirement from dst. This lifetime can be elided by users if they use a &'static dst, so the API for the user is the same really.

@daxpedda
Copy link
Contributor Author

I just realized, one solution could be to remove the whole Expander logic and just add a &mut out, like we had to do in hash_to_field anyway.

@daxpedda
Copy link
Contributor Author

I just realized, one solution could be to remove the whole Expander logic and just add a &mut out, like we had to do in hash_to_field anyway.

I decided to revert this, there is no real advantage and the current Expander logic is much more powerful actually.

@tarcieri
Copy link
Member

I'm a bit unclear what lifetime relationship the 'a parameters here are trying to describe. I suppose I could check out this branch, attempt to remove them, and see what the borrow checker complains about...

(Of course, I don't understand the impetus for the original 'static lifetimes either...)

@daxpedda
Copy link
Contributor Author

daxpedda commented Jan 10, 2022

(Of course, I don't understand the impetus for the original 'static lifetimes either...)

The issue arises when you want to implement a spec that requires different dsts depending on something that you want to make generic, for example the group.

To be more specific, VOPRF requires a different dst depending on which group you are using, let me give you an example:

impl VoprfParameters for NistP256 {
    const ID: u16 = 0x0003;
    type Hash = sha2::Sha256;
}

// https://www.ietf.org/archive/id/draft-irtf-cfrg-voprf-08.html#appendix-A-6
fn derive_key_pair<G: VoprfParameters, H: Digest>(seed: &[u8]) -> ... {
    // contextString = "VOPRF08-" || I2OSP(modeBase, 1) || I2OSP(suite.ID, 2)
    let context_string = concat!("VOPRF-08", 0, G::ID);
    // skS = GG.HashToScalar(seed, DST = "HashToScalar-" || contextString)
    let sk_s = NistP256::hash_to_scalar::<ExpandMsgXmd<H>>(seed, context_string)?;
    ...
}

This will fail, because G::ID isn't a literal. As far as I'm aware there is no other way to do this.


As for the 'a lifetime, the reason it is needed is because currently ExpandMsg returns an Expander, which needs to hold a reference to the dst, to do that it requires a lifetime. Now that I think about it though, I think the lifetime can still be removed in a couple of places.

EDIT: I attempted, but it wasn't possible.


I hope this clears up any questions. Please hit me up if there is anything unclear.

@daxpedda
Copy link
Contributor Author

If the lifetime is a big issue, I could go back to the solution of removing the Expander. To do this we have to replace the len_in_bytes with a &mut out. But I really think that the lifetime is not an issue here, as users don't actually have to specify it if their dst is 'static.

@tarcieri
Copy link
Member

This will fail, because G::ID isn't a literal. As far as I'm aware there is no other way to do this.

Yeah, that's not going to work without a heap allocation. The alternative IMO would be to have an &str associated type and compute the entire string in advance. It could possibly be assisted by a macro.

As for the 'a lifetime, the reason it is needed is because currently ExpandMsg returns an Expander, which needs to hold a reference to the dst, to do that it requires a lifetime.

But that should only require a lifetime on Expander, not ExpandMsg? Or at the very least, the lifetime can be on the ExpandMsg::expand_message method rather than the trait itself?

@daxpedda
Copy link
Contributor Author

But that should only require a lifetime on Expander, not ExpandMsg? Or at the very least, the lifetime can be on the ExpandMsg::expand_message method rather than the trait itself?

I thought so too, but couldn't make it work sadly. Feel free to try.

@tarcieri
Copy link
Member

tarcieri commented Jan 10, 2022

Aah I see, you need the lifetime for the associated Expander type. Never mind then!

@tarcieri
Copy link
Member

@mikelodder7 any thoughts on this?

@mikelodder7
Copy link
Contributor

The lifetime is solely for the dst. I put it as static so I wouldn’t have to copy the dst to a GenericArray of a fixed size less than 255. If we can somehow manage the lifetime without static then great but it pollutes all interfaces when ExpandMsgXof doesn’t even need it. If we had a heap then you could just convert to Vec and be done.

@mikelodder7
Copy link
Contributor

It seems we’re “crossing the streams” with voprf and hash2curve with multiple args but I guess it’s probably okay for now

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.

3 participants