signer: refactor SSlibSigner #590

lukpueh · 2023-05-30T15:31:53Z

edit: ticketized and removed TODOs below (#593, #594, #592)

This patch refactors the SSlibSigner signing implementation, akin to #585, which refactored signature verification (see PR for details about legacy code and rationale).

Unlike, #585 this patch does not only condense and move the code fo singing, but creates a new hierarchy of signers to achieve tw additional goals:

Provide tiny rsa, ecdsa and ed25519 pyca/cryptography Signer implementations, which are independent of private key serialization formats, above all, of the proprietary legacy keydict format. This is particularly interesting, when refactoring existing or designing new key generation or import interfaces, where it would be annoying to move back and forth over the legacy keydict.
Preserve SSlibSigner including its internal legacy keydict data structure. SSlibSigner is and remains a backwards-compatibility crutch. Breaking its existing users to make it a little less awkward would defeat that purpose. And even though the Signer API doc says that the internal data structure is not part of the public API, users may rely on it (python-tuf actually does so at least in tests and demos).

To achieve these goals, SSlibSigner becomes a container for the newly added CryptoSigner class, whose implementations can also be used as independent Signers, and above all created or imported, with very few lines of pyca/cryptography code.

Caveat:
Latest python-tuf tests pass against this patch, except for one, which expects a keydict deserialization failure in sign, which now happens in __init__ initialization time. This seems feasible to fix in python-tuf.

Also note that private key format errors are now ValueErrors and no longer unreliably either FormatErrors or sometimes UnsupportedAlgorithmErrors.

See full diff including the removal of related legacy code for how much more lightweight the code becomes: 194 additions and 1,264 deletions(!)

This patch refactors the SSlibSigner signing implementation, akin to secure-systems-lab#585, which refactored signature verification (see PR for details about legacy code and rationale). Unlike, secure-systems-lab#585 this patch does not only condense and move the code for singing, but creates a new hierarchy of signers to achieve two additional goals: 1. Provide tiny rsa, ecdsa and ed25519 pyca/cryptography Signer implementations, which are independent of private key serialization formats, above all, of the proprietary legacy keydict format. This is particularly interesting, when refactoring existing or designing new key generation or import interfaces, where it would be annoying to move back and forth over the legacy keydict. 2. Preserve SSlibSigner including its internal legacy keydict data structure. SSlibSigner is and remains a backwards-compatibility crutch. Breaking its existing users to make it a little less awkward would defeat that purpose. And even though the Signer API doc says that the internal data structure is not part of the public API, users may rely on it (python-tuf actually does so at least in tests and demos). To achieve these goals, SSlibSigner becomes a container for the newly added CryptoSigner class, whose implementations can also be used as independent Signers, and above all created or imported, with very few lines of pyca/cryptography code. **Caveat:** Latest python-tuf tests pass against this patch, except for one, which expects a keydict deserialization failure in `sign`, which now happens in `__init__` initialization time. This seems feasible to fix in python-tuf. Also note that private key format errors are now ValueErrors and no longer unreliably either FormatErrors or sometimes UnsupportedAlgorithmErrors. **Future work (will ticketize):** - Signing schemes strings should not be hardcoded all over the place but defined once in constants for all of securesystemslib. - There is some duplicate code for scheme string dissection and algorithm selection, which could be unified for all signers and public keys. - (bonus) secure-systems-lab#585 considered creating separate RSAKey, ECDSAKey, ED25519Key classes, but ended up putting everything into SSlibKey. Now that we have separate signers for each of these key types, which have a field for the corresponding public key object, it might make sense to reconsider this separation. This would give us a more robust data model, where e.g. allowed signing schemes are only validated once in the public key constructor and are thus validated implicitly in the signer constructor. Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh · 2023-06-02T11:07:45Z

Just went through all legacy sign/verify tests, of which ~98% assert FormatError for bad function arguments. Most of them are obsolete or different now, given the class-based design and use of mypy. Still, we could probably better test error scenarios depending on our decisions in #468 and #595 (I'll make a note in those issues).

Apart from that, basic signature verification testing is already fully covered by new signer API tests, using pre-generated signatures (see #585 (comment)).

Similar sign -> verify tests already exist for all key types, but not all schemes. I can change the latter, and maybe port the "auto rsa pss salt length"-test from #436.

Also moves error tests to separate function. Signed-off-by: Lukas Puehringer <[email protected]>

This allows us to add new test cases to the same table. Note: used the following script to patch the tests ``` from cryptography.hazmat.primitives.asymmetric.ec import ECDSA from cryptography.hazmat.primitives.hashes import SHA384 from securesystemslib.interface import ( import_ecdsa_privatekey_from_file, import_ed25519_privatekey_from_file, import_rsa_privatekey_from_file, ) from securesystemslib.signer import SSlibSigner rsa_priv = import_rsa_privatekey_from_file("tests/data/keystore/rsa_key", "password") ed25519_priv = import_ed25519_privatekey_from_file("tests/data/keystore/ed25519_key", "password") ecdsa_priv = import_ecdsa_privatekey_from_file("tests/data/keystore/ecdsa_key", "password") ed25519_keyid = ed25519_priv["keyid"] ed25519_pub = ed25519_priv["keyval"]["public"] rsa_keyid = rsa_priv["keyid"] rsa_pub = repr(rsa_priv["keyval"]["public"]).strip("'") ecdsa_keyid = ecdsa_priv["keyid"] ecdsa_pub = repr(ecdsa_priv["keyval"]["public"]).strip("'") def signature(key, scheme): key["scheme"] = scheme signer = SSlibSigner(key) # Hack requires secure-systems-lab#590 and disabling supported-scheme-check in ECDSASigner if scheme == "ecdsa-sha2-nistp384": signer._crypto_signer._signature_algorithm = ECDSA(SHA384()) signature = signer.sign(b"DATA") return signature.signature print( f""" ed25519_keyid = ( "{ed25519_keyid}" ) ed25519_pub = ( "{ed25519_pub}" ) rsa_keyid = ( "{rsa_keyid}" ) rsa_pub = "{rsa_pub}" ecdsa_keyid = ( "{ecdsa_keyid}" ) ecdsa_pub = "{ecdsa_pub}" key_sig_data = [ ( ed25519_keyid, "ed25519", "ed25519", ed25519_pub, "{signature(ed25519_priv, "ed25519")}", ), ( rsa_keyid, "rsa", "rsassa-pss-sha224", rsa_pub, "{signature(rsa_priv, "rsassa-pss-sha224")}", ), ( rsa_keyid, "rsa", "rsassa-pss-sha256", rsa_pub, "{signature(rsa_priv, "rsassa-pss-sha256")}", ), ( rsa_keyid, "rsa", "rsassa-pss-sha384", rsa_pub, "{signature(rsa_priv, "rsassa-pss-sha384")}", ), ( rsa_keyid, "rsa", "rsassa-pss-sha512", rsa_pub, "{signature(rsa_priv, "rsassa-pss-sha512")}", ), ( rsa_keyid, "rsa", "rsa-pkcs1v15-sha224", rsa_pub, "{signature(rsa_priv, "rsa-pkcs1v15-sha224")}", ), ( rsa_keyid, "rsa", "rsa-pkcs1v15-sha256", rsa_pub, "{signature(rsa_priv, "rsa-pkcs1v15-sha256")}", ), ( rsa_keyid, "rsa", "rsa-pkcs1v15-sha384", rsa_pub, "{signature(rsa_priv, "rsa-pkcs1v15-sha384")}", ), ( rsa_keyid, "rsa", "rsa-pkcs1v15-sha512", rsa_pub, "{signature(rsa_priv, "rsa-pkcs1v15-sha512")}", ), ( ecdsa_keyid, "ecdsa", "ecdsa-sha2-nistp256", ecdsa_pub, "{signature(ecdsa_priv, "ecdsa-sha2-nistp256")}", ), ( ecdsa_keyid, "ecdsa", "ecdsa-sha2-nistp384", ecdsa_pub, "{signature(ecdsa_priv, "ecdsa-sha2-nistp384")}", ), ( ecdsa_keyid, "ecdsa-sha2-nistp256", "ecdsa-sha2-nistp256", ecdsa_pub, "{signature(ecdsa_priv, "ecdsa-sha2-nistp256")}", ), ( ecdsa_keyid, "ecdsa-sha2-nistp384", "ecdsa-sha2-nistp384", ecdsa_pub, "{signature(ecdsa_priv, "ecdsa-sha2-nistp384")}", ), ] """ ) ``` Signed-off-by: Lukas Puehringer <[email protected]>

Since secure-systems-lab#585 SSlibKey no longer uses securesystemslib.keys to verify signatures, and thus no longer is tested via `test_*keys`. Good test coverage of the new SSlibKey implementation is already available in test_signer. This PR ports one missing test from: `test_rsa_keys.TestRSA_keys.test_verify_rsa_pss_different_salt_lengths` Used script to create test table entry (requires secure-systems-lab#590): ``` from cryptography.hazmat.primitives.asymmetric.padding import MGF1, PSS from cryptography.hazmat.primitives.hashes import SHA256 from securesystemslib.interface import import_rsa_privatekey_from_file from securesystemslib.signer import SSlibSigner scheme = "rsassa-pss-sha256" rsa_priv = import_rsa_privatekey_from_file( "tests/data/keystore/rsa_key", password="password", scheme=scheme ) signer = SSlibSigner(rsa_priv) signer._crypto_signer._padding = PSS( mgf=MGF1(SHA256()), salt_length=PSS.MAX_LENGTH ) signature = signer.sign(b"DATA") print( f""" # Test sig with max salt length (briefly available in v0.24.0) ( rsa_keyid, "rsa", "{scheme}", rsa_pub, "{signature.signature}", ), """ ) ``` Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh · 2023-06-05T10:51:03Z

Similar sign -> verify tests already exist for all key types, but not all schemes. I can change the latter, and maybe port the "auto rsa pss salt length"-test from #436.

Added sign-verify tests for all schemas here and rsa pss salt length test in #598. I think this PR is good to go.

lukpueh · 2023-06-16T14:07:32Z

securesystemslib/signer/_signer.py

@@ -225,5 +261,146 @@ def sign(self, payload: bytes) -> Signature:
                securesystemslib.exceptions.UnsupportedAlgorithmError:
                Signing errors.
        """


TODO: Update exceptions in docstring

Add optinal signer arg to runlib's run and record methods. NOTE: needs not yet public CryptoSigner from securesy secure-systems-lab/securesystemslib#590 TODO: - Add deprecation warnings to helper (<key> argument is deprecated and will be removed in 3.0.0 - Add tests, also with non-default signers - Support verifier (Key) argument in in_toto_record_stop if passed: use verifier.keyid to load preliminary link metadata pass verifier.to_dict() to Metadata.verify_signature elif signer is a CryptoSigner: use signer.public_key.keyid to load ... pass signer.public_key.to_dict to ... elif signer is a GPGSigner do gpg tricks (glob and grab signature keyid) to load file and export key from keychain to get verifier Future work (follup-up) - Add something like Metadata.verify(public_key: Key), if necessary - add deprecation warning to Metadata.verify_signature( - update record_stop and other places where verify_signature is used Signed-off-by: Lukas Puehringer <[email protected]>

IMPORTANT: Requires unlreleased securesystemslib code from secure-systems-lab/securesystemslib#590 Adds optional signer (Signer) arg to runlib's run/record functions, as alternative way of signing resulting link metadata, instead of using signing_key, gpg_keyid, or use_default_gpg. Closes in-toto#532 **Addtional notes:** In in_toto_record_stop, the public_key (Key) field of the signer is used to verify the preliminary link metadata file. The field is not yet part of the interface, although all signer implementations in secureystemslib have it: secure-systems-lab/securesystemslib#605 The patch aims to be minimally invasive, and thus barely refactors any of the existing signing argument handling in the relevant functions. Although, it was tempting to simplify the code, it turned out harder than thought, and therefor not worth the effort, given that these arguments are bound to deprecation. This is patch is part of a series of patches to prepare for the planned removal of securesystemslib legacy interfaces, see secure-systems-lab/securesystemslib#604 for details. **TODO (follow-up)** - add deprecation warning for legacy key args and legacy gpg formats (only show on API use, not on CLI use!) - add Metadata.verify(public_key: Key), if necessary (maybe we can just do this in runlib/verifylib), and use in in_toto_record_stop instead of passing `signer.public_key.to_dict()` to `Metadata.verify_signature(...)` - prepare for ssslib legacy interface removal in other parts of in_toto: - in-toto-run, in-toto-record (in-toto#533) - verifylib / in-toto-verify - in-toto-sign - Metadata API / docs - ... Signed-off-by: Lukas Puehringer <[email protected]>

jku

I think the core idea makes sense: separate two responsibilities:

loading the private keys from storage (handled by SSLibSigner at the moment, could change in future)
actual cryptography API usage

Code looks good. Two questions:

is it not confusing that CryptoSigner claims to be a Signer? (more in a code comment)
should we maybe move SSlibSigner out of _signer.py? The interface definition might be easier to understand without a screenful of crypto imports

jku · 2023-07-05T07:35:17Z

securesystemslib/signer/_signer.py

+        return self._crypto_signer.sign(payload)
+
+
+class CryptoSigner(Signer, metaclass=ABCMeta):


What's the advantage of deriving here? You say that the individual CryptoSigners could be used as Signers directly but if they don't implement from_priv_key_uri() I'm not sure I agree. The class hierarchy then has a lot of "Signers" but only some can be used as documented: others will throw a RuntimeError

I think the division of responsibility makes sense but could this just be a clearly internal class that isn't a Signer?

jku · 2023-07-05T07:56:47Z

securesystemslib/signer/_signer.py

@@ -3,15 +3,49 @@
 import logging
 import os
 from abc import ABCMeta, abstractmethod
-from typing import Any, Callable, Dict, Optional, Type
+from typing import Any, Callable, Dict, Optional, Type, cast
 from urllib import parse

 import securesystemslib.keys as sslib_keys


As a note (unrealted to this PR), I was wondering what was still keeping this import in:
We call decrypt_key() for the encrypted private key use case (there's also a matching securesystemslib.keys.encrypt_key() call).

I'm sure cryptography has something we could use instead?

I'm sure cryptography has something we could use instead?

Yes, it has, and #604 uses it consistently for all 3 supported key types to import encrypted private key files from a standard PEM format simply by...

securesystemslib/securesystemslib/signer/_signer.py

Line 305 in a1e280f

private_key = load_pem_private_key(private_pem, secret)

#604 does not yet provide API to serialise and dump corresponding private keys to disk but that would be extremely easy to to add using CryptoSigner.private_key.private_bytes as described here.

securesystemslib really shouldn't implement any more complex encryption or decryption logic than this.

lukpueh · 2023-07-17T13:43:27Z

Code looks good. Two questions:

is it not confusing that CryptoSigner claims to be a Signer? (more in a code comment)

Agreed, when looking at this PR alone CryptoSigner looks like an implementation detail of SSlibSigner.

However, I did intend for CryptoSigner to become a reasonable replacement for SSlibSigner. I just didn't want to open the can of worms of private key de/serialization just yet. This PR is only about CryptoSigner.sign. #604 adds, among other things, a CryptoSigner.from_priv_key_uri implementation.

should we maybe move SSlibSigner out of _signer.py? The interface definition might be easier to understand without a screenful of crypto imports

Yes, we should. I had a version of this PR, where I did move it out, but then reconsidered to keep the diff simpler and also due to some circular import hiccups, which surely can be fixed.

lukpueh · 2023-08-11T11:50:38Z

superseded by #604

lukpueh force-pushed the refactor-sslib-signer branch from 62d0c93 to ad64dcd Compare May 31, 2023 10:30

lukpueh changed the title ~~signer: refactor SSlibSigner (WIP)~~ signer: refactor SSlibSigner May 31, 2023

lukpueh marked this pull request as ready for review May 31, 2023 10:36

lukpueh requested a review from jku May 31, 2023 10:36

This was referenced May 31, 2023

key: consider adding RSAKey, ECDSAKey and ED25519Key classes #592

Closed

signer: deduplicate signing scheme dissection #594

Open

test: test SSlibSigner with all supported schemes

82b0b09

Also moves error tests to separate function. Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh force-pushed the refactor-sslib-signer branch from 87537ac to 82b0b09 Compare June 5, 2023 08:10

lukpueh commented Jun 16, 2023

View reviewed changes

lukpueh mentioned this pull request Jun 27, 2023

Add replacement API for '*keys' and 'interface' key generation and import #604

Merged

lukpueh mentioned this pull request Jul 4, 2023

runlib: add signer kwarg to run and record methods in-toto/in-toto#612

Merged

jku reviewed Jul 5, 2023

View reviewed changes

lukpueh mentioned this pull request Aug 11, 2023

signer: update raises section in SSlibSigner.sign #618

Closed

lukpueh closed this Aug 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signer: refactor SSlibSigner #590

signer: refactor SSlibSigner #590

lukpueh commented May 30, 2023 •

edited

Loading

lukpueh commented Jun 2, 2023 •

edited

Loading

lukpueh commented Jun 5, 2023 •

edited

Loading

lukpueh Jun 16, 2023

jku left a comment

jku Jul 5, 2023

jku Jul 5, 2023

lukpueh Jul 17, 2023 •

edited

Loading

lukpueh commented Jul 17, 2023

lukpueh commented Aug 11, 2023

		return self._crypto_signer.sign(payload)


		class CryptoSigner(Signer, metaclass=ABCMeta):

signer: refactor SSlibSigner #590

signer: refactor SSlibSigner #590

Conversation

lukpueh commented May 30, 2023 • edited Loading

lukpueh commented Jun 2, 2023 • edited Loading

lukpueh commented Jun 5, 2023 • edited Loading

lukpueh Jun 16, 2023

Choose a reason for hiding this comment

jku left a comment

Choose a reason for hiding this comment

jku Jul 5, 2023

Choose a reason for hiding this comment

jku Jul 5, 2023

Choose a reason for hiding this comment

lukpueh Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

lukpueh commented Jul 17, 2023

lukpueh commented Aug 11, 2023

lukpueh commented May 30, 2023 •

edited

Loading

lukpueh commented Jun 2, 2023 •

edited

Loading

lukpueh commented Jun 5, 2023 •

edited

Loading

lukpueh Jul 17, 2023 •

edited

Loading