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

Back implementations of SHA2, HMAC-SHA1, HMAC-SHA2 and HKDF-SHA2 by hacl-rs #659

Merged
merged 32 commits into from
Nov 14, 2024

Conversation

keks
Copy link
Member

@keks keks commented Nov 6, 2024

This PR replaces some use of hacl-c by hacl-rs.

The Rust files in the directories libcrux-hacl-rs and libcrux-hacl-rs-xxx are autogenerated, with two caveats:

  • some have been rustfmt'd
  • I added some use items to modules inside the same crate in some cases
  • Some modules paths changed when copying them into this crate, so I had to adapt that sometimes.

The 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 their bignum crate, and I move that crate into a module bignum inside our libcrux-hacl-rs crate, this becomes crate::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”.

@keks keks marked this pull request as draft November 6, 2024 11:33
@franziskuskiefer franziskuskiefer mentioned this pull request Nov 6, 2024
12 tasks
@franziskuskiefer franziskuskiefer linked an issue Nov 6, 2024 that may be closed by this pull request
@keks keks marked this pull request as ready for review November 6, 2024 14:29
@franziskuskiefer franziskuskiefer changed the title Back implementations of SHA2, HMAC-SHA1, HMAC-SHA2 and HKDF-SHA2 by hacl-rsh Back implementations of SHA2, HMAC-SHA1, HMAC-SHA2 and HKDF-SHA2 by hacl-rs Nov 6, 2024
@keks keks mentioned this pull request Nov 6, 2024
@keks keks marked this pull request as draft November 7, 2024 11:02
@franziskuskiefer franziskuskiefer marked this pull request as ready for review November 7, 2024 16:22
Copy link
Member

@franziskuskiefer franziskuskiefer left a 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.

sizes Outdated Show resolved Hide resolved
ml-kem.debug.objdump Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
libcrux-curve25519/Cargo.toml Outdated Show resolved Hide resolved
libcrux-sha2/Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@keks
Copy link
Member Author

keks commented Nov 11, 2024

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 hacl feature is set (which it is by default, but that might change in the future).

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: portable_hacl. The idea here is that it indicates that "the hacl code is used to provide a portable implementation". The idea is that we can then add e.g. an "avx_jasmin" or so to provide non-portable implementations, and then have code that does feature detection at runtime. Not sure that is valuable, but that is the idea behind the feature.

I also rebased on main.

Copy link
Member

@franziskuskiefer franziskuskiefer left a 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.

hacl-rs/src/bignum.rs Outdated Show resolved Hide resolved
curve25519/src/impl_hacl.rs Show resolved Hide resolved
ed25519/src/lib.rs Show resolved Hide resolved
hacl-rs/src/lib.rs Show resolved Hide resolved
hacl-rs/src/lib.rs Show resolved Hide resolved
Copy link
Member

@franziskuskiefer franziskuskiefer left a 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.

curve25519/src/impl_hacl.rs Show resolved Hide resolved
ed25519/src/impl_hacl.rs Outdated Show resolved Hide resolved
ed25519/src/impl_hacl.rs Outdated Show resolved Hide resolved
ed25519/src/lib.rs Show resolved Hide resolved
hacl-rs/src/bignum.rs Show resolved Hide resolved
@keks keks enabled auto-merge November 14, 2024 10:42
@keks keks added this pull request to the merge queue Nov 14, 2024
Merged via the queue into main with commit 04fdf14 Nov 14, 2024
52 checks passed
@keks keks deleted the keks/hacl-rs branch November 14, 2024 12:25
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.

Run Wycheproof tests on libcrux-hkdf and libcrux-hmac
2 participants