-
Notifications
You must be signed in to change notification settings - Fork 568
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
Generalize PQC KEM KAT Runner + Minor Adaptions in Kyber #3807
Conversation
This seems ok we should just make sure it's mentioned in the release notes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine. Left a couple of nits.
One thing we should consider is that if we are deprecated 90s mode, we should make the modern mode mandatory. Then the options become modern+90s, or modern only, but no 90s-only.
#if defined(BOTAN_HAS_KYBER_90S) | ||
is_90s() || | ||
#endif | ||
false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really hard to read!
Maybe
#if defined(BOTAN_HAS_KYBER)
if(is_modern())
return true;
#endif
...
return false;
src/tests/test_kyber.cpp
Outdated
// We use different hash functions for Kyber 90s and Kyber "modern", as | ||
// those are consistent with the requirements of the implementations. | ||
auto hash = (m_mode.is_modern()) ? Botan::HashFunction::create_or_throw("SHAKE-256(128)") | ||
: Botan::HashFunction::create_or_throw("SHA-256"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a ternary to get the hash name then a single call to create_or_throw
const std::string hash_name = m_mode.is_modern() ? "SHAKE-256(128)" : "SHA-256";
auto hash = Botan::HashFunction::create_or_throw(hash_name);
* add KyberMode::is_available() -- deprecated * Kyber::algo_name() now always returns 'Kyber'
* take input bytes via std::span<> * add Fixed_Output_RNG::empty() for readability
1613d5c
to
7ca45d5
Compare
Addressed review comments and rebased to latest master.
Sounds reasonable. I'll look into that independently. Edit: As far as I'm aware there's no concept of a "deprecated module", right? Hence, I don't see an obvious way to prohibit configuring like: |
There is a |
KAT Test Generalization
This generalizes the Known-Answer-Test runner for PQC KEMs. Kyber is currently the only consumer, but I'll rebase #3679 onto this and adapt the tests there as well. Also, it probably makes sense to generalize the signature KATs along the same line.
As a result, we can remove the heavy KAT vectors that we had used initially. Instead, the new vectors contain hashed versions of the KEM ciphertexts, and keys. That saves significant checkout space. Also, I reduced the set of KATs from 100 per algorithm parameterization to just 25. The latter is certainly debatable, but IMO 25 should be just good enough.
The general implementation (in
src/tests/test_pubkey_pqc.h
) is based on an "implementation delegate", that is passed in as a template parameter, rather than a class hierarchy. That way, the general implementation can directly use the public types (e.g.Kyber_PublicKey
,Kyber_PrivateKey
) instead of hiding key generation and serialization behind abstract methods.Minor Kyber Adaptions
I've taken over the idea of having a
KyberMode.is_available()
from the FrodoKEM PR. Albeit, this method being deprecated from the start, I figured it would be good to keep things consistent. Also it helps with the generic KAT test implementation.Additionally,
Kyber::algo_name()
reported the fully-qualified name (i.e. "Kyber-512-90s-r3") instead of just "Kyber". We don't do that anywhere else (e.g.ECDSA::algo_name()
just reports "ECDSA"). I'm aware that this is somewhat an API break, though. What do you think, @randombit?Finally, I removed the
stream
dependency of Kyber. That module was probably needed for theshake_cipher
that was replaced by theshake_xof
at some point. This must have been a leftover from this cleanup.