RustCrypto / hashes

Collection of cryptographic hash functions written in pure Rust
1.82k stars 247 forks source link

Added zeroize to blake2_simd #449

Closed laudiacay closed 8 months ago

laudiacay commented 1 year ago

This code is much cleaner (although a bit repetitive...). Zeroize added! 🖖

laudiacay commented 1 year ago

@tarcieri zeroize's unstable features seem to be angering the inliner ... not sure what you want me to do with this, just let me know what's preferable for the crate's users

tarcieri commented 1 year ago

@laudiacay are you talking about these?

error[E0658]: const generics are unstable
   --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/zeroize-1.5.7/src/lib.rs:353:15
    |
353 | impl<Z, const N: usize> Zeroize for [Z; N]
error[E0599]: no associated item named `MAX` found for type `isize` in the current scope
   --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/zeroize-1.5.7/src/lib.rs:431:32
    |
431 |         assert!(size <= isize::MAX as usize);
    |                                ^^^ associated item not found in `isize`

The MSRV of zeroize is 1.51, but you are testing on 1.41, so it won't compile (and isn't expected to).

laudiacay commented 1 year ago

So should I just remove the 1.41 tests or configure the crate to only zeroize with a feature, and then only run the 1.41 tests without that feature?

newpavlov commented 1 year ago

It may be worth to wait for the next breaking release cycle in which we bump MSRV to 1.57. If you absolutely need zeroization for blake2 v0.10, then you would need to reconfigure the CI job similarly to how we do it for oid features (e.g. see the sha2 job).

newpavlov commented 1 year ago

Also I don't think that zeroize support should be enabled by default, similarly to other crates it should be behind disabled-by-default crate feature.

laudiacay commented 1 year ago

I don't need zeroize myself- I'm just a little bored and contributing to this after work for fun. If you have another "good first issue" let me know- tarcieri suggested this one.

I'll make it a crate feature, give me a second... :)

newpavlov commented 1 year ago

Depending on your knowledge, it could be a good project to migrate the asm-hashes to inline assembly, similarly to #447. Also you could try to implement algorithms as per #1 (you could see other repos for similar issues).

laudiacay commented 1 year ago

featurified. Going to go look at the inline assembly one, that should be a fun learning experience, I've only done that in C and solidity before...

newpavlov commented 8 months ago

blake2 implementation probably will be replaced before v0.11 is published (see #228). Also see #545.