WIP: sphincs+ support, for post-quantum crypto #160

awwad · 2018-11-20T17:10:36Z

This PR is to create a place to discuss the changes that @cryptojedi and @joostrijneveld have recommended for adding sphincs+ post-quantum crypto support to secure-systems-lib as a means to allow TUF and in-toto to make use of it.

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

Only the passing case was copied from ed25519, and inserted between the passing and failure case. This effectively removed error handling for incorrect ed25519 schemes.

awwad · 2018-11-20T17:12:54Z

I think that it's important that sphincs support and the use of pyspx (which is a bit new and could use more testing) be optional (not a requirement for ssl installation). What do you think, @SantiagoTorres?

awwad · 2018-11-20T17:15:55Z

securesystemslib/interface.py

@@ -900,6 +900,260 @@ def import_ecdsa_privatekey_from_file(filepath, password=None):
  return key_object


+def generate_and_write_spx_keypair(filepath=None, password=None):


Sidenote (doesn't have to be done for this PR): much of the code in this function is duplicated in multiple places: generate_and_write_..._keypair. This makes it clear that we should modularize the generate functions and do things like argument checking, password prompting and format validation, temp file creation, writing, etc. in one place used by all the generate_... functions.

The same goes for import_..._privatekey_....

awwad · 2018-11-20T17:17:26Z

securesystemslib/interface.py

+
+
+
+def import_spx_publickey_from_file(filepath):


Comment requiring no change to this PR: this function can be generalized across all keytypes.

SantiagoTorres · 2018-11-20T17:18:52Z

ACK, How time-sensitive is this review? :) Thanks! -Santiago.

…

On Tue, Nov 20, 2018 at 09:12:54AM -0800, Sebastien Awwad wrote: I think that it's important that sphincs support and the use of pyspx (which is a bit new and could use more testing) be optional (not a requirement for ssl installation). What do you think, @SantiagoTorres? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #160 (comment)

awwad · 2018-11-20T17:20:29Z

Just wanted to know if you have thoughts on optional-vs-required. No rush on that question.

awwad · 2018-11-20T18:51:38Z

@cryptojedi and @joostrijneveld: This looks good. I'll make more comments later, but I very much appreciate the effort to make this fit the style of the code and include good docstrings.

If you don't mind, I think I have to make a few edits to make the use of the sphincs+ implementation and pyspx optional before I can merge this. That'll entail a few edits after Thanksgiving (back on Monday). The thinking there is that pyspx is still pretty fresh and not heavily tested, and I don't think I should require folks using TUF to include it -- just allow them to use it optionally.

awwad · 2018-11-20T17:24:06Z

securesystemslib/keys.py

-  # Generate the keyid of the Ed25519 key.  'key_value' corresponds to the
-  # 'keyval' entry of the 'Ed25519KEY_SCHEMA' dictionary.  The private key
+  # Generate the keyid of the ECDSA key.  'key_value' corresponds to the
+  # 'keyval' entry of the 'ECDSAKEY_SCHEMA' dictionary.  The private key


^_^ Thanks for the typo fix.

joostrijneveld · 2018-11-20T23:29:36Z

@awwad Sounds great, thanks! We didn’t really give making the dependency optional much thought (other than checking for potentially unsuccessful imports), but that definitely sounds like a good idea.

I completely agree w.r.t the status of pyspx; I’ll make sure to add some more comprehensive tests.

alyptik

I think a lot of the lines broken due to length could be refactored into multiple statements instead. A lot of the multiline string literals could be made a bit neater by using \n escapes instead of literal new lines.

alyptik · 2018-11-27T18:21:37Z

securesystemslib/formats.py

@@ -227,7 +235,7 @@
 # Supported TUF key types.
 KEYTYPE_SCHEMA = SCHEMA.OneOf(
  [SCHEMA.String('rsa'), SCHEMA.String('ed25519'),
-   SCHEMA.String('ecdsa-sha2-nistp256')])
+   SCHEMA.String('ecdsa-sha2-nistp256'),SCHEMA.String('spx')])


Shouldn't there be a space after the "," to be consistent with the previous line?

You're right; fixed!

alyptik · 2018-11-27T18:25:39Z

securesystemslib/interface.py

+    # worry about leaking sensitive information about the key's location.
+    # However, care should be taken when including the full path in exceptions
+    # and log files.
+    password = get_password('Enter a password for the SPX'


Multi-line strings like this are kind of confusing to mentally parse when combined with string concatenation. Is there maybe a more compact way to write these?

This addition is an artifact of code duplication, which seems out of scope for this pull request.

alyptik · 2018-11-27T18:28:25Z

securesystemslib/interface.py

+    # to a programmer who can call the function with or without a 'password'.
+    # Hence, we treat an empty password here, as if no 'password' was passed.
+    password = get_password('Enter a password for an encrypted RSA'
+        ' file \'' + Fore.RED + filepath + Fore.RESET + '\': ',


What's the reason you chose single quotes for these literals instead of using double quotes such as in " file '" + ... vs. 'file \''?

This addition is an artifact of code duplication, which seems out of scope for this pull request.

alyptik · 2018-11-27T18:30:10Z

securesystemslib/keys.py

+    # If the JSON could not be decoded, it is very likely, but not necessarily,
+    # due to a non-empty password.
+    except securesystemslib.exceptions.Error:
+      raise securesystemslib.exceptions\


How come you broke the line before the "." instead of after like in previous statements?

This addition is an artifact of code duplication, which seems out of scope for this pull request.

alyptik · 2018-11-27T18:32:26Z

securesystemslib/spx_keys.py

+
+  signature = None
+
+  # An if-clause is not strictly needed here, since 'spx' is the only


Since we are bothering to handle the error with an else: clause, wouldn't you agree that the if check actually does matter? Maybe the comment could be reworded to reflect this if so.

This addition is an artifact of code duplication, which seems out of scope for this pull request.

JustinCappos · 2019-06-13T15:48:11Z

Looks like the remaining issues are very minor code changes. @lukpueh would you kindly take a look? If it's very minor edits, please just make them and we can merge...

lukpueh · 2019-08-14T13:28:12Z

Continued in #169. Closing here.

cryptojedi and others added 11 commits October 31, 2018 08:11

Added SPX test cases (copy-pasted from ed25510 test cases)

8f7307f

Added support for SPX signatures, still completely untested

79b4cd7

Changed from PyNaCl API to simpler (byte-string based) pyspx API

c43c5b2

Added required spx file

94a6e29

Fix import of specific SPX instance

a8f0ea7

Remove nacl-specific Exception for PySPX

c81e51f

Add PySPX as a requirement

5eb5f43

Fix tests for SPHINCS+

6e9cab1

Fix missing exception handling for invalid schemes

0c8010d

Only the passing case was copied from ed25519, and inserted between the passing and failure case. This effectively removed error handling for incorrect ed25519 schemes.

Clarify silenced Exception, mark for code coverage

6212fa9

Fixed merge conflict

33d70bb

awwad commented Nov 20, 2018

View reviewed changes

awwad changed the title ~~WIP: sphincs support, for post-quantum crypto~~ WIP: sphincs+ support, for post-quantum crypto Nov 20, 2018

alyptik suggested changes Nov 27, 2018

View reviewed changes

Fix small copypasting errors

e713a24

JustinCappos requested a review from lukpueh June 13, 2019 15:42

lukpueh mentioned this pull request Jun 17, 2019

sphincs+ support, for post-quantum crypto #169

Closed

3 tasks

lukpueh closed this Aug 14, 2019

rugo mentioned this pull request Sep 15, 2022

Add Sphincs+ To Signer Interface #427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: sphincs+ support, for post-quantum crypto #160

WIP: sphincs+ support, for post-quantum crypto #160

awwad commented Nov 20, 2018 •

edited

Loading

awwad commented Nov 20, 2018

awwad Nov 20, 2018 •

edited

Loading

awwad Nov 20, 2018 •

edited

Loading

SantiagoTorres commented Nov 20, 2018 via email

awwad commented Nov 20, 2018

awwad commented Nov 20, 2018

awwad Nov 20, 2018

joostrijneveld commented Nov 20, 2018

alyptik left a comment

alyptik Nov 27, 2018

joostrijneveld Nov 29, 2018

alyptik Nov 27, 2018

joostrijneveld Nov 29, 2018

alyptik Nov 27, 2018

joostrijneveld Nov 29, 2018

alyptik Nov 27, 2018

joostrijneveld Nov 29, 2018

alyptik Nov 27, 2018

joostrijneveld Nov 29, 2018

JustinCappos commented Jun 13, 2019

lukpueh commented Aug 14, 2019

		@@ -900,6 +900,260 @@ def import_ecdsa_privatekey_from_file(filepath, password=None):
		return key_object


		def generate_and_write_spx_keypair(filepath=None, password=None):


		signature = None

		# An if-clause is not strictly needed here, since 'spx' is the only

WIP: sphincs+ support, for post-quantum crypto #160

WIP: sphincs+ support, for post-quantum crypto #160

Conversation

awwad commented Nov 20, 2018 • edited Loading

awwad commented Nov 20, 2018

awwad Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

awwad Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

SantiagoTorres commented Nov 20, 2018 via email

awwad commented Nov 20, 2018

awwad commented Nov 20, 2018

Choose a reason for hiding this comment

joostrijneveld commented Nov 20, 2018

alyptik 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

JustinCappos commented Jun 13, 2019

lukpueh commented Aug 14, 2019

awwad commented Nov 20, 2018 •

edited

Loading

awwad Nov 20, 2018 •

edited

Loading

awwad Nov 20, 2018 •

edited

Loading