feat(hmac): Add generate_with_length method to hmac::Key
#1218
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1218 +/- ##
==========================================
- Coverage 93.65% 93.57% -0.08%
==========================================
Files 115 116 +1
Lines 17468 17459 -9
==========================================
- Hits 16359 16338 -21
- Misses 1109 1121 +12
Continue to review full report at Codecov.
|
src/hmac.rs
Outdated
| where | ||
| F: FnOnce(&mut [u8]) -> Result<(), error::Unspecified>, | ||
| { | ||
| let mut key_bytes = [0; digest::MAX_OUTPUT_LEN]; | ||
| let key_bytes = &mut key_bytes[..algorithm.0.output_len]; |
There was a problem hiding this comment.
This doesn't work when the requested length is larger than digest::MAX_OUTPUT_LEN.
Please write a parameterized test that accepts an hmac::Algorithm and a length, and then tests your new function with that algorithm and length, for the following lengths:
1, 2, output_len - 1, output_len, output_len + 1, block_len - 1, block_len, block_len + 1, (block_len * 2) - 1, block_len * 2, (block_len * 2) + 1.
There was a problem hiding this comment.
Updated logic and added tests.
There was a problem hiding this comment.
You can't add a call to to_vec to construct because construct needs to be able to work even in environments that doesn't have a memory allocator. Instead you need to move the allocation to your new constructor and mark that constructor #[cfg(feature = "alloc")].
There was a problem hiding this comment.
Ah nice catch! Moved new construct logic to generate_with_length behind the alloc feature 👍
| let _key = hmac::Key::generate_with_length( | ||
| *algorithm, | ||
| NonZeroUsize::new(*length).unwrap(), | ||
| &rng, | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
Not sure how to assert the key length with the expected one. 🤔
| @@ -364,4 +396,40 @@ mod tests { | |||
| assert!(hmac::verify(&key, HELLO_WORLD_BAD, tag.as_ref()).is_err()) | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
| pub fn hmac_generate_with_length() { | |||
There was a problem hiding this comment.
Tests of the public HMAC API should go in hmac_tests.rs.
I will write a test for the original generate() that you can emulate.
| let mut key_bytes = [0; digest::MAX_OUTPUT_LEN].to_vec(); | ||
| if key_size > digest::MAX_OUTPUT_LEN { | ||
| key_bytes.resize(key_size, 0); | ||
| } |
There was a problem hiding this comment.
I don't understand why we would truncate the length of the key to MAX_OUTPUT_LEN. The user asked for a key of length length and so I guess they should get a key of that length; i.e. without this truncation. Or if there's no need for such large keys then we'd return an error. But I don't think we'd ever silently truncate the key length.
| rng: &dyn rand::SecureRandom, | ||
| ) -> Result<Self, error::Unspecified> { | ||
| let key_size = length.get(); | ||
| let mut key_bytes = [0; digest::MAX_OUTPUT_LEN].to_vec(); |
There was a problem hiding this comment.
In the long run, we really should avoid heap allocation here, especially just to support huge key sizes. This makes the function much harder to implement. OTOH, if the caller is willing to do a heap allocation then you can just construct a random array using ring::rand and then call ::new(algorithm, ...) instead.
#1216