Support signing a pre-calculated hash #87
Conversation
This code change adds support to split the hashing of a message and the actual signing of the message.
rsa/pkcs1.py
Outdated
| """ | ||
|
|
||
| # Verify hash_method is a valid hash algorithm | ||
| if hash_method not in HASH_ASN1: |
There was a problem hiding this comment.
Why perform this check here? It's the same as in sign_hash() anyway.
There was a problem hiding this comment.
This was an oversight in my original commit. This is removed in the new commit.
This commit updates the unit test and usage docs. In addition, This change removes a redundant error check inside rsa.sign().
|
I added 2 new unit tests to cover the new hashing flow and updated the usage docs with the new flow. I am new to this type of documentation. Let me know if I missed anything. |
rsa/pkcs1.py
Outdated
| """ | ||
|
|
||
| # Calculate the hash and perform the signing | ||
| return sign_hash(hash(message, hash_method), priv_key, hash_method) |
There was a problem hiding this comment.
Please splitt his up into two lines, using a helper variable. The way it is now it does too much on one line; a traceback referring to an error in this line will be too ambiguous.
There was a problem hiding this comment.
Sure, this will be fixed in the next commit.
tests/test_pkcs1.py
Outdated
| signature1 = pkcs1.sign_hash(msg_hash, self.priv, 'SHA-256') | ||
| print("\tSignature1: %r" % signature1) | ||
|
|
||
| # Calculate the signature using the original method |
There was a problem hiding this comment.
The comment "the original method" implies that the reader knows that your method is something that was added later. This is only true within the context of this pull request, but not later once everything has been merged.
There was a problem hiding this comment.
Agreed, I switched it to now say unified method.
tests/test_pkcs1.py
Outdated
| """Hashing and then signing should match with directly signing the message. """ | ||
|
|
||
| message = b'je moeder' | ||
| print("\tMessage: %r" % message) |
There was a problem hiding this comment.
Please don't print things in a unit test.
There was a problem hiding this comment.
I was using the other test cases in this file as a template and many of them had prints. The prints are now removed.
Removed the print statements from the unit test and refactored a few code comments to improve readability.
|
Another thing: |
The new hash function had the same name as a function in the standard library. This commit changes the name to avoid conflicts.
|
Hmm not too fond of "generate", maybe "compute_hash"? |
This commit renames the hash function to compute_hash().
|
Thanks, it's in! |
This code change adds a new function that takes a pre-computed hash of a message that should be signed (as apposed to a message). This change allows one process to generate the hash of message and another process (over RPC in my case) to perform the signing.