key: refactor SSlibKey.verify_signature #585

lukpueh · 2023-05-24T11:02:06Z

EDIT 2023/05/20:

This is ready for review and can be merged as is.
Current python-tuf tests pass without changes.
Legacy code is not removed in this PR for the reasons outlined in 692b6ac. See full diff including the removal for how much more lightweight the code becomes: 13 additions and 1,010 deletions.(!)

Description of the changes being introduced by the pull request:

Signature verification for "securesystemslib keys" was previously implemented in 'rsa_keys', 'ecdsa_keys' and 'ed25519_keys' modules, which were called from SSlibKey.verify_signature via the legacy interface function keys.verify_signature().

This commit moves the entire implementation to SSlibKey, which will allow us (in a subsequent commit) to drastically decrease LOC count and drop 'nacl' optional dependency for ed25519 keys, in favour of 'pyca/cryptography', which we already use for all other sslib keys.

An alternative design for this refactor used separate RSAKey, ECDSAKey and ED25510Key classes to replace SSlibKey, but that mostly added redundant boilerplate code. To the user it shouldn't matter, so let's do what makes sense from maintainer perspective.

securesystemslib/signer/_key.py

Signature verification for "securesystemslib keys" was previously implemented in 'rsa_keys', 'ecdsa_keys' and 'ed25519_keys' modules, which were called from `SSlibKey.verify_signature` via the legacy interface function `keys.verify_signature()`. This commit moves the entire implementation to SSlibKey, which will allow us (in a subsequent commit) to drastically decrease LOC count and drop 'nacl' optional dependency for ed25519 keys, in favour of 'pyca/cryptography', which we already use for all other sslib keys. This works as is with current version of python-tuf! An alternative design for this refactor used separate RSAKey, ECDSAKey and ED25510Key classes to replace SSlibKey, but that mostly added redundant boilerplate code. To the user it shouldn't matter, so let's do what makes sense from maintainer perspective. Signed-off-by: Lukas Puehringer <[email protected]>

jku · 2023-05-30T15:12:04Z

wow. Haven't read the code yet but that looks really nice in comparison.

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]>

jku

I think this looks nice. Approving as is but commented one possible change.

I have a question about testing though. How well is signer.Key.verify_signature() getting tested? Obviously we're not adding new API here but... before this it wasn't really that critical that signer.Key.verify_signature() had good coverage as the implementation was known to be a wrapper over keys.verify_signature() with some testing of its own. Now the situation is a bit different.

So... how do you feel about the test coverage for the "new" implementation? Is it good enough?

securesystemslib/signer/_key.py

lukpueh · 2023-05-31T12:50:22Z

So... how do you feel about the test coverage for the "new" implementation? Is it good enough?

Oh, I forgot to mention that I have already added more tests for SSlibKey.verify_signature tests in preparation for this PR beforehand, also to show that the refactor really doesn't change the behaviour: see #555, #556.

So, I do feel quite confident.

@jku

Re-structure try/except block according to @jku's review comment, for better readability. Signed-off-by: Lukas Puehringer <[email protected]>

@jku

Further re-structure try/except block according to @jku's review comment, for better readability. Move "verify with pyca/crypto" and "verify with vendored ed25519" to separate helpers. Signed-off-by: Lukas Puehringer <[email protected]>

jku

Yes, feels much easier to understand.

The exceptions still feel a bit clunky (like handling generic Exception) but I think my suggestions are not very actionable... so LGTM

jku · 2023-06-01T10:44:37Z

securesystemslib/signer/_key.py

+        except UnverifiedSignatureError as e:
+            raise UnverifiedSignatureError(
+                f"Failed to verify signature by {self.keyid}"
+            ) from e
+
+        except Exception as e:
+            logger.info("Key %s failed to verify sig: %s", self.keyid, e)
+            raise VerificationError(
                f"Unknown failure to verify signature by {self.keyid}"
            ) from e


Handling generic Exception is not amazing but I can see how it's useful here

Yeah, you are right. The question is, what do we actually want to raise as VerificationError?

Should UnsupportedLibraryError be re-raised as VerificationError? Was e6529cd a mistake?

jku · 2023-06-01T10:50:35Z

securesystemslib/signer/_key.py

+            if signature.keyid != self.keyid:
+                raise ValueError(
+                    f"keyid mismatch: 'key id: {self.keyid}"
+                    f" != signature keyid: {signature.keyid}'"
                )
-        except (
-            exceptions.CryptoError,
-            exceptions.FormatError,
-            exceptions.UnsupportedAlgorithmError,
-            exceptions.UnsupportedLibraryError,
-        ) as e:
-            logger.info("Key %s failed to verify sig: %s", self.keyid, str(e))
-            raise exceptions.VerificationError(
+
+            signature_bytes = bytes.fromhex(signature.signature)
+


this could all be outside of the try block... raising things just to catch them in the same method seems odd

just raising a ValueError from this method might be fine: the idea is that it is a programming error to try to verify a signature that does not match the key --

if you don't want to raise ValueError from this method, you could just raise VerificationError before the try block

That said, doing this does have similarities to how the _verify* methods raise ValueError etc that are then handled in this method... so maybe it's fine

this could all be outside of the try block... raising things just to catch them in the same method seems odd

just raising a ValueError from this method might be fine: the idea is that it is a programming error to try to verify a signature that does not match the key --

I added this here to pass python-tuf tests. Previously, the keyid mismatch triggered a FormatError, which was caught and re-raised as VerificationError and asserted for in tests. Without the keyid check, this will try verification and fail as UnverifiedSignatureError.

Obviously, we can change the python-tuf test. Then we should maybe update the Key.verify_signature docs.

if you don't want to raise ValueError from this method, you could just raise VerificationError before the try block

Yes, I considered that too. But I thought it is semantically more correct to raise a ValueError as VerificationError, and that I could re-use the generic log.info + raise VerificationError block used for everything that is not an UnverifiedSignatureError. But now that you point this out, it does not seem fully correct to re-raise this ValueError as "Unknown failure"

That said, doing this does have similarities to how the _verify* methods raise ValueError etc that are then handled in this method... so maybe it's fine

Yes, it is consistent, which does not mean it is right. :) I could also raise a VerificationError in _verify*, which I don't catch here and just let fall through.

lukpueh · 2023-06-01T12:29:00Z

Btw. we use the except Exception: raise VerificationError from e-pattern in SpxKey and SigstoreKey as well. I suggest we merge here and I'll open a ticket for Key.verify_signature exception taxonomy specifically, okay?

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]>

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 commented May 24, 2023

View reviewed changes

securesystemslib/signer/_key.py Show resolved Hide resolved

lukpueh force-pushed the refactor-sslib-signature-verification branch from 92c3809 to cb09097 Compare May 24, 2023 13:36

lukpueh closed this May 25, 2023

lukpueh deleted the refactor-sslib-signature-verification branch May 25, 2023 10:41

lukpueh restored the refactor-sslib-signature-verification branch May 25, 2023 10:42

lukpueh reopened this May 26, 2023

lukpueh force-pushed the refactor-sslib-signature-verification branch from cb09097 to 3247ed7 Compare May 30, 2023 14:11

lukpueh marked this pull request as ready for review May 30, 2023 14:16

lukpueh requested a review from jku May 30, 2023 15:02

lukpueh mentioned this pull request May 31, 2023

signer: refactor SSlibSigner #590

Closed

jku approved these changes May 31, 2023

View reviewed changes

securesystemslib/signer/_key.py Outdated Show resolved Hide resolved

key: re-structure SlibKey.verify_signature

73f84e2

Re-structure try/except block according to @jku's review comment, for better readability. Signed-off-by: Lukas Puehringer <[email protected]>

This was referenced May 31, 2023

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

Closed

signer: deduplicate signing scheme dissection #594

Open

lukpueh force-pushed the refactor-sslib-signature-verification branch from 21afae6 to 8160f6b Compare June 1, 2023 08:45

key: re-structure SlibKey.verify_signature v2

6e7a71e

Further re-structure try/except block according to @jku's review comment, for better readability. Move "verify with pyca/crypto" and "verify with vendored ed25519" to separate helpers. Signed-off-by: Lukas Puehringer <[email protected]>

lukpueh force-pushed the refactor-sslib-signature-verification branch from 8160f6b to 6e7a71e Compare June 1, 2023 08:49

jku approved these changes Jun 1, 2023

View reviewed changes

lukpueh merged commit 48b59cb into secure-systems-lab:main Jun 1, 2023

lukpueh mentioned this pull request Jun 2, 2023

signer API: review exceptions specifically for Key.verify_signature #595

Open

lukpueh mentioned this pull request Jun 5, 2023

test: port legacy pss max salt compat test #598

Merged

lukpueh mentioned this pull request Jun 27, 2023

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

Merged

MVrachev mentioned this pull request Aug 30, 2023

Release v0.29.0 #631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

key: refactor SSlibKey.verify_signature #585

key: refactor SSlibKey.verify_signature #585

lukpueh commented May 24, 2023 •

edited

Loading

jku commented May 30, 2023

jku left a comment •

edited

Loading

lukpueh commented May 31, 2023

jku left a comment

jku Jun 1, 2023

lukpueh Jun 1, 2023

lukpueh Jun 1, 2023

jku Jun 1, 2023 •

edited

Loading

lukpueh Jun 1, 2023

lukpueh commented Jun 1, 2023

key: refactor SSlibKey.verify_signature #585

key: refactor SSlibKey.verify_signature #585

Conversation

lukpueh commented May 24, 2023 • edited Loading

Description of the changes being introduced by the pull request:

jku commented May 30, 2023

jku left a comment • edited Loading

Choose a reason for hiding this comment

lukpueh commented May 31, 2023

jku left a comment

Choose a reason for hiding this comment

jku Jun 1, 2023

Choose a reason for hiding this comment

lukpueh Jun 1, 2023

Choose a reason for hiding this comment

lukpueh Jun 1, 2023

Choose a reason for hiding this comment

jku Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

lukpueh Jun 1, 2023

Choose a reason for hiding this comment

lukpueh commented Jun 1, 2023

lukpueh commented May 24, 2023 •

edited

Loading

jku left a comment •

edited

Loading

jku Jun 1, 2023 •

edited

Loading