signer: Add abstract Key class, implement private key uri scheme for Signer

[jku]

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

[jku]

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)

[jku]

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

[jku]

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)

[lukpueh]

A couple of high-level comments:

1. 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)
2. I think we can make the Key interface even more abstract (see inline comments above)
3. I don't like the half-baked GPGKey integration. It is not really a Key but still its subclass (see inline comment above)
4. 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]

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]

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

[lukpueh]

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]

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.

[jku]

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]

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

No it does not.

[lukpueh]

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

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.

[jku]

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.

[jku]

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

[jku]

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. 🤷