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 cSHAKE128 and cSHAKE256 implementations #355

Merged
merged 3 commits into from May 28, 2022
Merged

Add cSHAKE128 and cSHAKE256 implementations #355

merged 3 commits into from May 28, 2022

Conversation

jvdsn
Copy link
Contributor

@jvdsn jvdsn commented Jan 27, 2022

Based on pull request #325 from @elichai. I was planning on extending their branch and creating a merge commit first, to not lose the Git history, but unfortunately that proved too difficult.

This implementation uses digest v0.10.0. It differs in some places from the implementation in #325, most importantly:

  • I used a reset feature to indicate whether reset support should be compiled in.
  • The new_with_function_name accepts a function name argument. This results in some overhead for the cSHAKE initialization, but normally the initialization shouldn't be performed a lot anyway. Even though the end user will probably never need it, I didn't want to omit that functionality should it ever be necessary in the future. Though perhaps new_with_function_name could be made private for now?

Some additional notes:

  • Because CShake128 and CShake256 don't implement Default, the easiest way to construct an instance is using CShake128::from_core(CShake128Core::new(customization)) which is quite verbose. I didn't find a way to make this shorter in the current trait typesystem. Perhaps that's something that could be improved in digest.
  • Similarly, the xof_reset_test function doesn't apply here because the structs don't implement Default.
  • I didn't add benchmarks, I don't have a nightly Rust version installed. Feel free to add them.

The test vectors are still from https://github.com/damaki/libkeccak/tree/master/tests/kat/testvectors/cSHAKE together with the current SHAKE tests for an empty customization.

Once again, many credits to @elichai.

@aewag
Copy link
Contributor

aewag commented Jan 27, 2022

FYI: As a temporary fix for cross you could cherry-pick 10f5b73 (see #354)

@jvdsn
Copy link
Contributor Author

jvdsn commented Jan 27, 2022

FYI: As a temporary fix for cross you could cherry-pick 10f5b73 (see #354)

Thanks, done

@newpavlov
Copy link
Member

newpavlov commented Feb 10, 2022

Because CShake128 and CShake256 don't implement Default, the easiest way to construct an instance is using CShake128::from_core(CShake128Core::new(customization)) which is quite verbose

Yeah, it's quite unfortunate. I had a similar issue with MAC types in blake2, but it does not look like the current version of Rust has a proper solution for this problem. In the end, I had to define them as independent types instead of relying on the core wrapper. You could use a similar approach if you think that improved ergonomics will outweigh additional boilerplate.

BTW with #351 merged you can rebase and the CI should be fixed.

@elichai
Copy link

elichai commented Feb 10, 2022

Thanks for taking this up!
I started working on updating the PR, but the API changed so much it required too much time from me and it's a busy period.

@jvdsn
Copy link
Contributor Author

jvdsn commented Feb 10, 2022

Because CShake128 and CShake256 don't implement Default, the easiest way to construct an instance is using CShake128::from_core(CShake128Core::new(customization)) which is quite verbose

Yeah, it's quite unfortunate. I had a similar issue with MAC types in blake2, but it does not look like the current version of Rust has a proper solution for this problem. In the end, I had to define them as independent types instead of relying on the core wrapper. You could use a similar approach if you think that improved ergonomics will outweigh additional boilerplate.

BTW with #351 merged you can rebase and the CI should be fixed.

In that case, you use the RtVariableCoreWrapper right? The variable length output isn't really an issue here right now, because it doesn't need to be known when the instance is created. It's mainly about the function name/customization string which need to be known when the instance is created. I'm not very experienced in Rust, so maybe I'm not seeing the solution here

@jvdsn
Copy link
Contributor Author

jvdsn commented Feb 10, 2022

The new_with_function_name accepts a function name argument. This results in some overhead for the cSHAKE initialization, but normally the initialization shouldn't be performed a lot anyway. Even though the end user will probably never need it, I didn't want to omit that functionality should it ever be necessary in the future. Though perhaps new_with_function_name could be made private for now?

I just noticed the KMAC algorithm does use this function name, so we should probably keep it public for when KMAC is implemented.

@jvdsn
Copy link
Contributor Author

jvdsn commented Mar 3, 2022

@newpavlov when would you have time to review this PR? I'm also still open for suggestions to making the API more ergonomic, but right now I feel like this is the easiest way to construct a CSHAKE instance:

CShake128::from_core(CShake128Core::new(customization));

which is a bit more complex than other digests, but not too bad.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

LGTM. @newpavlov care to do a pass reviewing this?

@tarcieri tarcieri requested a review from newpavlov March 27, 2022 19:01
@tarcieri tarcieri merged commit f9ea367 into RustCrypto:master May 28, 2022
@elichai
Copy link

elichai commented Jul 30, 2022

Can we get a new version published with this?

@tarcieri tarcieri mentioned this pull request Jul 30, 2022
@tarcieri
Copy link
Member

Released in #384

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.

None yet

5 participants