fix(public_key): correctly strip DER leading zero in RSA key_size()#236
Open
sumleo wants to merge 1 commit intorusticata:masterfrom
Open
fix(public_key): correctly strip DER leading zero in RSA key_size()#236sumleo wants to merge 1 commit intorusticata:masterfrom
sumleo wants to merge 1 commit intorusticata:masterfrom
Conversation
cpu
reviewed
Feb 22, 2026
Collaborator
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
IMO this merits test coverage. I'd also prefer one commit instead of one with the change and one addressing a linting finding in the previous change.
sumleo
force-pushed
the
fix/rsa-key-size
branch
from
af03ddc to
7f76b92
Compare
cpu
reviewed
Mar 9, 2026
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
Collaborator
There was a problem hiding this comment.
I suppose this is better than nothing, but it seems conceivable you could generate real keys with the required properties and test them end-to-end through x509-parser as an integration test instead.
The previous implementation unconditionally stripped the first byte of the modulus when the MSB was clear, which is incorrect. DER encoding only prepends a 0x00 byte when the MSB of the actual value is set. This fix checks specifically for a leading 0x00 padding byte and only strips that, correctly computing the key size for all RSA keys. Add both unit tests for edge cases and integration tests that parse real DER certificates with 1024, 2048, and 4096-bit RSA keys to verify key_size() end-to-end.
sumleo
force-pushed
the
fix/rsa-key-size
branch
from
7f76b92 to
fd49368
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RSAPublicKey::key_size()incorrectly calculated the key size by checking if the first byte's MSB was unset (modulus[0] & 0x80 == 0) and unconditionally stripping the first byte0x00padding byte before stripping it, and returns the key size for all non-empty modulus values (not just those with MSB unset)Test plan
cargo test --all-features