signer: Add abstract Key class, implement private key uri scheme for Signer by jku · Pull Request #456 · secure-systems-lab/securesystemslib

jku · 2022-11-04T10:02:31Z

Fixes: #447

Summary

move Key from python-tuf to here
Make Key abstract so we can have multiple implementations
Add a private key URI scheme so that Signer is actually usable without knowing the specific implementation
Split the signer API to internal submodules (this is a large code move but not a change to public API)

The private key URI scheme allows application code to handle private keys in a generic way: the keytype, used signer implementation or private key storage system are not something the application needs to worry about. Example

# new signer usage
signer = Signer.from_priv_key_uri("envvar:PRIV_KEY", pubkey)
sig = signer.sign(b"DATA")

# Key usage is unchanged (apart from verify_signature() now being agnostic about the payload)
key = Key.from_dict(keyid, dict)

Changelist

The user facing API additions are:
- Signer.from_priv_key_uri() added
- private key URI schemes are now part of the public API
- SIGNER_FOR_URI_SCHEME and KEY_FOR_TYPE_AND_SCHEME define supported signers and keys
- Signer implementations are now 95% implementation details of Signer: only the (currently unimplemented) new key generation/import is "public"
- Key no longer has verify_signature() for Metadata arguments because that is python-tuf specific: instead it works with bytes. This should not be an issue but requires a change in python-tuf Metadata-verify_delegate()
- Key also does not have from_securesystemslib_key(): it is now in SSlibKey
Other changes that do not really affect users
- Key moved from python-tuf to securesystemslib
- Key and Signer are now both abstract classes
- signer API split to multiple files -- this is not visible to users

Other notes:

In normal use, Signer and Key subclasses are never needed. Instead they both have a dynamic dispatch to find the correct implementation:
- Signer dispatch from Signer.from_priv_key_uri()
- Key dispatch from Key.from_dict()
Signers using priv key uris now "own" a public Key, or in other words: A Key can exist without a Signer but a Signer cannot exist without a Key. How this affects usage:
- Keys can just be deserialized as before
- Signers can be loaded with a private key URI and a public Key
- Signers can be generated from scratch or imported using Signer implementation specific API (unimplemented in this PR). The public key can then be requested from the Signer
SSlibKey and SSlibSigner implementations are currently the only ones, but now others can be added
Exceptions in the whole signer API could use a review, but for this PR please look at the Key.verify_signature() exceptions: they are the important ones

TODO later: I will file issues

GPG support is left out of the private key uri scheme. plan is:
- intoto can take GPGSigner and GPGKey. GPGSigner can be removed from sslib
- intoto can hook GPGSigner and GPGKey into the mechanisms added in this PR
- intoto takes care of the special keyid matching required by GPGKey
- later we can add a more spec compliant GPG signer and key into sslib
Key.get_pre_hash_algorithm() is not implemented: this would be easy to implement and useful for KMS and HSM at least
Signer generation and import are not yet implemented: this should be an optional feature in Signer implementations (not part of generic Signer API).
Signer class currently does expose public key: it probably should as the A) the priv key uri scheme pretty much requires it already and B) key import would become nicer with it
users are likely to only read Signer docs (as that's what they are using). This leads to two issues
- implementation specific exceptions -- how do we handle e.g. GCPSigner raising on network error?
- documenting the URIs: once we have multiple different Signer subclasses this documentation should be collected somewhere for the supported signers...

The scheme allows instantiating signers and signing with them without knowing the signer or privtae key type beforehand., Signers can now be used like this: # demo setup pubkey = generate_ecdsa_key() privkey = pubkey["keyval"].pop("private") os.environ["myprivatekey"] = privkey # sign: Correct Signer subclass is chosen based on the URI signer = Signer.from_priv_key_uri("envvar://myprivatekey", pubkey) sig = signer.sign(b"data") Just three schemes are supported by default but more can be added here and exernally, in applications. Default schemes: envvar: read private key content from environment variable file: read private key content from file encfile: read encrypted private key content from file, decrypt with passphrase from application

This is copied from python-tuf with following changes * verify_signature() is left in python-tuf as Metadata-related * is_verified() is added instead * type of keyval is changed from Dict[str, str] to Dict[str, Any] * Some docstrings made more reasonable for SSLib Signer.from_priv_key_uri() now takes a Key as argument. SSlibSigner constructor still takes a classic keydict as constructor argument and not a Key (in order to not break API) but that is now documented as an implementation detail.

python-tuf has an extensive test suite for Metadata in general: * I don't think python-tuf should stop testing Key just because it's now in securesystemslib * Maybe it's not necessary to copy e.g. all serialization tests here? Still, added a few basic tests for Key. Key is also used in the Signer tests.

securesystemslib/signer.py

jku · 2022-11-08T13:16:04Z

Results from chat with Lukas:

I will try to modify Key class in this PR to support multiple key implementations
goal is to be compatible with the in-toto/securesystemslib forks concept of Key abstraction
... with the difference that everything should be abstracted: verify_signature, serialization, keyid matching, payload hash algorithm lookup ( the only exception is construction from scratch which is quite implementation-specific)

This is quite a bit more than just moving the Key class... but it fits in well with Signer change as they are both dispatcher-style abstractions (for Key and Signer respectively)

The idea is to allow more Key implementations, but SSlibKey provides the current implementations.

GPGSigner and GPGKey are not in the default dispatch list as the serialization formats they produce are not compatible with TUF or in-toto specifications Tests are very smoke testy so far

verify_signature now has two exceptions: class UnverifiedSignatureError(Error): class VerificationError(UnverifiedSignatureError): First one for those who just want to know if verify succeeded. Second one for those who want to also know if process failed somehow.

jku · 2022-11-10T12:44:12Z

This ballooned "a bit"... I've updated the description, hoping it makes sense to others.

jku · 2022-11-10T12:48:39Z

Oh and I forgot to mention: Most of the GPGKey implementation in here comes from https://github.com/in-toto/securesystemslib/ so is likely @PradyumnaKrishna's work: if this PR looks like a path forward, should make sure he gets credited (I can try editing the commit history so authorship is visible in git)

securesystemslib/signer.py

lukpueh · 2022-11-11T13:13:56Z

A couple of high-level comments:

This is really cool! I'm extremely excited about the prospect of a clear and slim signing/verification interface in securesystemslib (and about the cleaning up what then becomes an implementation detail)
I think we can make the Key interface even more abstract (see inline comments above)
I don't like the half-baked GPGKey integration. It is not really a Key but still its subclass (see inline comment above)
I think we can fix the discussed gpg issues in securesystemslib so that it becomes more attractive for tuf and dsse to use it too, and implement everything that's needed for backwards-compatibility directly in in-toto, where it is needed.

Some thoughts about fixing gpg issues

GPGKey has keytype (instead of type) and scheme instead of (method) fields, so that it can be created with Key.from_dict.
GPGSigner creates Signature-compatible signatures (keyid, sig, other_headers), so that no case handling is required when deserializing signatures.
GPGSigner assigns the keyid of the attached outer public key to Signature and not the subkey, so that no sophisticated keyid matching is needed verification time.
DSSE uses the fixed GPGKey and GPGSigner as there is no need to support old formats in DSSE.
in-toto uses GPGSigner and GPGKey for creating new signatures and verifying them.
in-toto implements classes and methods to continue support for parsing and verifying old gpg keys and signatures.

jku · 2022-11-11T14:47:56Z

I think we can do both:

Add a compliant BetterGPGKey and BetterGPGSigner classes
keep the current gpgp implementation until intoto does not need it

but I certainly wouldn't oppose to in-toto keeping the GPGKey and GPGSigner in their code base -- and with this architecture it would be quite easy

As mentionedin the other reply, GPGKey does support Key.from_dict().

jku · 2022-11-11T14:57:52Z

I think the special keyid matching is not too much to ask from the Key.verify_signature() callers using GPGKey, so we can leave it to them: In practice in-toto just needs to embed the matching function in their sources and remember to use that

securesystemslib/signer.py

lukpueh · 2022-11-15T10:50:40Z

I think we can do both:

Add a compliant BetterGPGKey and BetterGPGSigner classes

👍 Let's do that in a follow-up PR.

keep the current gpgp implementation until intoto does not need it

I suggest to remove it from securesystemslib, for these reasons (as discussed above):

GPGKey is not really compatible with Key as it has different fields and dispatching to it with Key.from_dict requires a work around. This should not be public securesystemslib API.
GPGSigner was originally added to securesystemslib to allow gpg signing with python-tuf's new metadata API (#341), and recent efforts with GPGKey are related to the securesystemslib DSSE implementation (in-toto/securesystemslib). However, neither python-tuf nor DSSE should use the current implementations of GPG{Signer, Key, Signature}, but rather a BetterGPG{Signer, Key}, which is spec compliant wrt metadata format. And securesystemslib should not host features that are only relevant to one of its users.

lukpueh · 2022-11-15T10:55:22Z

I think the special keyid matching is not too much to ask from the Key.verify_signature() callers using GPGKey, so we can leave it to them: In practice in-toto just needs to embed the matching function in their sources and remember to use that

Yes, this should be easy if it's only needed in in-toto. The reason why we defined the match_keyid interface here was, that we needed it in in-toto's metadata API and also for DSSE, which was going to be implemented in securesystemslib. But if we decide that DSSE should only use a BetterGPG{Signer, Key} that does not have special keyid matching then we don't need to worry about this in securesystemslib.

Signer and Key are now abstract and can have multiple implementations that get dispatched by the base classes. The current GPG implementation is just non-compliant enough that it is not included in the dispatch. Plan is: * move GPGSigner to intoto, add GPGKey in intoto: this allows intoto to keep using this implementation without too much pain * Possibly implement a more compliant GPG signer and key in securesystemslib

jku · 2022-11-16T13:54:21Z

Current state:

GPGKey has been removed from the PR in the last commit, GPGSigner no longer claims to be compatible with the signer URI scheme
PR is ready for general "is this a good idea" sort of review but feel free to leave line-by-line level review for until polishing happens
the idea is that intoto can implement both GPGKey and GPGSigner in their code base and plug them in via the dispatch system (in a hacky way but still). I did not remove the original commits from this branch so the GPGKey implementation I tested with (which again was mostly copied from the intoto fork) is available if needed.

Potential review questions/improvements:

split code to multiple files? How should users import this API? How do we make it clear these modules are separate from the old modules?
key import is not implemented: I think it should be Signer implementation specific functionality (but I don't see a problem if a specific Key implementation wants to add a SomeKey.import(...))
Are the privkey URIs clear enough? I think being a URI allows required complexity (and library support)... I did not try to stay "compatible" with the sigstore scheme: I don't think that is valuable and if nothing enforces it, the compatibility will deteriorate anyway over time.
do we want to model base Key class on current TUF & intoto specs or just on what securesystemslib actually needs? I've tried to do former but this leads to Key having both a keytype and scheme which, as lukas has mentioned, may be unnecessary for the implementation... It just felt like being spec compatible by default would be good.
exceptions should maybe get more attention -- as an example I have no idea what from_priv_key_uri() can currently throw. This is not super critical as this happens during signing, based on user input: it's not nearly as fatal as failing on verify based on deserialized remote data

lukpueh · 2022-11-25T12:53:40Z

@lukpueh does this look familiar (just happened on CI, never seen before myself):

No it does not.

lukpueh · 2022-11-25T13:05:54Z

the big difference for python-tuf code was that Key.from_securesystemslib_key() no longer exists. This feels like a correct change to me but it is an API change. We could add a workaround for backwards compat but I think I'm fine with changing API here?

I think it's more than just from_securesystemslib_key. The python-tuf pr completely removes tuf.api.Key, right? It has almost an identical replacement in SSlibKey, but it is still a breaking change and warrants a new major release. It should only affect repositories, though. Client application code is very unlikely to directly use the Key class. So I'm fine with it.

jku · 2022-11-25T13:25:54Z

I think it's more than just from_securesystemslib_key. The python-tuf pr completely removes tuf.api.Key, right? It has almost an identical replacement in SSlibKey, but it is still a breaking change and warrants a new major release. It should only affect repositories, though. Client application code is very unlikely to directly use the Key class. So I'm fine with it.

No, Key is still part of the python-tuf Metadata API -- it's just that it comes from securesystemslib. This is not as clear as it should be since we are not explicit about API: I think it would make sense to use tuf/api/metadata/__init__.py for that purpose (or even tuf/api/__init__.py to make the import statements shorter) ...

So currently python-tuf users can do from tuf.api.metadata import Key and I think the object they get will quack like the duck they expect... Obviously there are small differences, from_securesystemslib_key() being the major one.

Now that I think about it the Key() constructor is probably now also useless or broken, it makes no sense... should have a look at that.

securesystemslib/signer/_key.py

jku · 2022-11-25T14:00:39Z

Related comment on Signer:

it has the same issue where signer = Signer() seems to work as but the resulting signer is useless because the abstractmethod is unimplemented...
since this is the case... I wonder if we should just rename Signer.from_priv_key_uri() to Signer() -- it's clear that the constructor isn't being used since it does nothing useful? Using Signer() would also make sense if we're trying to propose this as the way to create Signers.

the former is #463. The latter idea (just using Signer()) sounds really appealing but I think it would mean a harder API break as it would likely break constructors for SSLibSigner and GPGSigner and those are being used (the break comes from having to add a __new__() method to Signer and that kind of forces the signature for subclass constructors... ).

joshuagl

I really like this, nice work! I left a couple of minor comments, notably a request to document the dynamic dispatch method and how folks add their own implementations of the abstract base classes and wire them in.

securesystemslib/signer/_key.py

* Fix the incorrect metaclass syntax * Also refactor constructor so it is shorter

jku · 2022-11-29T10:07:04Z

fixed a few latest comments (thanks!): No real functional changes here.

securesystemslib/signer/_key.py

There is (at least currently) no promise that Signer implementations do not raise unexpected errors.

jku · 2022-11-29T15:15:04Z

Ok, I think I'm now done. All future tickets are filed, PR should be ready.

joshuagl

LGTM

lukpueh

This is so neat! Many thanks, Jussi. I'm approving, although we should probably check for None instead of Falseyness when creating the default self.unrecognized_fields value. But this is no blocker.

Also, there is an annotation inconsistency in Key and Signature unrecognized_fields constructor argument and instance variables (Dict vs. Mapping, and type hint at declaration in one but not the other). You probably just copied those over. 🤷

securesystemslib/signer/_signature.py

securesystemslib/signer/_key.py

* do not check falsyness of argument containers: the results are unexpected if the argument container is empty * Do not use Mapping annotation for unrecognized_fields. The original idea was that Metadata API is not changing them: this is true but we don't want to prevent users from modifying the fields...

jku mentioned this pull request Nov 4, 2022

Move (most of) Key to Securesystemslib theupdateframework/python-tuf#2165

Merged

jku force-pushed the privkeyuri branch from 9833f41 to bfeda3c Compare November 4, 2022 10:54

jku force-pushed the privkeyuri branch from 1a3684e to 61f1774 Compare November 4, 2022 20:19

jku commented Nov 4, 2022

View reviewed changes

securesystemslib/signer.py Outdated Show resolved Hide resolved

jku commented Nov 4, 2022

View reviewed changes

securesystemslib/signer.py Outdated Show resolved Hide resolved

jku commented Nov 8, 2022

View reviewed changes

securesystemslib/signer.py Outdated Show resolved Hide resolved

lukpueh reviewed Nov 8, 2022

View reviewed changes

securesystemslib/signer.py Outdated Show resolved Hide resolved

jku added 3 commits November 9, 2022 14:10

Key: Make Key an abtract class

4dd2f7b

The idea is to allow more Key implementations, but SSlibKey provides the current implementations.

Add GPGKey, use it in GPGSigner

707e0de

GPGSigner and GPGKey are not in the default dispatch list as the serialization formats they produce are not compatible with TUF or in-toto specifications Tests are very smoke testy so far

jku changed the title ~~signer: Add Key class, implement private key uri scheme~~ signer: Add abstract Key class, implement private key uri scheme for Signer Nov 10, 2022

lukpueh reviewed Nov 11, 2022

View reviewed changes

securesystemslib/signer.py Outdated Show resolved Hide resolved

securesystemslib/signer.py Outdated Show resolved Hide resolved

securesystemslib/signer.py Outdated Show resolved Hide resolved

jku commented Nov 11, 2022

View reviewed changes

securesystemslib/signer.py Outdated Show resolved Hide resolved

This was referenced Nov 11, 2022

Feature: Add support for KeyVault for AWS (AWSKMS) repository-service-tuf/repository-service-tuf#24

Open

Add support for Signature class instances #409

Closed

Ssh keys #451

Closed

lukpueh mentioned this pull request Nov 15, 2022

Add DSSE support for in-toto in-toto/in-toto#503

Merged

3 tasks

jku marked this pull request as ready for review November 25, 2022 12:31

jku commented Nov 25, 2022

View reviewed changes

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

jku mentioned this pull request Nov 25, 2022

Signer uses python2 "__metaclass__" #463

Closed

joshuagl reviewed Nov 28, 2022

View reviewed changes

securesystemslib/signer/_key.py Show resolved Hide resolved

securesystemslib/signer/_key.py Show resolved Hide resolved

securesystemslib/signer/_key.py Show resolved Hide resolved

jku commented Nov 28, 2022

View reviewed changes

securesystemslib/signer/_key.py Show resolved Hide resolved

jku added 3 commits November 29, 2022 11:35

signer API: Make Key actually abstract

549affd

* Fix the incorrect metaclass syntax * Also refactor constructor so it is shorter

signer API: Add doc example of using a custom signer

b69303e

tests: Add a test for custom Signer

acd99fd

lukpueh mentioned this pull request Nov 29, 2022

Tool for importing PEM formatted ecdsa keys in-toto/in-toto#522

Closed

jku commented Nov 29, 2022

View reviewed changes

securesystemslib/signer/_key.py Show resolved Hide resolved

jku added 2 commits November 29, 2022 15:22

tests: Add test for custom Key

c3c1ca8

signer: Add note about unhandled exceptions

98442f5

There is (at least currently) no promise that Signer implementations do not raise unexpected errors.

This was referenced Nov 29, 2022

Design key import/generation for the signer API #466

Closed

signer API: review exceptions #468

Open

Key should expose pre hash algorithm #469

Closed

joshuagl approved these changes Nov 29, 2022

View reviewed changes

joshuagl mentioned this pull request Nov 29, 2022

WIP: Fix implementation of abstract base classes (ABCs) to use Python3 syntax #465

Closed

3 tasks

jku mentioned this pull request Nov 30, 2022

Move GPGSigner to intoto #471

Closed

lukpueh approved these changes Nov 30, 2022

View reviewed changes

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

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

lukpueh merged commit 14ee264 into secure-systems-lab:master Nov 30, 2022

This was referenced Dec 16, 2022

Add GPGKey and use with GPGSigner #488

Merged

Document public API for tuf.api and tuf.ngclient theupdateframework/python-tuf#2234

Closed

lukpueh mentioned this pull request Jan 10, 2023

Exception handling on Key.verify for incompatible type of signature in-toto/securesystemslib#8

Closed

Conversation

jku commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelist

Other notes:

TODO later: I will file issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jku commented Nov 8, 2022

Uh oh!

jku commented Nov 10, 2022

Uh oh!

jku commented Nov 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lukpueh commented Nov 11, 2022

Uh oh!

jku commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku commented Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lukpueh commented Nov 15, 2022

Uh oh!

lukpueh commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukpueh commented Nov 25, 2022

Uh oh!

lukpueh commented Nov 25, 2022

Uh oh!

jku commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jku commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshuagl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jku commented Nov 29, 2022

Uh oh!

Uh oh!

jku commented Nov 29, 2022

Uh oh!

joshuagl left a comment

Choose a reason for hiding this comment

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jku commented Nov 4, 2022 •

edited

Loading

jku commented Nov 11, 2022 •

edited

Loading

jku commented Nov 11, 2022 •

edited

Loading

lukpueh commented Nov 15, 2022 •

edited

Loading

jku commented Nov 16, 2022 •

edited

Loading

jku commented Nov 25, 2022 •

edited

Loading

jku commented Nov 25, 2022 •

edited

Loading