-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Allow RSA signing with raw data (without a DigestInfo) #13740
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
base: main
Are you sure you want to change the base?
Conversation
|
I haven't reviewed in depth -- but I don't think we should use @reaperhulk wdyt? |
|
@alex yeah that is a fair concern 👍 in some early attempt I did add a I'll be happy to apply what you folks think is the best, if possible, please provide pointers where in the code-base a similar pattern is used so I can take inspiration from it. |
I would like to highlight that the API already uses None for this purpose (for the RSA signature recover functionality, ref. issue #5495). So whatever you decide, you may want to use the same method in all the API functions to make them symmetric. |
|
Hmm, the inconsistency is a bit unfortunate. I'd be inclined to do a |
|
@alex @reaperhulk I did add a sentinel Please have an other look and let me know what you think |
cca3371 to
31d0cfd
Compare
reaperhulk
left a comment
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.
We also need docs for this new value in RSAPrivateKey.sign
| encode_dss_signature = asn1.encode_dss_signature | ||
|
|
||
|
|
||
| class NoDigestInfo: |
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.
We need to document this in the same section as Prehashed
| signature = private_key.sign( | ||
| binascii.unhexlify( | ||
| compute_rsa_hash_digest( | ||
| backend, hashes.SHA1(), example["message"] |
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.
I'd prefer we used SHA256 for the tests here because otherwise we'll increasingly have coverage challenges as more and more things disable SHA1.
|
|
||
| # Test recovery of all data (full DigestInfo) with hash alg. as | ||
| # None | ||
| rec_sig_data = public_key.recover_data_from_signature( |
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.
We need to document that recover_data_from_signature now takes this new argument.
| backend.signature_hash_supported(hashes.SHA1()) | ||
| and backend.rsa_padding_supported(padding.PKCS1v15()) | ||
| ), | ||
| skip_message="Does not support SHA1 and PKCS1v1.5.", |
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.
Same SHA1 concern
This fixes #3713 and #10226.
Instead of using the script written by @misterzed88 in #10226 to generate modified tests vectors, I did directly implement the same logic in the test infrastructure, so we can reuse directly the NIST (or others...) vectors stored in the tests.