Hash to curve as defined in the standard #377
Conversation
|
I see in #438 that the function |
|
Yes, thank u for following up. I do want to merge this but I haven't gotten a chance to do a proper review. We really need to get to a 4.0 release, so I think this might have to wait until 4.1. Thank you so much for this contribution. I promise it will get the attention it deserves. |
|
Great, thanks! And whenever you think you have time, you can ping me here, and I'll rebase (as there's been some changes in the hashing functions) and make sure it follows the latest version of the standard 👍 Thanks for working on this 🙏 |
|
It would also be interesting if someone could attempt to impl the |
|
Happy to do it 👍 given that there is no rush, I should be able to dedicate a few cycles in a reasonable amount of time. |
|
Well for starters, it's full of merge conflicts |
|
Rebase done 👍 Happy to make any further changes if necessary. |
|
Thank you very much @iquerejeta (I have also started rebasing your PR on Fri but you were quicker 😄 ) |
|
@tarcieri @rozbb @oleganza can we do with @iquerejeta something more ? We are working on VRF library based on dalek-cryptography and basing atm on this branch and we would like to have just master as dependency rather than branch. And also humbly thinking this is a good addition 😄 Cheers! |
|
Apologies for pestering you @tarcieri but we (Amaru team) ultimately depend on this PR to be included in a proper version of curve255159-dalek in order to simplify our dependencies on VRF library and also to be able to request a proper audit on a stable code base. |
|
I'll also note this looks fine to me at first glance, but I'd prefer one of us go through it in detail |
| .expect("Montgomery conversion to Edwards point in Elligator failed"); | ||
|
|
||
| // Now we recover v, to ensure that we got the sign right. | ||
| let mont_v = |
There was a problem hiding this comment.
Maybe silly question: where do these steps come from precisely? I'm following appendix F.3 and it looks like encode_elligator2 is doing up to step 11. So this is doing step 19 by recomputing some values? Why precisely is mont_v set this way if it's supposed to be sqrt(y2) per step 17?
Sorry, I'm not the most familiar with these algorithms because I didn't write the original version
There was a problem hiding this comment.
There are a few ways this is done. In libsodium, for example, they compute v via a square root, pick the sign accordingly, and then convert to Edwards using that sign (see here).
What we do is different: note that is_square tells us the sign of the Montgomery v coordinate, not the Edwards X. However, the to_edwards function only lets us specify the sign of X, so we:
- Call
to_edwardswith an arbitrary sign (in our case, 0), - Recompute
vfrom theuand the EdwardsXusing the mapping formula (recall thatX = ED25519_SQRTAM2 * u / v) - Then flip the sign of
Xif the resulting sign ofvdoesn't matchis_sq ^ v.is_negative().
This avoids using square roots and works with our existing conversion interface (the current interface would not allow us to do it the square root way, because there is no Montgomery -> Edwards conversion that takes both (u, v) as input). We could add code that enables it—happy to benchmark it.
There was a problem hiding this comment.
Sorry, my brain is a bit fried but I'll try to understand better. To be clear, my absolute top concern is maintainability. It would be way easier for me if all the formulas in this PR used notation directly from the spec. Do you think that'd be possible? I don't mind some added code (eg the conversion method you mentioned) to get there. I'll even accept a performance hit if necessary. I think @tarcieri would agree
There was a problem hiding this comment.
I'd be happy to do that. I'm wondering if it makes sense to change the elligator_encode function directly, and perform elligator as specified in the standard. What I have in mind is that we implement the straight line implementation they present in the standard (here. And then we have two functions, an elligator2 for montgomery, and an elligator2 for edwards. WDYT?
There was a problem hiding this comment.
That sounds wonderful, thank you!!
There was a problem hiding this comment.
I started trying to do this, and there's some functionality missing. The only pow implementation is pow2k. Following the inline implementation we need to compute
y11 = tv2^c4
where
c4 = (q - 5) / 8
with c4 not being a power of 2 (see step 16 here). Don't know if I'm missing a trick here. I think at this point it would be easier to properly document, rather than implementing new functions. If you are happy with that path I can add proper documentation to the function, and leave it as is.
There was a problem hiding this comment.
Ah, good catch. Yeah I think it'd be fine to document the parts that deviate. In my mind, the bar is to have it "so someone with a a highschool math background can see that these routines are clearly the same". It's kinda vague, but I'm sure we can converge on something
There was a problem hiding this comment.
I've added a comment. I decided to link this section, instead of the inline section, as I believe its easier to follow along. Let me know what you think.
There was a problem hiding this comment.
I think I can follow that, ty
|
@rozbb Would it be possible to get approval to run workflows so that we can check CI is passing? |
curve25519-dalek/src/field.rs
Outdated
| { | ||
| let len_in_bytes = 48; | ||
| let l_i_b_str = [0u8, len_in_bytes]; | ||
| let z_pad = [0u8; 128]; |
There was a problem hiding this comment.
The spec says:
Z_pad = I2OSP(0, s_in_bytes)
Where s_in_bytes is:
- s_in_bytes, the input block size of H, measured in bytes (see
discussion above). For example, for SHA-256, s_in_bytes = 64.
128 is correct for SHA-512, but it isn't for any arbitrary hash. E.g. plugging in SHA3-512 would work but produce non-spec compliant results.
I've no idea how to solve this here without generic-array or hybrid-array:
| let z_pad = [0u8; 128]; | |
| let z_pad = [0u8; { D::BlockSize::USIZE }]; |
This doesn't work AFAIK.
Alternatively D could be restricted to BlockSizeUser<BlockSize = U128>.
There was a problem hiding this comment.
I've gone the BlockSizeUser way. Let me know if that works.
There was a problem hiding this comment.
You have used a Vec, which works but now the crate feature digest requires alloc.
I've just realized that digest already depends on generic-array and it also re-exports it. So you can actually use generic-array here.
There was a problem hiding this comment.
Now that we do handle BlockSizeUser correctly, we also need to enforce further requirements from the spec:
For correctness, H requires b <= s.
This can be done via:
BlockSizeUser<BlockSize: Greater<D::OutputSize, Output = True>>
|
@iquerejeta there are some deprecations you'll need to fix |
|
My bad 🙏 forgot about those in the rebase. |
|
Thank you, and sorry for the delay. Will make time for this tomorrow |
curve25519-dalek/src/field.rs
Outdated
|
|
||
| #[test] | ||
| #[cfg(feature = "digest")] | ||
| fn from_hash_wide() { |
There was a problem hiding this comment.
One last question I promise: Where are these test vectors from?
There was a problem hiding this comment.
I must admit that I do not remember. It's been a while. I was looking for those test vectors in the libsodium implementation or some drafts (which were the most likely sources) but couldn't find any.
I'm happy to regenerate them from any source you'd like and document it accordingly.
|
Ok, just cleaned up docs and clarified the origin of some KATs. I left just 1 question, namely where the from_bytes_wide test vectors came from. Other than that, I think we're good to go! |
|
This is confusing to me. It seems the |
|
What should we do about the fiat backend? The logic of the from_bytes_wide is not from fiat crypto 🤔 |
|
@iquerejeta Well my question is: what was it doing when the build was succeeding? Shouldn’t the fiat build have always been failing? |
|
Yeah, that's a good question. I have no idea why. I believe we never had an implementation of from_bytes_wide for fiat. |
|
Regarding |
|
Thank you @tarcieri, that gave me the idea to make Do you wanna do one last look-over? I had to bump the |
|
Thank you so much @iquerejeta @tarcieri and @daxpedda !! |
|
🎉 Thank you everyone! Looking forward to next release so that we can release our stuff too :) |
|
🎉 Thanks everyone! 🙏 |
* Implementation of `hash_to_field` as defined in the standard * Implementation of `hash_to_curve` as defined in the standard, by changing the mechanism over which we chose the sign. * For the point above, had to change the `elligator_encode` to return whether `eps` is a square or not (required for `hash_to_curve`). * Included test vectors of the draft. * Included `FieldElement::from_bytes_wide(bytes: &u8; 64])` to reduce integers encoded in 64 bytes.
The current implementation is not compatible with the current definition of the standard. This PR provides a hash-to-curve implementation as defined in draft-irtf-cfrg-hash-to-curve-12.
hash_to_fieldas defined in the standardhash_to_curveas defined in the standard, by changing the mechanism over which we chose the sign.elligator_encodeto return whetherepsis a square or not (required forhash_to_curve).FieldElement::from_bytes_wide(bytes: &u8; 64])to reduce integers encoded in 64 bytes.