Revise module architecture and define clear API modules and functions

[lukpueh]

Description of issue or feature request: Securesystemslib lacks of a clear public API. It should be clear and intuitive for users of secureystemslib, which modules and which functions are public API.

Current behavior: API is scattered across:

• for general key operations (generate, import, sign, verify)

  • keys.py -- high-level public interface to sign/very (key type independent), and generate and import keys (key type dependent). Calls into low-level non-public {rsa, ecdsa, ed25519}_keys.py modules, which are (mostly) separated by key type.
  • interface.py -- higher-level public interface (mostly calls into key.py) to generate and import keys

• for GPG key operations (import, sign, verify)

  • gpg/functions.py -- public interface for gpg subpackage, independent from above key operations.

• for other other non-key related operations

  • hash.py -- facade for hashlib from stdlib and cryptography.hazmat.primitives.hashes
  • process.py -- thin subprocess wrapper
  • storage.py-- file system abstraction
  • util.py -- mostly I/O related utils
  • formats.py-- schema definition constants (likely to be deprecated, see https://github.com/secure-systems-lab/securesystemslib/issues/183), OLPC canonical json implementation

Expected behavior: Revise module architecture to use mnemonic names for (public) modules (not interface or functions) appropriate for the interface functions they contain. Also see discussion about import guidelines.

[trishankatdatadog]

💯 💯 💯

[lukpueh]

Following discussion in https://github.com/secure-systems-lab/securesystemslib/pull/409#issuecomment-1161469582 and recent developments related to the python-tuf v1 refactor, I suggest to shift signature-related API to Signer, Key, Signature classes, and making keys.{create, verify}_signature internal or deprecating them altogether once in-toto/tuf has migrated usage.

Why classes?

• stronger data model + validation (#183, python-tuf#1140)
• more flexibility (python-tuf#1263)

Details about these classes

• Signature

  • base data class for keyid and signature bytes
  • should be usable directly regardless of key/signature type, because of simple data structure and no behavior
  • can be subclassed, if additional fields are needed (e.g. #341), although those could be captured in the opaque unrecognized_fields dict.

• Signer

  • abstract base class that provides single signing key (access) and a sign method
  • implemented by SSlibSigner, which holds a securesystemslib-style private key dictionary and calls keys.create_signature dispatching on key type and signature scheme
  • implemented by GPGSigner (pending), which holds a gpg key id and calls gpg.functions.create_signature

• Key

  • class that provides public key and signature verification method
  • implemented in python-tuf, but seems like a good fit for securesystemslib
  • holds a securesystemslib-style public key dictionary and calls keys.verify_signature dispatching on key type and signature scheme
  • EDIT: Since writing this comment Key has been ported from python-tuf to the in-toto/securesystemslib fork, but is not yet available upstream

More thoughts

• Current SSlibSigner and Key implementation seem like good initial defaults, when replacing public usage of keys.{create, verify}_signature
• I suggest to rename the current Key class to SslibKey that inherits from an abstract base class Key, for more flexibility
• We might want to consider replacing SSlibSigner and SslibKey with less generic classes that are based on key types, and use polymorphism instead of long dispatch tables for behavior.
• We might want to consider enriching these classes with de/serialization behavior currently scatterd over keys and secureystemslib modules.

[lukpueh]

We might want to consider replacing SslibSigner and SslibKey with less generic classes that are based on key types, and use polymorphism instead of long dispatch tables for behavior.

I am thinking of classes like RSASigner, Ed25519Signer, ECDSASigner (instead of SSlibSigner); and RSAKey, etc. (instead of SslibKey). Note that these could be very thin (opinionated) wrappers around the lower-level pyca/cryptography objects that we already use, e.g.:

from cryptography.hazmat.primitives.asymmetric.rsa import RSAPrivateKey

class RSASigner(Signer):
    key: RSAPrivateKey
    ...

    def sign(self, payload: bytes) -> Signature:
      _sig = self.key.sign(payload, *opinionated_args)
      return Signature.from_pyca_crypto(_sig)

or with multi-inheritance

class RSASigner(Signer, RSAPrivateKey):
    ...
    def sign(self, payload: bytes) -> Signature:
      _sig = RSAPrivateKey.sign(self, payload, *opinionated_args)
      return Signature.from_pyca_crypto(_sig)

I think this could save many lines of abstraction-layer code in securesystemslib.

[MVrachev]

Overall the suggestions sound good.

On the topic of RSASigner example (as @lukpueh has already heard my opinion :D) I am not a fan of adding an inheritance just for the sake of simpler implementation without logical connection. In this example, I don't see a logical relationship between RSASigner and RSAPrivateKey. This of course is just my opinion and the maintainers should decide.

One minor naming suggestion - why don't we use SSlibSigner instead of SslibSigner? That's for all naming suggestions, I think double upper case S looks better.

[lukpueh]

One minor naming suggestion - why don't we use SSlibSigner instead of SslibSigner? That's for all naming suggestions, I think double upper case S looks better.

Agreed, the class in securesystemslib is actually called SSlibSigner. I'll update comments above...

[lukpueh]

In this example, I don't see a logical relationship between RSASigner and RSAPrivateKey.

The connection is that there is a 1-to-1 relationship between signing and private key (for tuf/in-toto), so it is possible to combine them in one class.

Of course, you could create an RSASigner class, which has an RSAPrivateKey attribute, but for our use case I don't see the benefit over having just one class that contains both the private key data and also has a sign method. You could call this one class either RSASigner or RSAPrivateKey, but for tuf/in-toto we care more about the signing aspect and less about the key aspect. Also, sometimes the signer isn't even a key, but just a reference to a key (e.g. GPGSigner - #341).

[jku]

my improvement ideas are in https://docs.google.com/document/d/1KQY5v1ASXpZM5GhfATV_WJf7GaMJrfBOXr06dtxc-ps/ but I'll summarize here -- just adding to what Lukas already listed. I believe this leads us to an API that is complete: users only need Key + Signer, there is no need to import any other securesystemslib modules.

Signer

• Signer can have multiple implementations (e.g. GCPKMSSigner, PGPSigner, SigstoreSigner), in SSLib or outside
• users of Signer API should not need to use the specific implementation, just Signer API. This covers Signer construction and signing
• signers are constructed using a "private key URI" configuration value that encodes both Signer implementation and the private key content location (see doc for details)
• the one special case where users would need the specific implementation is new key generation/import: these would be implementation specific methods
• Signer always has access to the corresponding Key (this makes many things easier)

Key

• Key can have multiple implementations, in SSLib or external
• users of Key API should never need to use the specific implementation, only the generic Key API. This includes serialization, deserialization and signature verification

The core idea is: The specific implementations of Key and Signer are 99% implementation details that securesystemslib users should not need to know about. My understanding is that we can do that: the only exception will be key generation/import, which should be implementation specific.

[jku]

I tend to agree that smaller specific implementations (e.g. RSAKey) would be good but note that if we manage what is proposed above, it is only relevant to users for the specific case of key generation/import. Otherwise how the implementation is split is an implementation detail that we can change without breaking API.

[lukpueh]

731 has removed much of the legacy "API" in favour of securesystemslib.signer. IMO this new Signer API is relatively clear and well documented. Remaining documentation deficits are described in reasonably scoped issues (see "docs" label).

Apart from securesystemslib.signer we still have a few modules/subpackages, which don't fit in quite well, but also can't just be removed.

• hash.py, storage.py and secureysystemslib.gpg -- I'd like to treat them as public but hidden API, to be used by in-toto and/or tuf. No idea how to best express this. Maybe make them public but document that they are for "internal" use?

• dsse.py and formats.py -- I think it's okay to treat them as public API, because they are reference implementations related to the two signature wrapper paradigms established by tuf and in-toto.