Implement TAP 9 (Mandatory metadata signing schemes) #48

vladimir-v-diaz · 2017-08-10T20:21:55Z

TAP 9 removes the method attribute from signed metadata to prevent malicious actors from controlling it. Instead, the signature scheme is now specified in the "signed" portion of metadata.

This pull request refactors a few functions to accept a signature scheme argument. In addition, the signature scheme is now included in the "keys" field of signed metadata.

TAP 9 and this implementation are backwards incompatible.

The create_signature() functions for keys.py and the specific library modules (e.g., pyca_crypto_keys.py) should be refactored to accept a 'scheme' argument

create_rsa_signature() and verify_rsa_signature() refactored to accept scheme argument

…hod->scheme

Also update its unit tests

coveralls · 2017-08-11T19:52:38Z

Coverage remained the same at 100.0% when pulling 1628118 on vladimir-v-diaz:tap9 into 42f8ae4 on secure-systems-lab:master.

lukpueh

Ignored pycrypto related diff, which is removed in #62.

lukpueh · 2017-10-30T20:30:03Z

securesystemslib/ecdsa_keys.py

@@ -179,12 +179,19 @@ def create_signature(public_key, private_key, data):
    data:
      Byte data used by create_signature() to generate the signature returned.

+    scheme:
+      The signature scheme used to generate the signature.  For example:
+      'ecesa-sha2-nistp256'.


ecdsa-sha2-nistp256

lukpueh · 2017-10-30T20:30:22Z

securesystemslib/ecdsa_keys.py

  <Exceptions>
    securesystemslib.exceptions.FormatError, if the arguments are improperly
    formatted.

    securesystemslib.exceptions.CryptoError, if a signature cannot be created.

+    securesystemslib.exceptions.UnsupportedAlgorithmError, if 'scheme' is
+    an not one of the supported signature schemes.


lukpueh · 2017-10-30T20:32:27Z

securesystemslib/ecdsa_keys.py

@@ -126,21 +126,21 @@ def generate_public_and_private(algorithm='ecdsa-sha2-nistp256'):
  # supported ECDSA algorithms.  It must conform to
  # 'securesystemslib.formats.ECDSAALGORITHMS_SCHEMA'.  Raise
  # 'securesystemslib.exceptions.FormatError' if the check fails.
-  securesystemslib.formats.ECDSAALGORITHMS_SCHEMA.check_match(algorithm)
+  securesystemslib.formats.ECDSA_SIG_SCHEMA.check_match(scheme)


Shouldn't this be ECDSA_SIG_SCHEME_SCHEMA now? (just kidding)

It's actually not purely a joke. Currently the schemas for signatures and signature schemes are not really distinguishable by their names. So *_SIG_SCHEME_SCHEMA might not be the worst of ideas? Especially since there already is a SIG_SCHEME_SCHEMA. :)

lukpueh · 2017-10-30T20:56:34Z

securesystemslib/ecdsa_keys.py

-      password=None, backend=default_backend())
+  # A defensive check for a valid 'scheme'.  The check_match() above
+  # should have already validated it...
+  if scheme == 'ecdsa-sha2-nistp256': #pragma: no cover


I don't see a reason for doing the exact same check twice in a row. I don't see a big problem either, but it feels strange to me to redo what ECDSA_SIG_SCHEMA.check_match(scheme) already does.

However, I would definitely remove the #pragma: no cover here, because that disables the decreased coverage alert when you remove tests that touch code inside the if-block.

Also, I'm not sure if it is a good idea to fail with different errors FormatError vs. UnsupportedAlgorithmError.

Maybe you can wrapECDSA_SIG_SCHEMA.check_match(scheme) in a try/except block and raise UnsupportedAlgorithmError on FormatError?

lukpueh · 2017-10-31T16:06:38Z

securesystemslib/ecdsa_keys.py

@@ -126,21 +126,21 @@ def generate_public_and_private(algorithm='ecdsa-sha2-nistp256'):
  # supported ECDSA algorithms.  It must conform to
  # 'securesystemslib.formats.ECDSAALGORITHMS_SCHEMA'.  Raise


There are still a couple of mentionings of algorithm that probably should be scheme now in comments, also the UnsupportedAlgorithmError is still used for unsupported schemes. I guess this is not a big deal.

lukpueh · 2017-10-31T16:12:39Z

securesystemslib/formats.py

  keyid = KEYID_SCHEMA,
  keyid_hash_algorithms = SCHEMA.Optional(HASHALGORITHMS_SCHEMA),
  keyval = KEYVAL_SCHEMA)

+# ECDSA signature schemes.
+ECDSA_SIG_SCHEMA = SCHEMA.OneOf([SCHEMA.String('ecdsa-sha2-nistp256')])


ECDSA_SIG_SCHEMA is already assigned in L187

lukpueh · 2017-10-31T16:14:15Z

securesystemslib/ecdsa_keys.py

+  # Is 'scheme' one of the supported ECDSA signature schemes?  A defensive
+  # check for a valid 'scheme'.  The check_match() above should have validated
+  # it...
+  if scheme in _SUPPORTED_ECDSA_SCHEMES: #pragma: no cover


See comment above about redundant checks with different Errors and #pragma: no cover. Btw. it seems that this comment applies to various places throughout the PR.

lukpueh · 2017-10-31T16:44:13Z

securesystemslib/keys.py

      The ECDSA algorithm.  By default, ECDSA NIST P-256 is used, with SHA256
      for hashing.

  <Exceptions>
-    securesystemslib.exceptions.FormatError, if 'algorithm' is improperly or invalid
-    (i.e., not one of the supported ECDSA algorithms).
+    securesystemslib.exceptions.FormatError, if 'algorithm' is improperly or


s/algorithm/scheme/ (not only here)

lukpueh · 2017-10-31T17:01:15Z

securesystemslib/pyca_crypto_keys.py

+      # encrypted, or if the key was encrypted but no password was supplied.
+      # Note: A passphrase or password is not used when generating
+      # 'private_key', since it should not be encrypted.
+      except TypeError: #pragma: no cover


Any reason for the #pragma: no cover here and with the ValueError above?

lukpueh · 2017-10-31T17:02:00Z

securesystemslib/pyca_crypto_keys.py

+      # encrypted with a symmetric cipher that is not supported by the backend.
+      except cryptography.exceptions.UnsupportedAlgorithm: #pragma: no cover
+        raise securesystemslib.exceptions.CryptoError('The private key is'
+          ' encrypted with a unsupported algorithm.')


Address review of pull request #48

vladimir-v-diaz added 30 commits August 7, 2017 12:59

Add argument for signature schemes to generate_rsa_key()

a4143ac

Add schemas for signature schemes

710c696

Test for the signature schemes

0ed5802

Add schemeta for specific signature schemes

4978009

Ensure signature schemes can be set on imported keys

6a5fd86

Add ecdsa signature schemata

55f261c

Edit ecdsa_keys.py to accept signature schemes [WIP]

211e6a4

Begin refactoring the create_signature functions

b5cd93b

The create_signature() functions for keys.py and the specific library modules (e.g., pyca_crypto_keys.py) should be refactored to accept a 'scheme' argument

Fix schema for sig scheme: rsassa-pss-256->rsassa-pss-sha256

523fb18

Fix doctests for pycrypto_keys.create_signature()

9305757

Fix argument name: signature_method->signature_scheme

c8bb55b

Refactor signature functions

94b1ff5

create_rsa_signature() and verify_rsa_signature() refactored to accept scheme argument

Fix default argument in keys.py

0c9b4ba

Change verify_rsa_signare() and verify_signature() argument name: met…

f7e398d

…hod->scheme

Add scheme argument to ed25519_keys.py signature functions

35be45f

Change remaining keys.py argument names: method->scheme

96afcbe

Change ecdsa_keys.py signature functions to use scheme argument

d7770a5

Fix remaining doctest failures in keys.py

5e888e1

Update/resolve pyca_crypto_key.py unit test failures

b76b3d9

Fix unit tests for pycrypto_keys.py

5d7425c

Resolve unit tests failures in ed25519_keys.py

92099a0

Resolve unit tests failures in test_ecdsa_keys.py

c2d4f00

Change rsa signature scheme in formats.py

3f03dc9

Verify signature scheme in keys.verify_signature()

990404c

Resolve unit test failures in test_keys.py

bcccd97

Add signature scheme argument to interface.py

7593196

Also update its unit tests

Fix typo in keys.py docstring

92d24f5

Add missing scheme argument to test_util.py

a3dc965

Replace remaining instances of signature 'method' in docstrings

343ef5c

Resolve conflicts

fb5e623

vladimir-v-diaz added 5 commits August 11, 2017 11:08

Fix code coverage for keys.py

573f5c4

Address code coverage for pycrypto_keys.py

c2a3c4b

Address code coverage for pyca_crypto_keys.py

b2ef419

Resolve code coverage for ecdsa_keys.py

c28d552

Resolve code coverage for ed25519_keys.py

1628118

vladimir-v-diaz merged commit 13bdc05 into secure-systems-lab:master Aug 11, 2017

lukpueh reviewed Oct 31, 2017

View reviewed changes

vladimir-v-diaz mentioned this pull request Nov 6, 2017

Address review of pull request #48 #80

Merged

3 tasks

vladimir-v-diaz added a commit that referenced this pull request Nov 6, 2017

Merge pull request #80 from vladimir-v-diaz/address_review_pr#48

177cecf

Address review of pull request #48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement TAP 9 (Mandatory metadata signing schemes) #48

Implement TAP 9 (Mandatory metadata signing schemes) #48

vladimir-v-diaz commented Aug 10, 2017

coveralls commented Aug 11, 2017

lukpueh left a comment

lukpueh Oct 30, 2017

lukpueh Oct 30, 2017

lukpueh Oct 30, 2017

lukpueh Oct 31, 2017

lukpueh Oct 30, 2017

lukpueh Oct 31, 2017

lukpueh Oct 31, 2017

lukpueh Oct 31, 2017

lukpueh Oct 31, 2017

lukpueh Oct 31, 2017

lukpueh Oct 31, 2017

lukpueh Oct 31, 2017

		@@ -126,21 +126,21 @@ def generate_public_and_private(algorithm='ecdsa-sha2-nistp256'):
		# supported ECDSA algorithms. It must conform to
		# 'securesystemslib.formats.ECDSAALGORITHMS_SCHEMA'. Raise

Implement TAP 9 (Mandatory metadata signing schemes) #48

Implement TAP 9 (Mandatory metadata signing schemes) #48

Conversation

vladimir-v-diaz commented Aug 10, 2017

coveralls commented Aug 11, 2017

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

Choose a reason for hiding this comment