support SSH certificate parsing

[reaperhulk]

No description provided.

[mjpieters]

Is there any reason this has to be a separate function?

It feels to me like load_ssh_public_key() can can return a plain key or a certificate, because the certificate itself is also a public key. This is how the go crypto library handles parsing, it returns a PublicKey interface (the Go equivalent of an ABC), and Certificate is one implementation of the interface.

If I have a pile of SSH public keys as files, I'd much rather use:

keys: Iterable[bytes]
for keydata in keys:
    key = ssh.load_ssh_public_key(keydata)
    if isinstance(key, SSHCertificate):
        # do something with a certificate, like verifying it against a private key
    else:
        # do something with a non-cert public key, like checking it against a authorized keys store

than have to catch the ValueError and check that it signalled that the key wasn't a certificate key:

keys: Iterable[bytes]
for keydata in keys:
    try:
        key = ssh.load_ssh_public_key(keydata)
        # do something with a certificate, like verifying it against a private key
    except ValueError as ex:
        if ex.args[0] != "Not an SSH certificate":
            raise
    else:
        # do something with a non-cert public key, like checking it against a authorized keys store

If this must be a separate function, at least give us a better way to distinguish certificate public keys from non-certificate public keys.

If, instead, all public keys and the SSHCertificate shared a common ABC (with public_bytes(encoding: Encoding, format: PublicFormat) -> bytes and verify(signature: bytes, data: bytes, *args, **kwargs) -> None methods) then that'd aid many usecases that don't even need to know exactly what type of key was loaded:

# verify that the keys are in a valid format and re-encode to a canonical form
verified: list[bytes] = []
for keydata in keys:
    try:
        key = ssh.load_ssh_public_key(keydata)
        verified.append(key.public_bytes(Encoding.OpenSSH, PublicFormat.OpenSSH))
    except ValueError:
        logger.warn("%s is not a valid SSH public key", keydata)

[reaperhulk]

There's no hard requirement that this has to be a separate function, but having it return SSHCertificate for certs would represent a breaking API change. We could instead make the new function load_ssh_public_key_or_cert (or some better name) though.

[mjpieters]

There's no hard requirement that this has to be a separate function, but having it return SSHCertificate for certs would represent a breaking API change. We could instead make the new function load_ssh_public_key_or_cert (or some better name) though.

A, yes, code that currently expects the unwrapped public key without certificate metadata is going to be unhappy. However, wouldn't a default-False option on the function not be better option then? E.g., expressed as overloads:

@typing.overload
def load_ssh_public_key(
    data: bytes, backend: typing.Any = None, full_certificate: typing.Literal[False] = False
) -> _SSH_PUBLIC_KEY_TYPES: ...

@typing.overload
def load_ssh_public_key(
    data: bytes, backend: typing.Any = None, full_certificate: typing.Literal[True] = True
) -> typing.Union[_SSH_PUBLIC_KEY_TYPES, SSHCertificate]: ...

@typing.overload
def load_ssh_public_key(
    data: bytes, backend: typing.Any = None, full_certificate: bool
) -> typing.Union[_SSH_PUBLIC_KEY_TYPES, SSHCertificate]: ...

def load_ssh_public_key(
    data: bytes, backend: typing.Any = None, full_certificate: bool = False
) -> typing.Union[_SSH_PUBLIC_KEY_TYPES, SSHCertificate]:
    """Load public key from OpenSSH one-line format.

    If full_cerficate is True, will return a SSHCertificate instance for ssh certificates, otherwise, only
    the embedded public key is returned.
    
    """

(not married to full_certificate as the name of the flag, could very much be improved upon).

[reaperhulk]

@mjpieters Trying to align our types to allow Go-like interfaces here would be very difficult and would result in odd situations like verify on SSHCertificate meaning something entirely different from verify on a public key. There's also no meaning to encoding formats for an SSH certificate as best I can tell since it only has one?

You've definitely convinced me that we should support both in a single function though, so this PR has been updated to implement that as load_ssh_public_identity. A future PR will implement load_ssh_public_key in terms of this function to simplify the code, but we'll leave both.

[alex]

apparently this is uncovered?