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

Add pure-Rust Blake2 #755

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add pure-Rust Blake2 #755

wants to merge 5 commits into from

Conversation

keks
Copy link
Member

@keks keks commented Jan 16, 2025

This PR adds the pure-rust implementation from hacl-rs. So far this only features a low-level byte-oriented API.

One thing I am not sure about is the last_node flag. From reading the spec I think it maybe should be set to true, but that makes it produce wrong/inconsistent results.

It also reduces the visibility of some hacl-related submodules of ed25519, that is an unrelated cleanup change.

@keks keks requested a review from a team as a code owner January 16, 2025 15:51
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.

Just a quick drive-by. I'll look at it tomorrow.

@@ -0,0 +1,13 @@
//#![no_std]
Copy link
Member

Choose a reason for hiding this comment

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

Is it supposed to be no_std? alloc is used unconditionally right now, so it won't be no_std without changes.

@@ -0,0 +1,6 @@
pub fn memzero<T: Copy>(x: &mut [T], len: u32) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use this memzero. It won't actually do anything. Let's throw in proper zeroization into a new crate that we already have on the secret ints branch. Or not do it at all. But using this looks bad.


/// Constructs the [`Blake2b`] hasher.
pub fn build(self) -> Blake2b<KEY_LEN, OUT_LEN> {
// these are safe because they bot are at most 64
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// these are safe because they bot are at most 64
// these are safe because they both are at most 64

}

/// A hasher struct for the Blake2b (optionally keyed) hash function.
pub struct Blake2b<const KEY_LEN: usize, const OUT_LEN: usize> {
Copy link
Member

Choose a reason for hiding this comment

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

To start the conversation about how to add high level APIs, can you add a generic function on top here where the output lenght OUT_LEN is not known at compile time?


/// Constructs the [`Blake2s`] hasher.
pub fn build(self) -> Blake2s<KEY_LEN, OUT_LEN> {
// these are safe because they bot are at most 32
Copy link
Member

Choose a reason for hiding this comment

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

This is a public function, so how can you know that a caller won't use something larger than 32?

let key_length = KEY_LEN as u8;
let digest_length = OUT_LEN as u8;

// NOTE: I am not entirely sure that this is the correct value. From reading the spec I
Copy link
Member

Choose a reason for hiding this comment

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

false is correct here. Aymeric wrote some notes when adding tests for true to hacl. hacl-star/hacl-star#934. I don't think we need to expose the true version 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.

2 participants