Support for RSA-PKCS1v15-HASH_ID scheme #173

danixeee · 2019-08-12T16:36:39Z

Add support for RSA-PKCS1v15 signatures, as well as other supported openssl hash algorithms.

Description of the changes being introduced by the pull request:

Allow passing one of the following schemes, when generating keys and creating/verifying signatures:

rsassa-pss-HASH_ID
rsa-pkcs1v15-HASH_ID

Where HASH_ID is one of: md5, sha1, sha224, sha256, sha384 and sha512.

HASH_ID is picked from a scheme, so changes are backwards compatible.

Please verify and check that the pull request fulfills 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

danixeee · 2019-08-13T17:29:42Z

Hi @lukpueh, I didn't see that you are working on gpg transition on your fork. I was looking at branches that belongs to this repository, but now I can see that our work does not overlap, which is great. :)

I guess we should continue the conversation from #163 here.

Note that I didn't adapt SIGNATURE_SCHEMA in #174 as you did here, because it felt like a bigger change. I'd rather do this in a separate PR, maybe as part of a broader formats/schema revision. Do you think you can work around this by adding something like below to your taf.formats and patching append_signature accordingly?

Do you think I should merge your work (#174) to mine (#173) and update things that you have proposed in this PR?

lukpueh

Thanks for the solid PR, @danixeee! I have a few minor comments inline and two style wishes below. Please consider addressing them and we should be able to merge this rather soon.

minor style wishes:

Would you mind removing the trailing whitespace you added in hash.py (see e.g. git diff --check)?
Would you please revert the alignments of continued lines in pyca_crypto_keys.py that are unrelated to this PR (was that your IDE?)?
In general, I think our code style is in favor of hanging indent, although not strictly according to pep8, i.e. arguments on the first line are okay. IMO vertical alignment is okay, but it shouldn't exceed the max line length of 80 characters.

lukpueh · 2019-08-14T08:56:08Z

tests/test_formats.py

+
+      'RSA_SCHEME_SCHEMA_PKCS1v15_4': (securesystemslib.formats.RSA_SCHEME_SCHEMA,
+          'rsa-pkcs1v15-sha512'),
+


Just for completeness the sake of completeness, could you add two more lines for sha384?

lukpueh · 2019-08-14T09:16:41Z

securesystemslib/keys.py

+  'rsa-pkcs1v15-sha256',
+  'rsa-pkcs1v15-sha512',
+]
+


Are the sha384 variants left out on purpose?

On an unrelated side-note: I really think securesystemslib should have a constants module that's also used to define schemas in the formats module, so that we don't re-define the same string constants in the different places. We can leave it like it is here, but I'll follow up with an issue about it.

I left it accidentally, thanks.

lukpueh · 2019-08-14T10:35:25Z

securesystemslib/hash.py

+
+  # Dictionary of `pyca/cryptography` supported hash algorithms.
+  PYCA_DIGEST_OBJECTS_CACHE = {hash_algo.name: hash_algo
+                               for hash_algo in cryptography.hazmat.primitives.hashes.HashAlgorithm._abc_registry}


This is pretty neat but it has a few downsides:

Just by looking at it we don't know what hash algos we get here,

We actually get a lot more algos than we "support" (or test for):
>>> list(PYCA_DIGEST_OBJECTS_CACHE.keys()) ['sha224', 'md5', 'sha1', 'sha3-224', 'blake2s', 'sha384', 'shake256', 'sha3-256', 'blake2b', 'sha512-224', 'sha256', 'sha3-512', 'sha512-256', 'sha3-384', 'shake128', 'sha512']

abc.ABC._abc_registry is not officially documented and e.g. won't be available in Python 3.7.

So for the sake of readability and explicitness I suggest a less neat solution, e.g.:

import cryptography.hazmat.primitives.hashes as _pyca_hashes PYCA_DIGEST_OBJECTS_CACHE = { "md5": pyca_hashes.MD5, "sha1": _pyca_hashes.SHA1, "sha224": _pyca_hashes.SHA224, "sha256": _pyca_hashes.SHA256, "sha384": _pyca_hashes.SHA384, "sha512": _pyca_hashes.SHA512 }

I could also see a compromise of neat and explicit like...

import cryptography.hazmat.primitives.hashes SUPPORTED_HASH_ALGOS = ["md5", "sha1"] # ... PYCA_DIGEST_OBJECTS_CACHE = { algo: vars(cryptography.hazmat.primitives.hashes)[algo.upper()] for algo in SUPPORTED_HASH_ALGOS}

What do you think?

Agreed, I think that explicit way is the most readable. I had similar code already (https://github.com/openlawlibrary/securesystemslib/blob/e5051e610a6e6d3c58c9b72d3d79e1ed955ea188/securesystemslib/pyca_crypto_keys.py#L159), not sure why I changed it. :)

lukpueh · 2019-08-14T11:50:26Z

securesystemslib/hash.py

-    pass
+  elif hash_library == 'pyca_crypto' and hash_library in SUPPORTED_LIBRARIES:
+    try:
+      global PYCA_DIGEST_OBJECTS_CACHE


I don't think you need the global keyword here.

lukpueh · 2019-08-14T11:51:02Z

securesystemslib/hash.py

+      hash_algorithm = PYCA_DIGEST_OBJECTS_CACHE[algorithm]()
+      return Pyca_Diggest_Wrapper(
+        cryptography.hazmat.primitives.hashes.Hash(hash_algorithm,
+        cryptography.hazmat.backends.default_backend()))


Please double indent continued lines.

lukpueh · 2019-08-14T11:55:55Z

securesystemslib/hash.py

+
+  SUPPORTED_LIBRARIES.append('pyca_crypto')
+
+  class Pyca_Diggest_Wrapper(object):


Minor nit: We usually use UpperCamelCase for classes

lukpueh · 2019-08-14T12:24:54Z

securesystemslib/hash.py

+  # Get hash algorithm from rsa scheme (hash algorithm id is specified after
+  # the last dash; e.g. rsassa-pss-sha256 -> sha256)
+  hash_algorithm = scheme.split('-')[-1]
+  return digest(hash_algorithm, hash_library)


Please add newline

lukpueh · 2019-08-14T12:51:29Z

securesystemslib/pyca_crypto_keys.py

@@ -302,51 +305,56 @@ def create_rsa_signature(private_key, data, scheme='rsassa-pss-sha256'):
  # 'private_key' has variable size and can be an empty string.
  if len(private_key):


Since you already nicely re-wrote most of this function, would you mind changing above to:

if not len(private_key): raise ValueError('The required private key is unset.')

... and move the "try/except/except/except" block on indentation level inwards. I think this makes quite a difference in terms of readability. :)

lukpueh · 2019-08-14T13:19:51Z

Do you think I should merge your work (#174) to mine (#173) and update things that you have proposed in this PR?

No need to do that. Let's treat them separately while reviewing, and rebase the pending one, once the other one gets merged. I suspect your's will be merged sooner than mine, so I'll do the rebase on my side. :)

danixeee · 2019-08-14T16:03:39Z

@lukpueh I think that I have addressed all your comments for now, thank you for the fast review!

Would you please revert the alignments of continued lines in pyca_crypto_keys.py that are unrelated to this PR (was that your IDE?)?

It was, I am using VS Code but I disabled all formatters, not sure why it changed those alignments. It should be fine now.

Do you think I should merge your work (#174) to mine (#173) and update things that you have proposed in this PR?

I asked this because I will need gpg formats from your PR in order to change schemas. Should I copy them manually then?

coveralls · 2019-08-14T17:04:48Z

Coverage remained the same at 100.0% when pulling 3116ee2 on openlawlibrary:danixeee/rsa-pkcs1v15-scheme into 6ef6c61 on secure-systems-lab:master.

lukpueh

LGTM. Thanks! :)

lukpueh · 2019-08-14T19:05:16Z

I asked this because I will need gpg formats from your PR in order to change schemas. Should I copy them manually then?

I'll merge this PR now and will rebase my lukpueh:add-gpg from #174 onto master. I suggest you work on top of lukpueh:add-gpg then. It shouldn't change a lot, because the logic has already been reviewed in in-toto. However, it still needs someone to take a look at it before we merge it here (cc @SantiagoTorres, @mnm678).

danixeee added 7 commits August 8, 2019 15:11

Add rsa-pkcs1v15-sha256 scheme

e5051e6

Update RSA_SCHEME_SCHEMA

a957444

Add formats tests

c3a45b1

Add tests

205c5c2

Change formatting

07ed98c

File formatting

fa4614d

Support for pyca/crypto hash library

c6819ed

JustinCappos assigned lukpueh Aug 12, 2019

lukpueh requested changes Aug 14, 2019

View reviewed changes

danixeee added 2 commits August 14, 2019 17:43

Address CR comments

36cd762

Change formatting

3116ee2

danixeee mentioned this pull request Aug 14, 2019

Implement yubikey API by using yubikey-manager lib (remove oll-sc) openlawlibrary/taf#58

Merged

4 tasks

lukpueh approved these changes Aug 14, 2019

View reviewed changes

lukpueh merged commit d6227b4 into secure-systems-lab:master Aug 14, 2019

danixeee mentioned this pull request Aug 15, 2019

Add pkcs1v15 signature schema openlawlibrary/securesystemslib#4

Merged

3 tasks

danixeee deleted the danixeee/rsa-pkcs1v15-scheme branch August 21, 2019 11:49

danixeee mentioned this pull request Sep 20, 2019

Changing signature schemes without rotating the keys theupdateframework/python-tuf#920

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for RSA-PKCS1v15-HASH_ID scheme #173

Support for RSA-PKCS1v15-HASH_ID scheme #173

danixeee commented Aug 12, 2019 •

edited

Loading

danixeee commented Aug 13, 2019

lukpueh left a comment

lukpueh Aug 14, 2019

lukpueh Aug 14, 2019

lukpueh Aug 14, 2019

danixeee Aug 14, 2019

lukpueh Aug 14, 2019

danixeee Aug 14, 2019

lukpueh Aug 14, 2019

lukpueh Aug 14, 2019

lukpueh Aug 14, 2019

lukpueh Aug 14, 2019

lukpueh Aug 14, 2019

lukpueh commented Aug 14, 2019

danixeee commented Aug 14, 2019

coveralls commented Aug 14, 2019 •

edited

Loading

lukpueh left a comment

lukpueh commented Aug 14, 2019


		'RSA_SCHEME_SCHEMA_PKCS1v15_4': (securesystemslib.formats.RSA_SCHEME_SCHEMA,
		'rsa-pkcs1v15-sha512'),


		SUPPORTED_LIBRARIES.append('pyca_crypto')

		class Pyca_Diggest_Wrapper(object):

		@@ -302,51 +305,56 @@ def create_rsa_signature(private_key, data, scheme='rsassa-pss-sha256'):
		# 'private_key' has variable size and can be an empty string.
		if len(private_key):

Support for RSA-PKCS1v15-HASH_ID scheme #173

Support for RSA-PKCS1v15-HASH_ID scheme #173

Conversation

danixeee commented Aug 12, 2019 • edited Loading

danixeee commented Aug 13, 2019

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Aug 14, 2019

danixeee commented Aug 14, 2019

coveralls commented Aug 14, 2019 • edited Loading

lukpueh left a comment

Choose a reason for hiding this comment

lukpueh commented Aug 14, 2019

danixeee commented Aug 12, 2019 •

edited

Loading

coveralls commented Aug 14, 2019 •

edited

Loading