feature: implement blake3 hash and keyed hash modes - #641
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #641 +/- ##
==========================================
- Coverage 99.14% 99.05% -0.10%
==========================================
Files 108 112 +4
Lines 19961 20527 +566
==========================================
+ Hits 19791 20332 +541
- Misses 170 195 +25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
You were unlucky and hit a new Rust version which introduced a new lint which made the CI fail. So no fault at all on your part. If you rebase on current
This is a perfect approach, thank you for taking this into account! |
brycx
left a comment
There was a problem hiding this comment.
Thank you a lot for all this hard work, @jakub-mata! I get the feeling you were careful and put a lot of effort into it, which really tells.
Let me know if any of my review comments don't make sense or you believe you have a better approach.
| /// Regular hashing | ||
| Hash, | ||
| /// Hashing parametrized with a key. Useful for MACs. | ||
| KeyedHash { |
There was a problem hiding this comment.
This cannot simply be a raw array. We have newtypes for secret types that need to be used in order to make sure they aren't leaked or misused.
What we need here is something similar to how we handle it for the other MACs:
construct_secret_key! {
/// A type to represent the secret key that BLAKE2b uses for keyed mode.
///
/// # Errors:
/// An error will be returned if:
/// - `slice` is empty.
/// - `slice` is greater than 64 bytes.
///
/// # Panics:
/// A panic will occur if:
/// - Failure to generate random bytes securely.
(SecretKey, test_secret_key, 1, BLAKE2B_KEYSIZE, 32)
}Then we'd convert the key internally to words. That way we avoid forcing users to make a conversion, since secrets keys are usually handled as raw bytes.
Lore (not necessary):
This leads us to why the current BLAKE2b is split across two hazardous modules. Because MACs are by definition also sensitive types, that need constant-time handling and omitted debugs. But let's keep that out of scope for this PR for now, as long as we make the secret key the correct newtype.
If you have any ideas for nice abstraction between those two, I'm all open for suggestions. Feel free to ignore though.
There was a problem hiding this comment.
Those are the things I know in theory but don't apply in practice :) Hopefully, I've used the abstraction correctly.
There was a problem hiding this comment.
While you've correctly set up SecretKey, it's not being used here. Because Blake3 derives Debug, which Mode does as well and mode is a field for Blake3, a Debug call to Blake3 instance will leak the secret key here.
So this field Mode.key either needs to be SecretKey instead of array or the parsing/holding of the key needs to be removed entirely from this enum
| // The popped value is not manually deleted, only the counter is decreased. | ||
| fn pop(&mut self) -> ChainingValue { | ||
| self.stack_len -= 1; | ||
| self.stack[self.stack_len as usize] // Panics if out-of-bounds |
There was a problem hiding this comment.
Is it reasonable to expect this to panic at any time?
There was a problem hiding this comment.
stack_len is updated only in the push and pop functions so the panics do not occur. I removed the comment.
|
|
||
| reader.fill(out_slice); | ||
| } | ||
| } |
There was a problem hiding this comment.
We want to add a test here at the bottom, that implements StreamingContextConsistencyTester found here: src/test_framework/incremental_interface.rs.
This is pretty crucial for testing the correctness of incremental handling of inputs.
There was a problem hiding this comment.
Done, but I've used a tiny workaround. My finalize() function consumes the hasher, which I find quite nice as it disallows any further usage. However, the testing trait expects to be able to call it, which in my case just cannot happen (I suppose the usage of mut& self is expected with an internal flag, something like is_finalized). Instead of changing the API, there is a special testing struct which has this flag and an inner hasher to work around it.
I hope that's fine.
There was a problem hiding this comment.
I completely understand your preference for this type of API, there's less testing even. But it does go against every other streaming API exposed in Orion, which isn't super user-friendly either. I'll have to think about this a bit more.
| let take = min(want, data.len()); | ||
| self.chunk.update(&data[..take]); | ||
|
|
||
| self.update(&data[take..]) |
There was a problem hiding this comment.
This recursive handling here worries me a bit. In release mode, I don't think this would overflow the stack, but in debug it could (not a argument to not remove recursion in crypto-code). If we ever start testing large inputs, this would eventually SIGABRT, depending on the stack for the test thread AFIU.
Have you looked at how we handle this with while !data.is_empty(), @jakub-mata? I think that approach would be more robust than this recursion-based one.
There was a problem hiding this comment.
That's reasonable, I've removed the recursion here and in the update() function inside ChunkState (called on line 112 here). A while loop is used instead.
The new version implements the `Drop` trait for various structs, where zeroization occurs. `Blake3`, which contains the secret key used for a keyed hash (if applicable). ChunkState and TreeStack also implement it, as they contain the key, its traces, or internal state.
brycx
left a comment
There was a problem hiding this comment.
I'll have to think about what to do with the mut self and &mut self a bit more.
The rest is just adding omitted debugs where you've added the zeroization.
|
|
||
| #[derive(Debug, Clone)] | ||
| pub(crate) struct TreeStack { | ||
| stack: [ChainingValue; MAX_TREE_DEPTH], |
There was a problem hiding this comment.
If the stack is zeroized, then it also needs to be an omitted debug.
| chunk_counter: u64, | ||
| /// Input buffer for the next block. When full, it is compressed | ||
| /// and flushed. | ||
| block: [u8; BLOCK_LEN], |
There was a problem hiding this comment.
If secret data flows into here and is zeroized it also needs omitted debug.
| #[derive(PartialEq, Debug, Clone)] | ||
| pub(crate) struct ChunkState { | ||
| /// The resulting chaining value of the last compressed block. | ||
| cv: ChainingValue, |
There was a problem hiding this comment.
If secret data flows into here and is zeroized it also needs omitted debug.
| /// Regular hashing | ||
| Hash, | ||
| /// Hashing parametrized with a key. Useful for MACs. | ||
| KeyedHash { |
There was a problem hiding this comment.
While you've correctly set up SecretKey, it's not being used here. Because Blake3 derives Debug, which Mode does as well and mode is a field for Blake3, a Debug call to Blake3 instance will leak the secret key here.
So this field Mode.key either needs to be SecretKey instead of array or the parsing/holding of the key needs to be removed entirely from this enum
|
|
||
| reader.fill(out_slice); | ||
| } | ||
| } |
There was a problem hiding this comment.
I completely understand your preference for this type of API, there's less testing even. But it does go against every other streaming API exposed in Orion, which isn't super user-friendly either. I'll have to think about this a bit more.
This PR introduces the BLAKE3 hash function into the
/hazardousmodule, implemented according to the official BLAKE3 specifications.Features Included:
Note: The "derive key" mode has been omitted from this PR to keep the scope focused on standard hashing. The underlying state machine has been structured so that key derivation can be easily added in a future PR if desired.
Testing includes:
This initial implementation establishes a single-threaded baseline. Because BLAKE3's tree structure is designed for infinite parallelism, a natural next step will be adding a data-parallelism feature flag to support multi-threaded hashing for large inputs.
Resolves #394