-
Notifications
You must be signed in to change notification settings - Fork 15
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
Back implementations of SHA2, HMAC-SHA1, HMAC-SHA2 and HKDF-SHA2 by hacl-rs #659
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it was possible to move the curve code into the separate crate? We should document where it's the case that this isn't possible, and why.
There's a bunch of dead code in the hacl-rs crate now that we should drop.
There's no inline anywhere on functions right now. I thought they were generating that. We should add them manually to get proper speed. I'll also ask them where the inlines are.
Some things that need to get fixed in multiple places
- All dependencies need versions
- Crates need
description
fields - The crates should have an API that is independent of hacl. Right now there's only a
hacl
implementation for some of the crates. It's probably enough to just drop the hacl branding.
We should see how to get to better no_std
support. We should put allocations behind a feature at least. But later, not for this PR.
Alright, I added all the versions and description fields and renamed the crates. I also made it so the hacl-rs crate only contains code that is accessed by at least two actual crates. That means that most of the algorithm-specific crate code is now in the actual crates, and it is only exposed if the Note that one outlier here is curve25519, which has the implementation in the hacl-rs crate, because ed25519 also depends on it. I suppose ed25519 could also depend on curve25519 - Let me know if you prefer that and I'll make the change. I also introduced another feature in the crates: I also rebased on main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, only a few things we need to fix to get this first version in.
Follow-ups, please transfer to sub issues in #648 where there isn't one already
- Add labels for verification status. All the crates here should get verified, potentially with a hacl-rs note.
- The code is missing all the inlines. That's also why you didn't run into build performance issues. We should annotate all functions with a must inline in the hacl-rs code. Otherwise this code will be slow.
- Related: Let's make sure to have basic benchmarks on everything. There are benchmarks for most things on the main libcrux crate that we could re-use.
- Clearly document what's hand written and what's generated code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some CI jobs are failing.
But when that's fixed, let's get this in and do everything else in follow ups.
Co-authored-by: Franziskus Kiefer <franziskuskiefer@gmail.com>
This PR replaces some use of hacl-c by hacl-rs.
The Rust files in the directories
libcrux-hacl-rs
andlibcrux-hacl-rs-xxx
are autogenerated, with two caveats:use
items to modules inside the same crate in some casesThe reason for the last two items is that upstream they define several crates, and here I wanted to keep the number crates low. So when e.g. they use
crate::bignum_base
in theirbignum
crate, and I move that crate into a modulebignum
inside ourlibcrux-hacl-rs
crate, this becomescrate::bignum::bignum_base
.Maybe we should ask if they can put everything in a single crate (except the proc-macro of course), or maybe we can split these into separate crates. Not sure what is better.
In some places it also changed the API a little:
Everywhere
Pass in &mut [u8; N] instead of returning [u8; N]
Seems like it’s the more general thing, even though the API is a bit more clunky. We can implement the returning API on top of the borrowing API, as well.
HKDF
Trait and Structs
Use structs and traits for the implementations rather than modules. Somehow doing it this way seemed cleaner to me, but I can understand if that is not how it should be. More of a conversation starter, the old structure is still around. Happy to hear feedback around this.
Renamed “tag length” (in comments, $tag_len, …) to “hash length”
That’s what the RFC calls it.
fixed some comments on panics
removed one of the two errors
Until now, we had (a) libcrux_hkdf::Error and (b) libcrux_hkdf::hacl_hkdf::Error. However, (a) did not contain all error conditions, but was the only that was exposed, so we returned “too long okm” when in fact the input buffer was too large. Now we just have a single error, with both error conditions, and that’s just used everywhere.
HMAC
Nothing besides the change from “return array” to “take &mut”.
SHA2
Removed the additional layering between the public facing types and the state types
Previously, it looked like the inner type would only hide the unsafe-ness of the hacl-c implementation, and then we would just forward most of the API to the used in a wrapper around that. Now that this is safe, the utility became even more questionable, so I got rid of it. The digest module now uses hacl-rs directly.
ed25519
Nothing besides the change from “return array” to “take &mut”.