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.

Sign up for GitHub

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

Jump to bottom

Refactoring SHA3: based on new permutation keccak-fips #3673

Merged
merged 2 commits into from
Sep 1, 2023
Merged

Refactoring SHA3: based on new permutation keccak-fips #3673

merged 2 commits into from
Sep 1, 2023

Conversation

falko-strenzke
Copy link
Collaborator

This PR is needed as a prerequisite of #3570.

@falko-strenzke falko-strenzke mentioned this pull request Aug 16, 2023
Closed
@coveralls
Copy link

coveralls commented Aug 16, 2023 •
edited
Loading

Coverage Status

coverage: 91.717% (+0.005%) from 91.712% when pulling 85b70ed on falko-strenzke:refact-sha3 into dfe1714 on randombit:master.

@reneme reneme mentioned this pull request Aug 17, 2023
Merged
reneme
reneme reviewed Aug 17, 2023
View reviewed changes
Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

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

As a sanity check I used this refactoring as the foundation for the new XOF interface (#3671). In general that seems to work nicely. Though, I'm wondering whether we should make the Keccak_FIPS implementation more generic. Most notably to allow multiple (unaligned) calls to expand(). The logic for that already exists in SHAKE_XOF::generate_bytes(). But centralizing it here, would benefit other users of Keccak_FIPS.

Unfortunately, I didn't manage to finish a thorough review today. Will look deeper into it tomorrow.

In the meantime: @randombit, could you please skim this and give feedback on the general direction of this refactoring?

I didn't manage to finish the review this morning.

src/lib/permutations/keccak_fips/keccak_fips.h Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.cpp Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.cpp Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.cpp Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.cpp Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.h Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.h Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.h Outdated Show resolved Hide resolved
randombit
randombit reviewed Aug 17, 2023
View reviewed changes
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

Yeah this seems basically fine.

I'd suggest keccak_perm as the module name since keccak_fips doesn't really capture what the module is doing.

src/lib/permutations/keccak_fips/keccak_fips.h Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.h Outdated Show resolved Hide resolved
src/lib/utils/loadstor.h Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.cpp Outdated Show resolved Hide resolved
src/lib/permutations/keccak_fips/keccak_fips.h Outdated Show resolved Hide resolved
@reneme
Copy link
Collaborator

reneme commented Aug 18, 2023

@falko-strenzke I'm currently working at restructuring this even further (fully removing the static functions and encapsulating the Keccak state fully). It's fairly convenient to do that in the context of the XOF implementation. I'll post a patch later, so that we can discuss. Just wanting to make sure that we don't step on each other's toes.

@reneme reneme mentioned this pull request Aug 18, 2023
Closed
@reneme
Copy link
Collaborator

reneme commented Aug 19, 2023

I'll post a patch later, so that we can discuss.

#3675 contains two extra patches on top of this work. One adds some convenience functions to the internal BufferSlicer and BufferStuffer helpers. The other smoothens out the edges on the Keccak refactoring and addresses the comments in this PR. Details can be found in the description of #3675.

@falko-strenzke, I hope you'll excuse me for having taken this PR over like that. I suggest that you take my patches, squash the changes as you see fit and update this PR for merging. #3675 can then be closed.

@reneme reneme added this to the Botan 3.2.0 milestone Aug 19, 2023
@reneme reneme mentioned this pull request Aug 19, 2023
Closed
@falko-strenzke
Copy link
Collaborator Author

Yeah this seems basically fine.

I'd suggest keccak_perm as the module name since keccak_fips doesn't really capture what the module is doing.

Done.

@falko-strenzke falko-strenzke force-pushed the refact-sha3 branch 5 times, most recently from f4eb7aa to 3ced414 Compare August 21, 2023 11:52
reneme and others added 2 commits August 21, 2023 15:06
@reneme @falko-strenzke
add BufferSlicer/BufferStuffer single-byte accessors
51690f4
@falko-strenzke
refactored SHA-3 code: Keccak-FIPS is now the underlying building blo…
85b70ed 
…ck for SHA-3

renamed keccak_fips to keccak_perm(utation) everywhere

keccak finish now without output

removed output length from keccak permutation

Clean up the Keccak_Permutation

* replace single-shot ::expand() with more versatile ::squeeze()
* use BufferStuffer/BufferSlicer in ::squeeze() and ::absorb()
* remove low-level access to Keccak state
* remove static Keccak state mutation methods
* ::permute() is now private
@falko-strenzke
Copy link
Collaborator Author

@randombit now @reneme and me agree that this combined PR of ours is ready for final review

@reneme reneme mentioned this pull request Aug 22, 2023
Merged
randombit
randombit reviewed Aug 23, 2023
View reviewed changes
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

This looks fine, thanks.

One request before merge: please benchmark the various operations to make sure we're not introducing any major performance regression. Especially I would be concerned about Kyber as it seems very sensitive to the XOF performance.

@reneme
Copy link
Collaborator

reneme commented Aug 24, 2023 •
edited
Loading

SHA-3(256) SHA-3(512)
this pull request 436MiB/sec 221MiB/sec
latest master 426MiB/sec 227MiB/sec

... measured as the average of three runs of ./botan speed --msec=2000 "algoname". 11th Gen Intel(R) Core(TM) i7-11850H @ 2.50GHz 2.50 GHz on Windows inside a WSL2 VM.

SHA-3 is very much biased towards absorb(). I've added a benchmark for the SHAKE XOF in #3671 (comment).

@falko-strenzke
Copy link
Collaborator Author

falko-strenzke commented Aug 24, 2023 •
edited
Loading

SHA-3(256) SHA-3(512)
this pull request 436MiB/sec 221MiB/sec
latest master 426MiB/sec 227MiB/sec

... measured as the average of three runs of ./botan speed --msec=2000 "algoname"

SHA-3 is very much biased towards absorb(). I've added a benchmark for the SHAKE XOF in #3671 (comment).

I also observe a performance loss for ./botan speed --msec=2000 "SHA-3(256)", but it is in the domain of 1%, which is much less than what @reneme sees (about 3%).

SHA-3(256)
this pull request 389 MiB/sec
master 392 MiB/sec

It is also based on the average of 3 measurements, plattform Intel(R) Core(TM) i7-1065G7 CPU,

@reneme @randombit Is this worth exploring in more detail?

@reneme
Copy link
Collaborator

reneme commented Aug 24, 2023

Is this worth exploring in more detail?

I feel this deviation is within the tolerance of the measurement accuracy of this benchmark tool.

@randombit
Copy link
Owner

I feel this deviation is within the tolerance of the measurement accuracy of this benchmark tool.

Agree. I would be concerned about anything >10%, numbers here seem ok

@reneme reneme merged commit 560aec3 into randombit:master Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reneme reneme reneme approved these changes

@randombit randombit randombit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Botan 3.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@falko-strenzke @coveralls @reneme @randombit