Revert to digest sized salts in rsassa-pss signing #436
Conversation
In secure-systems-lab#422 we changed `create_rsa_signature` to use the maximum salt length available, instead of the digest length, when creating rsassa-pss signatures, and adapted `verify_rsa_signature` to infer the salt length automatically. This made it impossible for users of the old verify function, which could only handle digest sized salts, to verify signatures created by users of the new signing function (see secure-systems-lab#430). Since the advantage of max salt lengths is mostly academic, this patch reverts `create_rsa_signature` to use digest sized salts (as agreed in secure-systems-lab#431). Note that we now use the `padding.PSS.DIGEST_LENGTH` constant instead of passing the actual digest length, as we did before. Using the constant has the same result, but is recommended by the library documentation. Also note that the patch does not revert the `verify_rsa_signature` part of secure-systems-lab#422. This allows verifying signatures created with the securesystemslib release that used max salt lengths, or created outside of securesystemslib. Signed-off-by: Lukas Puehringer <[email protected]>
|
@trishankatdatadog, @yzhan289, does this work for you? |
| signature = private_key_object.sign( | ||
| data, padding.PSS(mgf=padding.MGF1(digest_obj.algorithm), | ||
| salt_length=padding.PSS.MAX_LENGTH), digest_obj.algorithm) | ||
| salt_length=padding.PSS.DIGEST_LENGTH), digest_obj.algorithm) |
There was a problem hiding this comment.
The original code had digest_obj.algorithm.digest_size instead, is there a difference here?
There was a problem hiding this comment.
See PR description / commit mesesage:
Note that we now use the
padding.PSS.DIGEST_LENGTHconstant instead of passing the actual digest length, as we did before. Using the constant has the same result, but is recommended by the library documentation.
There was a problem hiding this comment.
I locally created two signature one with the constant and one with the digest length and can confirm that they do the same thing.
I also checked the code.
There was a problem hiding this comment.
There was a problem hiding this comment.
Right: I think padding.PSS.DIGEST_LENGTH is simply an internal cryptography constant that does look like it's used to end up using digest_obj.algorithm.digest_size in the end. This flag is also much easier to understand.
There was a problem hiding this comment.
Thanks! Oof my bad I didn't read the description first 🤦
| rsa_scheme = 'rsassa-pss-sha256' | ||
| data = 'The ancients say the longer the salt, the more provable the security'.encode('utf-8') | ||
| data = 'The ancients say, salt length does not matter that much'.encode('utf-8') |
There was a problem hiding this comment.
Mathematically speaking, apparently this is not strictly true, although practically it might be the case 😉
There was a problem hiding this comment.
Yeah, I would also not trust every word the ancients say. ;)
| signature = private_key_object.sign( | ||
| data, padding.PSS(mgf=padding.MGF1(digest_obj.algorithm), | ||
| salt_length=padding.PSS.MAX_LENGTH), digest_obj.algorithm) | ||
| salt_length=padding.PSS.DIGEST_LENGTH), digest_obj.algorithm) |
There was a problem hiding this comment.
Right: I think padding.PSS.DIGEST_LENGTH is simply an internal cryptography constant that does look like it's used to end up using digest_obj.algorithm.digest_size in the end. This flag is also much easier to understand.
tests/test_rsa_keys.py
Outdated
| @@ -27,6 +27,7 @@ | |||
|
|
|||
| from cryptography.hazmat.backends import default_backend | |||
| from cryptography.hazmat.primitives.asymmetric import padding | |||
| from cryptography.hazmat.primitives.hashes import SHA256 | |||
There was a problem hiding this comment.
Unused constant? We really need a linter.
There was a problem hiding this comment.
And yes, we need a linter!! I've been hesitant to turn one on because of the flag day it will entail, and because I hope to soon remove larger chunks of internals (see #270). But I don't know how quickly I will get to the latter. So maybe the flag day is okay?
There was a problem hiding this comment.
What's a "flag day"?
There was a problem hiding this comment.
Ultimately, I think a linter (and using a standard coding style) makes the code easier to work on so I am in favour of the flag day (event where the entire codebase changes to the new style, making a large diff with only mechanical changes which invalidate patches and make revision control "noise").
Signed-off-by: Lukas Puehringer <[email protected]>
|
Thanks very much, @lukpueh and friends! No rush, but in which release could we expect this, please? |
Fixes #430, Supersedes #431
In #422 we changed
create_rsa_signatureto use the maximum salt length available, instead of the digest length, when creating rsassa-pss signatures, and adaptedverify_rsa_signatureto infer the salt length automatically.This made it impossible for users of the old verify function, which could only handle digest sized salts, to verify signatures created by users of the new signing function (see #430).
Since the advantage of max salt lengths is mostly academic, this patch reverts
create_rsa_signatureto use digest sized salts.Note that we now use the
padding.PSS.DIGEST_LENGTHconstant instead of passing the actual digest length, as we did before. Using the constant has the same result, but is recommended by the library documentation.Also note that the patch does not revert the
verify_rsa_signaturepart of #422. This allows verifying signatures created with the securesystemslib release that used max salt lengths, or created outside of securesystemslib.