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

Added zeroize to blake2_simd #449

Closed
wants to merge 5 commits into from

Conversation

laudiacay
Copy link

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

@laudiacay laudiacay mentioned this pull request Jan 28, 2023
@laudiacay
Copy link
Author

@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
Copy link
Member

@laudiacay are you talking about these?

error[E0658]: const generics are unstable
   --> /home/runner/.cargo/registry/src/github.aaakk.us.kg-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.aaakk.us.kg-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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

@newpavlov newpavlov closed this Jan 11, 2024
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.

3 participants