Conversation
e22c7bb to
86f8839
Compare
|
Sorry we haven't taken a look at this yet. We're trying to wrap up our next stable release series and will have time after that |
|
No worries at all, thank you Tony! |
| rust-version = "1.85" | ||
|
|
||
| [dependencies] | ||
| cshake = { git = "https://github.com/RustCrypto/hashes", branch = "master" } |
There was a problem hiding this comment.
Currently using a pre-release version of cshake after the sha3 release to 0.11 refactors cshake into a separate crate. Will fix when cshake is released.
kmac/src/lib.rs
Outdated
| impl ExtendableOutput for $kmac { | ||
| type Reader = $reader; | ||
|
|
||
| /// Finalize this KMAC to a variable-length (extendable) output stream, as defined in |
There was a problem hiding this comment.
Note that this comment will not be visible in crate docs.
|
@newpavlov Thank you for the review, the implementation is significantly cleaner with a trait-based I've had to add the |
b80b3f1 to
fa9b52d
Compare
| impl<Rate: BlockSizes + sealed::KmacParams> MacMarker for Kmac<Rate> {} | ||
|
|
||
| impl<Rate: BlockSizes + sealed::KmacParams> OutputSizeUser for Kmac<Rate> { | ||
| type OutputSize = <Rate as sealed::KmacParams>::OutputSize; |
There was a problem hiding this comment.
We could use this instead:
use digest::array::typenum::Diff;
use core::ops::Sub;
impl<Rate: BlockSizes> OutputSizeUser for Kmac<Rate>
where
U200: Sub<Rate>,
Diff<U200, Rate>: ArraySize,
{
type OutputSize = Diff<U200, Rate>;
}It would be more "generic" and allow to remove the sealed trait, but with a bit more annoying trait bounds, so I am not sure whether it's worth the trouble.
There was a problem hiding this comment.
Unfortunately, the Rust compiler is not "smart" enough for Rate: IsLess<U200, Output = True> to be sufficient.
There was a problem hiding this comment.
Alternatively, I had considered allowing the OutputSize to be fully generic*, as it's valid to generate any output length for KSF purposes.
I still want to ensure the Kmac128 and Kmac256 types provide the recommended default MAC output sizes, but I also don't want the average user to need to understand the Rate. One option is a second set of type aliases that only expose a generic OutputSize (Kmac128Something<OutputSize>).
Alternatively the core Kmac parameters could be the Capacity and OutputSize which are easier for a user to understand (eg. Kmac<U128, OutputSize>). The Rate can be computed via type Rate = Diff<U200, Capacity>. I slightly prefer this approach from a user's perspective, but I don't know if using a Capacity parameter (instead of Rate) would break tradition from the other RustCrypto crates?
*I would still also keep finalize_into_buf for its runtime-determined output size.
Integrating comments from @newpavlov's code review. - Remove the messy impl kmac macro in favour of a cleaner trait system approach. - Clean up test vectors and use hex-literal, and use raw NIST vectors copy/pasted from the PDFs to make review easier. - Move public API tests into tests/kmac.rs - Remove std from the std tests. - Remove the bench_full benchmarks, we will discuss this in a separate PR. - Adjust docstrings and README for completeness.
I have tried to follow the RustCrypto MACs workflow, and used the traits as best I can.
This implements KMAC128, KMAC256, KMACXOF128, and KMACXOF256.
Key deviations from the existing mac traits:
Mac::finalize()outputs a 32-bit digest for KMAC128, and 64-bit for KMAC256 following the NIST default MAC output length guidance in section 8.4.Kmac*::finalize_into_buf(out: &mut [u8])is a generic finalize method for any fixed output size, following section 4.3 of NIST SP 800-185. I wasn't aware of a suitable trait to express this.FixedOutput::finalize_into, but it's limited to the Mac-defined output sizeOutput<Self>. This implementation allows for runtime determined output sizes.ExtendableOutput::finalize_xof()is used to implement KMACXOF128 and KMACXOF256. We could add an ExtendableMac trait to incorporate ExtendableOutput?Kmac*::new_customization(secret: &[u8], customization: &[u8])is used to initialize with a customization string, and again I wasn't aware of any suitable existing trait to express this.I'd be happy to try to add traits for this, if you believe this is the right direction? The crate should already be useful in it's current form though, and we could add traits later.
An alternate design could separate Kmac128, Kmac256, KmacXof128, and KmacXof256 types. This PR combines the fixed and XOF outputs into the same type, with different finalization methods for each output.
Happy to hear any feedback!
Related issues: