sphincs+ support, for post-quantum crypto #169
Conversation
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.
Import third-party pyspx package in securesystemslib.spx_keys only and require whoever imports securesystemslib.spx_keys to handle ImportError (or IOError in case of missing C backend). See securesystemslib.keys for exemplary import handling. This commit also moves three spx-specific schema definitions from securesystemslib.formats to securesystemslib.spx_keys, so that the former does not have to deal with a conditional import of an optional library.
Add pyspx to `extra_require` in setup.py, to optionally install e.g. with `pip install securesystemslib[pyspx]`.
To create a signature the public key is not required.
- Remove trailing whitespace
- Remove redundant try/except blocks in functions, where
- the exception is already handled by earlier function code
(schema check)
- the exception should be handled on module level (ImportError
for optional PySPX)
- De-clutter (e.g. remove some obvious code comments, superfluous
variable null value instantiations, redundant checks)
- Fix typos
- Enhance line wrapping
In 0baedd3 we changed keys.create_signature to take the canonicalized and utf-8 encoded data as argument. This adopts the change in the spx related create_signature dispatch block.
lukpueh
force-pushed
the
add-pyspx-support
branch
from
8cf37ec to
b0735d5
Compare
| # Generate a new SPX key object. | ||
| spx_key = securesystemslib.keys.generate_spx_key() | ||
|
|
||
| if not filepath: |
There was a problem hiding this comment.
Should there be a check to see if a provided filepath is a directory?
And it could either be a warning or it could accept the directory and change the path to directory/keyid.
There was a problem hiding this comment.
Good point. I created an issue for this. Feel free to comment there. #172
|
|
||
| # Begin building the SPX key dictionary. | ||
| spx_key = {} | ||
| keytype = 'spx' |
There was a problem hiding this comment.
Do we need keytype and scheme separately? I see that SPX_SIG_SCHEMA is set to OneOf but only spx is provided. Is this just leaving it open for for future enhancement?
There was a problem hiding this comment.
Yes we do. Currently we only support one spx scheme, i.e. SPHINCS+-SHAKE256, but it's conceivable to later add others (see sphincs.org).
I probably would have opted for a more descriptive scheme name, but that was the choice of the original author of this PR, and I think it is okay.
securesystemslib/spx_keys.py
Outdated
| True | ||
| >>> scheme == 'spx' | ||
| True | ||
| >>> signature, scheme = \ |
There was a problem hiding this comment.
Good eyes. Seems like a copy-pasta error from ed25519_keys.py, which apparently served as base for spx_keys.py. Fixed in f6e47b9.
Adopt changes from a67a6b3 in doctest.
| raise securesystemslib.exceptions.CryptoError('An "spx" signature' | ||
| ' could not be created with pyspx: ' + str(e)) | ||
|
|
||
| return signature, scheme |
There was a problem hiding this comment.
This function takes scheme as a parameter and also returns it, is there a reason for this?
There was a problem hiding this comment.
Well spotted indeed. ;) I briefly considered removing it but left it for consistency with the rest of the create_signature functions, and to keep the diff small.
Besides, it kinda does make sense if scheme becomes an optional argument with a default, as is the case in ecdsa_keys.create_signature. So that, if the caller does not pass a scheme, s/he learns about the default by looking at the return value.
OTOH and in contrast to being consistent and keeping the diff small, I did remove the public_key argument from spx_keys.create_signature, because it makes no sense (see
a67a6b3).
If you're not opposed, I'd just leave it.
There was a problem hiding this comment.
That makes sense, it seems fine as is.
|
Thanks for picking this up! I'm afraid I cannot really contribute or comment until late September, but I wanted to point out that pyspx is still at the round 1 parameter sets (and code), and we've had a few minor changes to SPHINCS+ since then. From the outside it's all trivialities, but it would be annoying to roll out something and then have to make small tweaks shortly after. Perhaps @cryptojedi can comment / help out with that if necessary. In general I feel like the naming of the parameter set needs to be improved to stress explicitly that we're dealing with |
|
Hi @joostrijneveld, thanks for chiming back in!
Do I understand correctly that you advice to hold back merging until new releases of pyspx/SPHINCS+?
Are you referring to the scheme discussion we had above? |
|
In Issue #179 we're working to ensure that any functionality with a native dependency presents meaningful user feedback when the native dependency is not available. It would be great if this PR could implement functionality similar to #200 to ensure the |
|
Superseded by #427 |
Fixes issue #:
Updates #160
Description of the changes being introduced by the pull request:
This is a review + friendly take-over, as requested by @JustinCappos in #160 (comment).
The PR does some clean-up and minor refactoring. See lukpueh/securesystemslib@d653691...lukpueh:add-pyspx-support for the changes on top of #160, and see #160 for prior reviews.
Thanks @cryptojedi and @joostrijneveld for your contribution. Please let me know what you think of my changes.
Please verify and check that the pull request fulfills the following
requirements: