KMAC, 2nd: added keccak-fips as standalone; added KMAC-256

[falko-strenzke]

This is a new PR for the addition of KMAC. This time, Keccak[c], the building block underlying all SHA-3 derived hash functions and also KMAC (build on cSHAKE, currently not implemented in Botan) is added as a freestanding class that is used as a member in the SHA-3 derived functions.

This PR supersedes the previous PR for KMAC: #3525

[coveralls]

coverage: 91.718% (+0.006%) from 91.712% when pulling 7b94190 on falko-strenzke:kmac_05_23 into dfe1714 on randombit:master.

[reneme]

Good direction, I think! Some remarks/questions regarding the basic design of Keccak_FIPS:

1. Is there a use-case for handling the basic data structures (m_S, m_S_pos, ...) outside of the class?
   Maybe we should just fully encapsulate them and replace the low-level static method absorb(), finish(), expand(), permute() with ordinary members that handle the data structures internally. Exposing those data structures via getters and setters would probably give enough flexibility if needed, while hiding some of the Keccak complexity inside the class.
2. For new code, I'd love to see full adoption of std::span<> and std::string_view where applicable. Instead of C-style pointer-length-parameters or even raw pointer+hope kind of interfaces.
3. With Keccak_FIPS deliberately being designed as a component for composition rather than a base for inheritance, I think it should be a final class without any virtual methods.

Some more details are in inline comments.

[falko-strenzke]

Good direction, I think! Some remarks/questions regarding the basic design of Keccak_FIPS:

1. Is there a use-case for handling the basic data structures (`m_S`, `m_S_pos`, ...) outside of the class?
   Maybe we should just fully encapsulate them and replace the low-level static method `absorb()`, `finish()`, `expand()`, `permute()` with ordinary members that handle the data structures internally. Exposing those data structures via getters and setters would probably give enough flexibility if needed, while hiding some of the Keccak complexity inside the class.

I have implemented the encapsulation now for SHA3 and Keccak, but not yet for SHAKE and SHAKE-CIPHER.
The main reason was that the adaptions of the latter aren't (at least for me) entirely trivial if there is complete encapsulation. But I agree that this should be the desired goal. But in order not to delay the PR for this feature further, I would suggest.

2. For new code, I'd love to see full adoption of `std::span<>` and `std::string_view` where applicable. Instead of C-style pointer-length-parameters or even raw pointer+hope kind of interfaces.

I am using std::span now in the interface (everywhere I hope).

3. With `Keccak_FIPS` deliberately being designed as a component for composition rather than a base for inheritance, I think it should be a `final` class without any virtual methods.

virtuality is removed now and the class is final.

[reneme]

Thanks! A few more minor things. Also, I'd be really curious what @randombit has to say to this proposal. :)

[reneme]

As a more generic remark: Would it make sense to pull the Keccak_FIPS into its own module? It might be a bit contrived, but some consumer might want to enable SHAKE but not SHA-3. That's currently not possible as the underlying Keccak_FIPS is (somewhat arbitrarily) a part of the sha3 module. Jack suggested to put it into a new permutations subdir`.

[falko-strenzke]

As a more generic remark: Would it make sense to pull the Keccak_FIPS into its own module? It might be a bit contrived, but some consumer might want to enable SHAKE but not SHA-3. That's currently not possible as the underlying Keccak_FIPS is (somewhat arbitrarily) a part of the sha3 module. Jack suggested to put it into a new permutations subdir`.

Done as well.

[reneme]

Could you squash your commits and rebase to master? There are a few minor conflicts. If possible, maybe separate the SHA3/Keccak refactoring from the addition of KMAC in the commit history.

[falko-strenzke]

Could you squash your commits and rebase to master? There are a few minor conflicts. If possible, maybe separate the SHA3/Keccak refactoring from the addition of KMAC in the commit history.

Done.

[falko-strenzke]

I would like to inquire about the state of this PR as we need KMAC integrated upstream in our project. If there any open issues remaining with this PR please let me know.

[lieser]

@falko-strenzke I don't know about the state of the review, but as minimum for this to get merged the CI would need to be green.

[reneme]

The requested changes below should fix the remaining CI issues.

[reneme]

Once the CI is fixed, I would suggest the following:

1. Split this PR into two (for better reviewability):
   1.1. Just the Keccak related refactoring (eXtendable Output Functions as first-class citizen #3671 sort-of depends on it)
   1.2. The addition of KMAC on top of the refactoring
2. Squash commits (individually in both proposed PRs)
3. Have another round of review and eventually bring this upstream

@falko-strenzke If you need any assistance, please don't hesitate to contact me on my work email or schedule a Teams meeting.

[falko-strenzke]

Once the CI is fixed, I would suggest the following:

1. Split this PR into two (for better reviewability):
   1.1. Just the Keccak related refactoring ([eXtendable Output Functions as first-class citizen #3671](https://github.com/randombit/botan/pull/3671) sort-of depends on it)
   1.2. The addition of KMAC on top of the refactoring

2. Squash commits (individually in both proposed PRs)

3. Have another round of review and eventually bring this upstream

@falko-strenzke If you need any assistance, please don't hesitate to contact me on my work email or schedule a Teams meeting.

I rewrote history now so that there are only two commits, one for SHA-3 refactoring and one for the addition to keccak.

So do I understand correctly, that the goal is to have both PRs on top of master:

master
  - PR 1 for SHA-3 refactoring
  - PR 2 for addition of KMAC

where PR 2 will compile only after rebasing onto master after the merge of PR 1?

[reneme]

Thanks for rewriting the history. That helps a lot already.

Usually, I'd open two PRs on top of each other: I.e. (without rebasing any commits): One PR that contains 8f126dd and 1b089e1 (the refactoring) and another one containing 8f126dd, 1b089e1 and 45bd450 (the refactoring + KMAC). In fact, the second proposed PR basically is this one already.

Typically, GitHub is clever enough to notice once the "smaller" PR is merged and adapts the diff view of the "bigger" PR accordingly. At least that's the workflow @randombit and me are typically using when splitting change sets like that.

One noteworthy downside: In case the reviewer of the "smaller" PR desires changes, it becomes a bit messy as the "bigger" PR must be rebased to the "smaller" one.

[falko-strenzke]

Thanks for rewriting the history. That helps a lot already.

Usually, I'd open two PRs on top of each other: I.e. (without rebasing any commits): One PR that contains 8f126dd and 1b089e1 (the refactoring) and another one containing 8f126dd, 1b089e1 and 45bd450 (the refactoring + KMAC). In fact, the second proposed PR basically is this one already.

Typically, GitHub is clever enough to notice once the "smaller" PR is merged and adapts the diff view of the "bigger" PR accordingly. At least that's the workflow @randombit and me are typically using when splitting change sets like that.

One noteworthy downside: In case the reviewer of the "smaller" PR desires changes, it becomes a bit messy as the "bigger" PR must be rebased to the "smaller" one.

OK, I have now created #3673.

[reneme]

OK, I have now created #3673.

Thanks. I'll review (hopefully) tomorrow morning.

[reneme]

Before going deeper into a review: NIST SP.800-185 defines KMAC in terms of cSHAKE (customizable SHAKE) which is defined in terms of SHAKE. As I understand it, cSHAKE is basically SHAKE with incorporated domain separation byte strings.

You are intermingling the cSHAKE implementation with KMAC, which is technically fine. Though, I'm wondering whether we should split this up to ease future implementations of other functions defined in SP.800-185.

We could either extend Botan's SHAKE implementation to optionally take the domain separators N and S. They could both default to the empty string (resulting in the vanilla SHAKE function). Or we add another dedicated cSHAKE implementation that builds on the existing SHAKE and requires N and S. Personally, I'd go for the first option, as I believe the fairly simple domain separation approach in cSHAKE doesn't really justify the addition of another class.

What do you think? @falko-strenzke @randombit

[falko-strenzke]

Before going deeper into a review: NIST SP.800-185 defines KMAC in terms of cSHAKE (customizable SHAKE) which is defined in terms of SHAKE. As I understand it, cSHAKE is basically SHAKE with incorporated domain separation byte strings.

You are intermingling the cSHAKE implementation with KMAC, which is technically fine. Though, I'm wondering whether we should split this up to ease future implementations of other functions defined in SP.800-185.

We could either extend Botan's SHAKE implementation to optionally take the domain separators N and S. They could both default to the empty string (resulting in the vanilla SHAKE function). Or we add another dedicated cSHAKE implementation that builds on the existing SHAKE and requires N and S. Personally, I'd go for the first option, as I believe the fairly simple domain separation approach in cSHAKE doesn't really justify the addition of another class.

What do you think? @falko-strenzke @randombit

Either option is fine for me. Given that apparently it is forbidden to use inheritance other than for mere interface inheritance, I would fear that a cSHAKE implementation would introduce a great deal of code duplication. If the SHAKE interface is extended as you propose, it should be well documented how to use it as cSHAKE.

[reneme]

General remark: Consider adding KMAC to the ./botan speed benchmark tool. See src/cli/speed.cpp.

[reneme]

@falko-strenzke This now needs a rebase to master after #3673 was merged. It will reduce the patch displayed by GitHub to the relevant changes to introduce KMAC.

[reneme]

. Though, I'm wondering whether we should split this up to ease future implementations of other functions defined in SP.800-185.

In #3671 I now added a XOF implementation of cSHAKE-128/256. This required implementing some of the helper functions in SP.800-185 a bit more generically. See src/lib/permutations/keccak_perm/keccak_helpers.h and src/lib/xof/shake_xof.h.

Basically, KMAC would need to depend on the shake_xof module, include botan/internal/shake_xof.h and instantiate cSHAKE like so:

cSHAKE_256_XOF xof("KMAC");
xof.start(S, {});
xof.update(/* incorporate key using helpers in keccak_helpers.h */);
xof.update(msg)
xof.output(output_len);

@falko-strenzke If you're okay with it, I'm going to give the adaption of the KMAC implementation a try tomorrow.

[reneme]

See: #3689

[reneme]

Closing in favor of #3689.