Ssh keys

[KOLANICH]

Description of the changes being introduced by the pull request:

Allows to import SSH keys as securesystemlib dicts.

Please verify and check that the pull request fulfils the following requirements:

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[jku]

This PRc contains other PRs #453 and #452, let's look at them first.

In future I would appreciate it if PR messages had more context (like mentions of dependencies to other PRs etc)

[jku]

In future I would appreciate it if PR messages had more context (like mentions of dependencies to other PRs etc)

and for bigger feature PRs, a link to an issue that explains the background, design options and decisions would be great

[jku]

WRT SSH support itself I'll leave a bit of context:

• There is an attempt at moving to a more maintainable API in the future, so one wouldn't have to add more and more modules to support new keys-- the current *keys modules are a bit of a maintenance issue
• My PR for some of that work is in signer: Add abstract Key class, implement private key uri scheme for Signer #456
• that one does not yet contain generating keys from scratch or importing them, but I believe we can have an API where
  • normal usage (verifying and signing) is possible with just using generic Signer and Key APIs -- no need to call keytype specific modules
  • for initial import of keys, a Signer subclass method is used -- in your case that could be SSHSigner.import(private_key_path, pubkey_path)
  • implementing Signers and Keys is possible outside of securesystemslib tree, in the applications themselves

None of the above prevents taking your SSH work in tree but I'm saying this to explain why there might be some friction -- The various PRs (and a in-toto/securesystemslib fork) are pushing to several directions right now so choosing the right path may not be obvious...

[KOLANICH]

Thank you for letting me know.

[KOLANICH]

for initial import of keys, a Signer subclass method is used

It is highly illogical and inconvenient.

[lukpueh]

for initial import of keys, a Signer subclass method is used

It is highly illogical and inconvenient.

Please try to be constructive and respectful in your comments.

[lukpueh]

I appreciate the effort to support standard formats. Thanks for the initiative, @KOLANICH!

I suggest to land #456 first and design key imports around it. We can leave this PR open, maybe as draft, and use it as inspiration once we have a clear vision.

[KOLANICH]

OK, this code is pretty standalone, I can temporarily put it to another package.

[KOLANICH]

Worked around by carrying an impl in https://github.com/KOLANICH/securesystemslib_KOLANICH.py

[lukpueh]

Apologies for the long radio silence. We did land #456 a while ago, and the resulting signer API has turned out quite nicely and also quite easily extendable (we have GCP, HSM, and sigstore (WIP) support) . Official docs are still sparse, but there is a nice blog post and browsing the code should also give you a good idea of the concept.

Are you interested in contributing an SSHSigner, as @jku suggested above, which supports your uses cases? It could look something like this:

class SSHSigner(Signer):
  @classmethod
  def import_file(cls, private_key_path: str, public_key_path: str) -> Tuple[str, SSlibKey]:
    # Load public key from public_key_path.
    # Convert ssh-formatted key data to create a valid SSlibKey from it.
    # Construct URI, used to load the actual signer, e.g.
    #   f"ssh+file:/{private_key_path}".
    # Return uri and public key.

  @classmethod
  def from_priv_key_uri(cls, priv_key_uri: str, public_key: Key, secrets_handler: Optional[SecretsHandler] = None) -> "SSHSigner":
    # Load private key from priv_key_uri.
    # Convert ssh-formatted key data to a dictionary accepted
    #   by securesystemslib.keys.create_signature (see SSlibSigner.key_dict).
    # Return signer.

  def sign(self, payload: bytes) -> Signature:
    # Sign using securesystemslib.keys.create_signature (see SSlibSigner.sign). """

The high-level idea for (public key) import in the signer API is also outlined in #466.

[KOLANICH]

Thanks for giving me the link to the blog post. I'm sorry for the harsh words I have to say, but I absolutely hate the design of this lib, the concept of key URIs and different signers for different keys.

1. Key uris encode the info from where the key is taken from, not about the key identity. We cannot use such URIs in untrusted data, for example if we embed such an URI into a TUF dict, when a user's impl tries to use the embedded URI, it'd access the source of a key. Let's imagine an attacker has put an HTTP URI there ... then everyone parsing a key from that URI will access attacker's server directly, even if the dict was got through an intermediary, this way disclosing the attacker IP addresses of victims to be scanned for vurnrs or to be exploited via 0-days in network stack. If we limit the URIs to some types of sources ... then the URIs make no sense and contain redundant info.
2. I like the idea of multiple signers because there may be multiple formats of signatures ... but binding signers to key types is IMHO not correct. For example Satoshi could have taken a key from blockchain and use it to sign a message to prove his identity, if he wanted.
3. that blog post says about API bloating .... but introducing signers bound to keys is exactly this.

IMHO the API should be the following:

• an interface for a Key of any type. Keys in narrow sense are mathematical objects.
• an interface for a Certificate, which is a key with additional metadata.
• an interface for a CryptoImpl, be it a software one or a hardware one. For the ones where keys are available, keys are stored within Key subclasses. For non-extractable keys they store handles.
• aCryptoImpl can do crypto operations and gen signatures and keys of own format. But "own format" for keys means just that in order to gen a key we have to call a method on an impl object. The generated key object can in most of the cases be used to other impls objects. If an incompatible key is passed to an impl, it is a runtime error.
• about API bloat... one should distinguish between different kinds of API. There is an API between core and extensions which should be pretty stable (and even in this case, some very small subset of it can be set in stone and the rest can be versioned), and API between extensions and their user. Introducing new user-extension API for hew functionality in extensions is inevitable. User-extension API can be easily updated without any stability guarantees - just a new extension is created with new API and ones who need old API use a pegacy package and the ones who have migrated use the new package.

IMHO current securesystemslib functionality is covered by existing libs like cryptography and using securesystemslib brings no benefit. It is not an abstraction layer around other crypto libs. It is just the way to force the vision "all the keys should be base64 serialized strings and should go from strings into in-memory kbjects and back on every operation". IMHO it'd have been better if the official TUF impl relied on either an exact blessed impl (like cryptography or nacl/sodium) not created by TUF impl authors, or to a properly designed abstraction layer around crypto libs.

[lukpueh]

Okay, thanks for you feedback! From what I understand, you are not interested in integrating your feature with our API. So, I'll close this PR. Feel free to submit another one if you reconsider.