Add Sphincs+ To Signer Interface #427

rugo · 2022-09-15T09:42:22Z

Fixes:
Updates: #169

Description of the changes being introduced by the pull request:

The PRs #160 and #169 tried to include the PQC scheme SPHINCS+, which is about to be NIST standardized.
However, work has stalled as people were busy with other stuff.

After talking to @lukpueh we decided to directly include SPHINCS+ into the "new" Signer API.

I welcome feedback and additional TODOs.

jku

I don't know anything about sphincs so just commenting on cosmetics: there's a couple of code comments that do not seem to match code. I think it would be best to not duplicate information like bytearray length in the comments if the code is clear as it seems to be here

jku · 2022-09-27T10:48:19Z

securesystemslib/formats.py

+# An SPHINCS raw public key, which must be 32 bytes.
+SPHINCSPRIVATE_SCHEMA = SCHEMA.LengthBytes(64)


comment seems incorrect

jku · 2022-09-27T10:49:42Z

securesystemslib/formats.py

+# An SPHINCS raw signature, which must be 64 bytes.
+SPHINCSSIGNATURE_SCHEMA = SCHEMA.LengthBytes(7_856)


comment byte length mismatch?

rugo · 2022-09-27T11:00:36Z

I don't know anything about sphincs so just commenting on cosmetics: there's a couple of code comments that do not seem to match code. I think it would be best to not duplicate information like bytearray length in the comments if the code is clear as it seems to be here

Makes sense. I removed them.

lukpueh

Thanks for the patch, @rugo! I think what's there is great. There are a few peculiarities that were likely inspired from the existing interfaces (e.g. passing args that are not used, keytype/scheme redundancy), but I think it's okay for the sake of consistency.

Would you mind adding tests to check_public_interfaces.TestPublicInterfaces.test_keys, to assert that we fail gracefully if PySPX is not installed?

It would also be good to have interface.generate_and_write_sphincs*, and interface.import_sphincs* functions as we have for other key types. But that can be added in a separate PR.

lukpueh · 2022-10-17T09:26:01Z

securesystemslib/formats.py

@@ -193,7 +193,7 @@
 # Supported securesystemslib key types.
 KEYTYPE_SCHEMA = SCHEMA.OneOf(
  [SCHEMA.String('rsa'), SCHEMA.String('ed25519'), SCHEMA.String('ecdsa'),
-   SCHEMA.RegularExpression(r'ecdsa-sha2-nistp(256|384)')])
+   SCHEMA.RegularExpression(r'ecdsa-sha2-nistp(256|384)'), SCHEMA.String('sphincs-shake-128s')])


Does it make sense to call the keytype just sphincs? (see discussion about keytype vs. scheme in #239 and #308)

lukpueh · 2022-10-17T09:47:10Z

securesystemslib/sphincs_keys.py

+    # be tested by running 'ed25519_keys.py' as a standalone module.
+    # python -B ed25519_keys.py
+    import doctest
+    doctest.testmod()


No need to add doctest boilerplate, there are no doctests in this module, and we wouldn't even run them in CI.

rugo · 2022-10-17T13:47:26Z

@lukpueh Thanks for the feedback. I added the required tests, changed the keytype and removed doctest.

Regadring interface.generate_and_write_sphincs: I would prefer to address this in a separate PR. This would need more coordination as SPHINCS+ does not have a standardized key OID yet (AFAIK). Decoding a SPHINCS+ key from PEM would therefore require us to create our own non-standard key identifier and format.

rugo · 2022-10-18T07:57:43Z

@lukpueh Could you check the failing test and give me some more insights? I don't really understand what is happening as the error says:

subprocess.TimeoutExpired: Command '['gpg', '--detach-sign', '--digest-algo', 'SHA256', '--local-user', 'E8AC80C924116DABB51D4B987CB07D6D2C199C7C', '--homedir', '/private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmpuwgkkmy9/rsa']' timed out after 3 seconds

Which doesn't seem related to what I did. Did the macos test case break or am I missing something?

lukpueh · 2022-10-18T08:00:38Z

This is an unrelated test failure (#428). We are working on a fix.

lukpueh

Thanks for addressing my comments! Just left a few more. May I also ask that you get rid of the merge commit in the PR? Otherwise, I think we can merge this and try using it with TUF and/or in-toto.

securesystemslib/keys.py

requirements.txt

tests/test_keys.py

lukpueh · 2022-10-21T12:39:51Z

@rugo: I just merged #439, which auto-formats the entire code base. The easiest thing might be to re-apply your changes manually on top of the updated master branch, run auto-formatters commands (w/o --check --diff options) over them, and push them in a single commit to this branch. I apologize for the inconvenience, and am happy to help resolve any conflicts.

rugo · 2022-10-24T17:54:42Z

Alright. I think I have addressed your points. Additional feedback welcome.

rugo · 2022-10-25T14:41:59Z

Since the pylint test case complained about some minor things I fixed them now.

…e but sphincs+ function is called.

…self explanatory

…f PySPX is not present.

…incs<.

…t installed

…t doesn't find a false positive for E1101.

rugo · 2022-10-27T10:41:36Z

I rebased again to avoid the requirements.txt merge conflict.

lukpueh

Great work! I think there's one mistake in a docstring (see inline comment). Otherwise, LGTM!

securesystemslib/keys.py

lukpueh

💯

jku reviewed Sep 27, 2022

View reviewed changes

lukpueh mentioned this pull request Oct 17, 2022

sphincs+ support, for post-quantum crypto #169

Closed

3 tasks

lukpueh reviewed Oct 17, 2022

View reviewed changes

lukpueh requested changes Oct 18, 2022

View reviewed changes

securesystemslib/keys.py Outdated Show resolved Hide resolved

requirements.txt Show resolved Hide resolved

tests/test_keys.py Outdated Show resolved Hide resolved

rugo force-pushed the add-sphincs-signer branch from 8562419 to 58a3f97 Compare October 24, 2022 15:56

Ruben Gonzalez and others added 14 commits October 27, 2022 12:40

Add experimental sphincs-shake-128s signer.

db4f69d

Check if pyspx is available. Raise exception if pyspx is not availabl…

d879759

…e but sphincs+ function is called.

Add Google style docstrings to spx file.

0124a29

Add pyspx to pinned requirements.

657a3ea

Remove comments over SPHINCS schema definitions as schemas should be …

7ca68fb

…self explanatory

Add sphincs+ to test_keys.py. Make sure securesystemslib works even i…

4dec395

…f PySPX is not present.

Remove doctest from sphincs_keys module.

075e4c7

Change keytype name of sphincs signature schemes to more general >sph…

c664434

…incs<.

Align Sphincs code to new coding style

658626b

Add PySPX to extras in setup.py

600ab2f

Add docstring to sphincs keygen function

9cfbc64

Add sphincs+ tests to check if code fails gracefully when PySPX is no…

0b52571

…t installed

Mark methods within sphincs module as dynamically generated, so pylin…

b62e8e2

…t doesn't find a false positive for E1101.

Renamed variables and added docstring to sphincs+ to make pylint happy.

b802315

rugo force-pushed the add-sphincs-signer branch from 3c8fe84 to b802315 Compare October 27, 2022 10:40

lukpueh reviewed Nov 2, 2022

View reviewed changes

securesystemslib/keys.py Outdated Show resolved Hide resolved

Fix copy pasta mistake in Sphincs docstring

9661565

lukpueh approved these changes Nov 3, 2022

View reviewed changes

lukpueh merged commit 4e63f99 into secure-systems-lab:master Nov 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Sphincs+ To Signer Interface #427

Add Sphincs+ To Signer Interface #427

rugo commented Sep 15, 2022

jku left a comment

jku Sep 27, 2022

jku Sep 27, 2022

rugo commented Sep 27, 2022

lukpueh left a comment

lukpueh Oct 17, 2022

lukpueh Oct 17, 2022

rugo commented Oct 17, 2022

rugo commented Oct 18, 2022

lukpueh commented Oct 18, 2022

lukpueh left a comment

lukpueh commented Oct 21, 2022

rugo commented Oct 24, 2022

rugo commented Oct 25, 2022

rugo commented Oct 27, 2022

lukpueh left a comment

lukpueh left a comment

		# An SPHINCS raw public key, which must be 32 bytes.
		SPHINCSPRIVATE_SCHEMA = SCHEMA.LengthBytes(64)

		# An SPHINCS raw signature, which must be 64 bytes.
		SPHINCSSIGNATURE_SCHEMA = SCHEMA.LengthBytes(7_856)

Add Sphincs+ To Signer Interface #427

Add Sphincs+ To Signer Interface #427

Conversation

rugo commented Sep 15, 2022

Description of the changes being introduced by the pull request:

jku left a comment

Choose a reason for hiding this comment

jku Sep 27, 2022

Choose a reason for hiding this comment

jku Sep 27, 2022

Choose a reason for hiding this comment

rugo commented Sep 27, 2022

lukpueh left a comment

Choose a reason for hiding this comment

lukpueh Oct 17, 2022

Choose a reason for hiding this comment

lukpueh Oct 17, 2022

Choose a reason for hiding this comment

rugo commented Oct 17, 2022

rugo commented Oct 18, 2022

lukpueh commented Oct 18, 2022

lukpueh left a comment

Choose a reason for hiding this comment

lukpueh commented Oct 21, 2022

rugo commented Oct 24, 2022

rugo commented Oct 25, 2022

rugo commented Oct 27, 2022

lukpueh left a comment

Choose a reason for hiding this comment

lukpueh left a comment

Choose a reason for hiding this comment