Golang-style salt lengths to verify RSA PSS sigs #262

trishankatdatadog · 2020-07-21T17:51:28Z

Signed-off-by: Trishank Karthik Kuppusamy [email protected]

Fixes issue #:

N/A

Description of the changes being introduced by the pull request:

Golang-style salt lengths to allow us to verify RSA PSS signatures produced in Go-based software such as Hashicorp Vault.

Please verify and check that the pull request fulfils the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

coveralls · 2020-07-21T17:54:10Z

Coverage decreased (-0.04%) to 98.746% when pulling 7edc11f on trishankatdatadog:trishankatdatadog/options-for-rsa-pss-salt-length into bbf1505 on secure-systems-lab:master.

securesystemslib/rsa_keys.py

shibumi · 2020-07-31T23:48:50Z

Coverage decreased (-98.9%) to 0.0% when pulling d148b64 on trishankatdatadog:trishankatdatadog/options-for-rsa-pss-salt-length into 6f88f63 on secure-systems-lab:master.

Lol.. this sounds odd.

joshuagl

This looks like a solid change, thanks @trishankatdatadog.

Travis is failing because coverage dropped to 98% with the addition of the code, could you please add some test coverage for SaltLength? Testing the Go compatible PSSSaltLengthAuto would be excellent.

securesystemslib/rsa_keys.py

trishankatdatadog · 2020-08-04T00:17:51Z

@shibumi @joshuagl Addressed your comments by adding tests and documentation.

I'm not happy with fixing the RSA-PSS salt length in the code, instead of as part of signed metadata on how to use the public key. We need to design a more sensible key distribution format going forward instead of using a single scheme string to encode hashing+signing+marshaling algorithms+etc.

joshuagl

Nice work @trishankatdatadog. I have some minor comments in-line, but otherwise this PR looks good to me.

securesystemslib/rsa_keys.py

tests/test_rsa_keys.py

pylintrc

securesystemslib/rsa_keys.py

tests/test_rsa_keys.py

securesystemslib/rsa_keys.py

tox.ini

trishankatdatadog · 2020-08-19T19:01:14Z

Done. Let me know if there are major issues. Any major objections @lukpueh?

lukpueh

Many thanks for the enhancements, @trishankatdatadog! Here are some high-level observations:

Tests need to pass
Could you please move the linter configuration and hence resulting style/format fixes to a separate PR?
Could you please squash your commits as appropriate and add a bit more info about the changes to the commit messages?
There should be no merge commits in PRs.

trishankatdatadog · 2020-08-21T14:49:11Z

Many thanks for the enhancements, @trishankatdatadog! Here are some high-level observations:

Is this OK?

lukpueh · 2020-08-24T08:43:13Z

Thanks for the updates! More details in the commit message would still be nice.

Previously, the securesystemslib RSA-PSS signature creation and verification code was fixed at using a salt length equal to the hash output length. This commit changes that to allow end-users to use a salt length that is the maximum available. The original motivation for this commit was to be able to verify RSA-PSS signatures generated by the Golang crypto RSA package, which defaults to the maximum salt length. For more information on how Golang crypto and Python cryptography handle salt lengths, please see: https://github.com/golang/go/blob/11f92e9dae96939c2d784ae963fa7763c300660b/src/crypto/rsa/pss.go#L225-L232 https://cryptography.io/en/3.1/hazmat/primitives/asymmetric/rsa/#cryptography.hazmat.primitives.asymmetric.padding.PSS.MAX_LENGTH For more information on salt lengths and how they relate to the proof of the security of the RSA-PSS scheme, please see: https://crypto.stackexchange.com/a/1222 This commit also includes test for the above changes, and some minor, unrelated formatting changes for pylint so that tests pass. Signed-off-by: Trishank Karthik Kuppusamy <[email protected]>

trishankatdatadog · 2020-11-23T08:05:12Z

Thanks for the updates! More details in the commit message would still be nice.

Finally done! Please take a last look...

trishankatdatadog · 2021-02-10T14:31:38Z

Any news, please? @lukpueh

lukpueh

Apologies for the late review. I think this looks quite good. I would probably implement it a bit differently, but I guess that's a matter of taste. Please remove unrelated changes and I can give it another pass.

lukpueh · 2021-02-25T17:19:42Z

securesystemslib/rsa_keys.py

+# its crypto package.
+# https://github.com/golang/go/blob/11f92e9dae96939c2d784ae963fa7763c300660b/src/crypto/rsa/pss.go#L225-L232
+# FIXME: really, we should encode the salt length as part of the metadata on
+# how to use the RSA-PSS public key.


👍 Agreed. Maybe configured via the "scheme" field? See #308.

lukpueh · 2021-02-25T17:29:39Z

securesystemslib/rsa_keys.py

+class SaltLengthType:
+  """A class to represent common salt lengths for RSA-PSS."""
+
+  @classmethod
+  def get_salt_length(self, algorithm):
+    """Get the salt length as integer."""
+    raise NotImplementedError
+
+
+
+
+
+# NOTE: This is what used to be the standard behaviour.
+class HashSaltLengthType(SaltLengthType):
+  """Salt length to equal the length of the hash used in the signature."""
+
+  @classmethod
+  def get_salt_length(cls, algorithm):
+    """Get the salt length as integer."""
+    return algorithm.digest_size
+
+
+
+
+
+class MaxSaltLengthType(SaltLengthType):
+  """Salt length in a PSS signature to be as large as possible when
+  signing, and to be auto-detected when verifying."""
+
+  @classmethod
+  def get_salt_length(cls, algorithm):
+    """Get the salt length as integer."""
+    # NOTE: We disregard algorithm here.
+    return padding.PSS.MAX_LENGTH


Smart architecture. :) Do we expect other SaltLengthType implementations that warrant an interface? Wouldn't a simple if/elif switch akin to the go code you referenced be just as good and a little less "engineered"?

lukpueh · 2021-02-25T17:39:42Z

securesystemslib/rsa_keys.py

+
+
+def create_rsa_signature(private_key, data, scheme='rsassa-pss-sha256',
+  salt_length_type=HashSaltLengthType):


I'm a bit worried how this extra parameter will fit in with the regular securesystemslib usage patterns, where algorithmic specific sign functions are only called indirectly via securesystemslib.keys.create_signature, dispatching based on the signing scheme field of the private key. So encoding the salt length type in the key, as you suggested above, is probably the way to go. But we can do this in a follow-up PR.

lukpueh · 2021-02-25T17:44:32Z

securesystemslib/rsa_keys.py

@@ -278,6 +327,10 @@ def create_rsa_signature(private_key, data, scheme='rsassa-pss-sha256'):
    scheme:
      The signature scheme used to generate the signature.

+    salt_length_type:
+      The strategy for determining the length of the salt used in RSA-PSS, one of


It's a bit ironic that there are plenty of unrelated re-wraps in your diff to match the prescribed line length, but a line that you added exceeds it.

I wish it was the other way around. Could you please remove non-related style changes from this PR as requested in an earlier review.

lukpueh · 2021-02-25T17:46:26Z

securesystemslib/rsa_keys.py

@@ -381,7 +437,8 @@ def create_rsa_signature(private_key, data, scheme='rsassa-pss-sha256'):



-def verify_rsa_signature(signature, signature_scheme, public_key, data):
+def verify_rsa_signature(signature, signature_scheme, public_key, data,
+  salt_length_type=HashSaltLengthType):


Same as above, we usually just call securesystemslib.keys.verify_signature and use the key to dispatch to the algorithm specific function.

trishankatdatadog · 2021-03-01T15:52:29Z

Thanks, Lukas, but I'm afraid I don't have the bandwidth to work on this PR anymore, especially since there are no plans to upstream my other PR which depends on this one. Should we just close this for now and revisit in the future?

trishankatdatadog · 2021-03-02T14:45:36Z

Note to @woodruffw though: you will need something like this PR to finish your Vault-TUF (or is it Vault-securesystemslib) integration. Closing for now.

shibumi reviewed Jul 31, 2020

View reviewed changes

securesystemslib/rsa_keys.py Outdated Show resolved Hide resolved

securesystemslib/rsa_keys.py Outdated Show resolved Hide resolved

joshuagl requested changes Aug 3, 2020

View reviewed changes

securesystemslib/rsa_keys.py Outdated Show resolved Hide resolved

securesystemslib/rsa_keys.py Outdated Show resolved Hide resolved

securesystemslib/rsa_keys.py Outdated Show resolved Hide resolved

joshuagl requested changes Aug 5, 2020

View reviewed changes

shibumi mentioned this pull request Aug 5, 2020

key format inconsistency #251

Closed

joshuagl reviewed Aug 18, 2020

View reviewed changes

lukpueh reviewed Aug 20, 2020

View reviewed changes

trishankatdatadog force-pushed the trishankatdatadog/options-for-rsa-pss-salt-length branch 2 times, most recently from bb248a9 to deb1cb1 Compare August 21, 2020 14:48

trishankatdatadog force-pushed the trishankatdatadog/options-for-rsa-pss-salt-length branch from deb1cb1 to 7edc11f Compare November 23, 2020 08:01

lukpueh requested changes Feb 25, 2021

View reviewed changes

trishankatdatadog closed this Mar 2, 2021

This was referenced Aug 3, 2022

Allow configuring the possible salt lengths for RSA PSS signatures hashicorp/vault#16549

Merged

Allow configuring the possible salt lengths for RSA PSS signatures hvac/hvac#846

Merged

trishankatdatadog mentioned this pull request Aug 11, 2022

Cross-platform verification of RSA PSS signatures #421

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Golang-style salt lengths to verify RSA PSS sigs #262

Golang-style salt lengths to verify RSA PSS sigs #262

trishankatdatadog commented Jul 21, 2020

coveralls commented Jul 21, 2020 •

edited

Loading

shibumi commented Jul 31, 2020

joshuagl left a comment

trishankatdatadog commented Aug 4, 2020 •

edited

Loading

joshuagl left a comment

trishankatdatadog commented Aug 19, 2020

lukpueh left a comment

trishankatdatadog commented Aug 21, 2020

lukpueh commented Aug 24, 2020

trishankatdatadog commented Nov 23, 2020

trishankatdatadog commented Feb 10, 2021

lukpueh left a comment

lukpueh Feb 25, 2021

lukpueh Feb 25, 2021

lukpueh Feb 25, 2021 •

edited

Loading

lukpueh Feb 25, 2021

lukpueh Feb 25, 2021

trishankatdatadog commented Mar 1, 2021

trishankatdatadog commented Mar 2, 2021



		def create_rsa_signature(private_key, data, scheme='rsassa-pss-sha256',
		salt_length_type=HashSaltLengthType):

Golang-style salt lengths to verify RSA PSS sigs #262

Golang-style salt lengths to verify RSA PSS sigs #262

Conversation

trishankatdatadog commented Jul 21, 2020

coveralls commented Jul 21, 2020 • edited Loading

shibumi commented Jul 31, 2020

joshuagl left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Aug 4, 2020 • edited Loading

joshuagl left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Aug 19, 2020

lukpueh left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Aug 21, 2020

lukpueh commented Aug 24, 2020

trishankatdatadog commented Nov 23, 2020

trishankatdatadog commented Feb 10, 2021

lukpueh left a comment

Choose a reason for hiding this comment

lukpueh Feb 25, 2021

Choose a reason for hiding this comment

lukpueh Feb 25, 2021

Choose a reason for hiding this comment

lukpueh Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

lukpueh Feb 25, 2021

Choose a reason for hiding this comment

lukpueh Feb 25, 2021

Choose a reason for hiding this comment

trishankatdatadog commented Mar 1, 2021

trishankatdatadog commented Mar 2, 2021

coveralls commented Jul 21, 2020 •

edited

Loading

trishankatdatadog commented Aug 4, 2020 •

edited

Loading

lukpueh Feb 25, 2021 •

edited

Loading