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

Refactor k12 #353

Merged
merged 1 commit into from
May 9, 2023
Merged

Refactor k12 #353

merged 1 commit into from
May 9, 2023

Conversation

aewag
Copy link
Contributor

@aewag aewag commented Jan 23, 2022

The k12 crate had its own keccak-p permutation implementation.
With RustCrypto/sponges#7 a generic keccak-p implementation would be available, which could be used within the k12 crate.
With #458 TurboSHAKE is available, which can be used within the k12 crate.

k12 uses a block size of 8192 to split the input. The maximum supported block size of block-buffer is limited to 256. Therefore this implementation buffers the input within an additional array.

k12/Cargo.toml Outdated Show resolved Hide resolved
@gvanas
Copy link

gvanas commented Jan 25, 2022

For K12, it would be interesting to also have SIMD implementations of parallel Keccak-p permutations.

Sorry, I cannot help directly because I am not fluent in Rust, but I can at least point out such implementations in XKCP/K12 for inspiration.

@aewag
Copy link
Contributor Author

aewag commented Jan 25, 2022

For K12, it would be interesting to also have SIMD implementations of parallel Keccak-p permutations.

Sorry, I cannot help directly because I am not fluent in Rust, but I can at least point out such implementations in XKCP/K12 for inspiration.

Yes, that definitely would get some nice improvements. Currently I have made a draft PR for SIMD backed Keccak-f permutations in RustCrypto/sponges#8. As soon as this is available, the k12 implementation could be updated to incorporate it.

EDIT: RustCrypto/sponges#8 would need an generic SIMD backed keccak-p permutation for use within k12. I'll extend the draft PR.

@gvanas
Copy link

gvanas commented Jan 25, 2022

I was not aware of that PR. Nice!

@tarcieri
Copy link
Member

@aewag this is great work. Anything I can do to help move it along?

@aewag
Copy link
Contributor Author

aewag commented Mar 28, 2022

@aewag this is great work. Anything I can do to help move it along?

If you have time to review RustCrypto/sponges#7, that would be great. Other than that, this propably needs a rebase and should more or less work.

(I planned to further update the implementation to be alloc-free and maybe also in the future with SIMD support, but I will not able to work on this in the near future.)

EDIT: I just rebased this on top of the current master.

@tarcieri
Copy link
Member

@aewag went ahead and merged RustCrypto/sponges#7. We can probably cut another release of the keccak crate.

I wanted to point out this is probably the biggest problem with the k12 crate in its current form:

hashes/k12/src/lib.rs

Lines 37 to 39 in 1cdbd53

/// Input to be processed
// TODO(tarcieri): don't store input in a `Vec`
buffer: Vec<u8>,

It buffers all of the input in a Vec<u8> and doesn't actually hash it until XofReader::read:

hashes/k12/src/lib.rs

Lines 128 to 177 in 1cdbd53

assert!(
!self.finished,
"not yet implemented: multiple XofReader::read invocations unsupported"
);
let b = 8192;
let c = 256;
let mut slice = Vec::new(); // S
slice.extend_from_slice(&self.buffer);
slice.extend_from_slice(&self.customization);
slice.extend_from_slice(&right_encode(self.customization.len())[..]);
// === Cut the input string into chunks of b bytes ===
let n = (slice.len() + b - 1) / b;
let mut slices = Vec::with_capacity(n); // Si
for i in 0..n {
let ub = min((i + 1) * b, slice.len());
slices.push(&slice[i * b..ub]);
}
// TODO(tarcieri): get rid of intermediate output buffer
let tmp_buffer = if n == 1 {
// === Process the tree with only a final node ===
f(slices[0], 0x07, output.len())
} else {
// === Process the tree with kangaroo hopping ===
// TODO: in parallel
let mut intermediate = Vec::with_capacity(n - 1); // CVi
for i in 0..n - 1 {
intermediate.push(f(slices[i + 1], 0x0B, c / 8));
}
let mut node_star = Vec::new();
node_star.extend_from_slice(slices[0]);
node_star.extend_from_slice(&[3, 0, 0, 0, 0, 0, 0, 0]);
#[allow(clippy::needless_range_loop)]
for i in 0..n - 1 {
node_star.extend_from_slice(&intermediate[i][..]);
}
node_star.extend_from_slice(&right_encode(n - 1));
node_star.extend_from_slice(b"\xFF\xFF");
f(&node_star[..], 0x06, output.len())
};
output.copy_from_slice(&tmp_buffer);
self.finished = true;

That makes the implementation unusable for large inputs, and it doesn't properly implement the XofReader contract in that it will panic if XofReader::read is called repeatedly.

@aewag
Copy link
Contributor Author

aewag commented May 12, 2022

@aewag went ahead and merged RustCrypto/sponges#7. We can probably cut another release of the keccak crate.

I wanted to point out this is probably the biggest problem with the k12 crate in its current form.
That makes the implementation unusable for large inputs, and it doesn't properly implement the XofReader contract in that it will panic if XofReader::read is called repeatedly.

Yep, agreed. I started to work on these (two) issue(s), but I don't have yet a working implementation.

@aewag aewag force-pushed the refactor-k12 branch 3 times, most recently from f5ec773 to 5f4996e Compare May 7, 2023 20:56
@aewag aewag marked this pull request as ready for review May 7, 2023 20:56
@aewag aewag force-pushed the refactor-k12 branch 3 times, most recently from 46da7e0 to a16eb1b Compare May 7, 2023 21:29
@aewag
Copy link
Contributor Author

aewag commented May 8, 2023

@tarcieri I finished the refactoring. The PR is ready for review.

k12/src/lib.rs Outdated Show resolved Hide resolved
Remove the use of vectors and implement `XofReader`.
@tarcieri tarcieri merged commit 91dfb20 into RustCrypto:master May 9, 2023
@aewag aewag deleted the refactor-k12 branch May 9, 2023 13:06
@aewag
Copy link
Contributor Author

aewag commented May 9, 2023

Thank you! 👍

@tarcieri
Copy link
Member

tarcieri commented May 9, 2023

I can cut a release if you'd like

@aewag
Copy link
Contributor Author

aewag commented May 9, 2023

Yeah, that would be nice. Thanks!

This was referenced Jun 10, 2023
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