Minimum modulus size checks #526
Conversation
tarcieri
force-pushed
the
modulus-size-checks
branch
from
2b9e941 to
f5eaf0e
Compare
| key_generation!(key_generation_128, 2, 128); | ||
| key_generation!(key_generation_1024, 2, 1024); | ||
|
|
||
| key_generation!(key_generation_multi_3_256, 3, 256); | ||
|
|
||
| key_generation!(key_generation_multi_4_64, 4, 64); | ||
|
|
||
| key_generation!(key_generation_multi_5_64, 5, 64); | ||
| key_generation!(key_generation_multi_8_576, 8, 576); | ||
| key_generation!(key_generation_multi_16_1024, 16, 1024); |
There was a problem hiding this comment.
I removed tests that generate tiny keys. I think we probably don't actually want to provide functionality to generate such keys?
Tests that need to go fast can use a tiny key test vector rather than a factory for insecure keys.
There was a problem hiding this comment.
they are pretty useful when debugging, so I would appreciate a 64 one
There was a problem hiding this comment.
We will need to add an API specifically for generating insecure keys then. Perhaps it could be crate internal?
There was a problem hiding this comment.
hmm, I would really like to have it under the hazmat API.. I know it's dangerous, but still sometimes useful :/
There was a problem hiding this comment.
hmm, I would really like to have it under the hazmat API.. I know it's dangerous, but still sometimes useful :/
There was a problem hiding this comment.
There is now RsaPrivateKey::new_unchecked through feature="hazmat"
| @@ -821,21 +877,43 @@ mod tests { | |||
| use serde_test::{assert_tokens, Configure, Token}; | |||
|
|
|||
| let mut rng = ChaCha8Rng::from_seed([42; 32]); | |||
| let priv_key = RsaPrivateKey::new(&mut rng, 64).expect("failed to generate key"); | |||
| let priv_key = RsaPrivateKey::new(&mut rng, 1024).expect("failed to generate key"); | |||
There was a problem hiding this comment.
I guess this is an example that should probably be switched to a test vector key rather than trying to make one on-the-fly with a deterministic RNG.
| // Always validate the key, to ensure precompute can't fail | ||
| k.validate()?; | ||
|
|
||
| // Precompute when possible, ignore error otherwise. | ||
| k.precompute().ok(); |
There was a problem hiding this comment.
These comments seem a little confusing, the first says "precompute can't fail", and the second says "ignore error", so it seems even with validation precomputation can still fail?
If that's the case, perhaps precomputation can be moved to from_components_unchecked
|
Down to 35 test failures 😅 |
tarcieri
force-pushed
the
modulus-size-checks
branch
2 times, most recently
from
83c6ba8 to
b83ae75
Compare
tarcieri
force-pushed
the
modulus-size-checks
branch
from
b83ae75 to
f016f0b
Compare
tarcieri
changed the title
Changes the existing checked APIs to respect a minimum modulus size in addition to a maximum one. Note: several tests fail because of this, so we'll need to go through them and convert to an unchecked API where appropriate (or decide if the test is bogus to begin with)
… again (#561) `RSAPrivateKey::new_unchecked` added (gated under hazmat) All unit RSAPrivateKeys are now 1024/2048 bits and I re-ran the snapshots based on that. I don't think it makes sense to test with smol keysize and it doesn't take that much longer regardless. If we insist on using the existing test data i can re-do with unchecked but 🤷♀️ proptests (gated now to hazmat using `RSAPrivateKey::new_unchecked()` are using 512 bits given with 2048 bits CI increases from 4 mins to 15 mins instead of staying the ~same time. --------- Co-authored-by: Tony Arcieri <[email protected]>
tarcieri
force-pushed
the
modulus-size-checks
branch
from
5917602 to
216a904
Compare
Changes the existing checked APIs to respect a minimum modulus size in addition to a maximum one.
Note: several tests fail because of this, so we'll need to go through them and convert to an unchecked API where appropriate (or decide if the test is bogus to begin with).
Closes #445