pyca / cryptography

cryptography is a package designed to expose cryptographic primitives and recipes to Python developers.
https://cryptography.io
Other
6.59k stars 1.51k forks source link

Help/Feature: common introspection methods for asymmetric key types #11107

Closed dlenskiSB closed 2 months ago

dlenskiSB commented 3 months ago

As described in https://cryptography.io/en/latest/hazmat/primitives/asymmetric/#common-types

Asymmetric key types do not inherit from a common base class. The following union type aliases can be used instead to reference a multitude of key types.

The lack of common base class(es) and shared functionality can cause problems for code that needs to deal with arbitrary keys of different types, for example code that loads keys from serialization formats like load_ssh_public_key or load_pem_public_key.

Certain operations do work on all key types (e.g. public_bytes(Encoding.DER, PublicFormat.SubjectPublicKeyInfo) to get the key information in a standardized self-describing binary format).

… but there are other operations that users might want to do on keys of arbitrary types, which don't always work.

Two that I've run into:

  1. Determining key size in bits: pubkey.key_size works for {DH,RSA,DS,EC}PublicKey classes… but not for {Ed25519,Ed448,X25519,X448}PublicKey classes which all have fixed key sizes.

    (Of course, key size is not directly comparable across different types of asymmetric encryption.)

  2. Determining the name of the algorithm from the resulting key object: The key classes don't have any introspection methods, and in some cases the names differ between the Python versions and the Rust versions, for example cryptography.hazmat.primitives.asymmetric.ec.EllipticCurvePublicKey vs. cryptography.hazmat.bindings._rust.openssl.ec.ECPublicKey in v42.

My request

Consider adding common introspection methods for the different asymmetric key classes, so that users can rely on common functionality shared across them.

For example, ensure that key_size and algorithm_name methods exist for all asymmetric key classes.

Adding shared base classes (e.g. BaseAsymmetricPublicKey) might make this easier to maintain.

Dealing with the current complexity

In order to get the algorithm name and key size in a consistent format from any of the PublicKeyTypes, I need to use the following code which parses and normalizes the class name, and uses the raw-format public key as a fallback when .key_size doesn't exist:

from typing import Tuple
from cryptography.hazmat.primitives.asymmetric.types import PublicKeyTypes  # 40+

def pubkey_algo_and_key_size(pubkey: PublicKeyTypes) -> Tuple[str, int]:
    # 1. Per https://cryptography.io/en/latest/hazmat/primitives/asymmetric/#common-types
    #    "Asymmetric key types do not inherit from a common base class".
    # 2. Some of them lack a key_size property, but do have a public_bytes_raw() method.
    # 3. For EC, the class name is abbreviated in the Rust version, but spelled out in full in
    #    the Python version. https://github.com/pyca/cryptography/blob/42.0.0/src/cryptography/hazmat/primitives/asymmetric/ec.py#L192

    algo = pubkey.__class__.__name__.removesuffix('PublicKey').replace('EllipticCurve', 'ec').lower()  
    try:
        # this property exists ONLY for dh, rsa, dsa, ec (variable key sizes)
        key_size = pubkey.key_size
    except AttributeError:
        try:
            # this method exists ONLY for ed25519, ed448, x25519, x448
            # (fixed public key size -> straightforward "raw" representation)
            key_size = len(pubkey.public_bytes_raw()) << 3  
        except AttributeError as exc:
            raise NotImplementedError(f"Don't know how to get key_size for {pubkey.__class__.__name__}")

    return algo, key_size
alex commented 3 months ago

Can you share a bit more about what you're trying to accomplish?

In general we don't have a common base class across key types precisely because they are not substitutable: Key sizes mean different things, there are not standard algorithm names (further complicated by some key types can implement multiple algorithms), and operations such as signing require different parameters.

In general, you're probably better of specifically checking which key type is returned and handling it intentionally.

dlenskiSB commented 3 months ago

Can you share a bit more about what you're trying to accomplish?

I'm writing an application for auditing different kinds of keys, so it really wants to be able to handle the common features of different key types in common ways.

there are not standard algorithm names

Right, but as long as there is a reasonable set of names (and most importantly, stable across versions and Rust/C/Python implementations), then those names could be remapped in different ways.

My impression from having used a large number of crypto libraries and tools is that the short names dh, rsa, dsa, ec, ed25519, ed448, x25519, x448 (as output by my code snippet are widely-used in those forms, including by OpenSSL and OpenSSH.

(further complicated by some key types can implement multiple algorithms)

True. "Algorithm name" is too vague. Perhaps something more like key_name, key_type would be better here?

k1 = EllipticCurvePublicKey(...)
assert k1.key_name == 'ec'
assert k1.key_type == 'public'      # Enum this?

k2 = RSAPrivateKye(...)
assert k2.key_name == 'rsa'
assert k2.key_type == 'private'
alex commented 3 months ago

Generally speaking, the approach we'd suggest for this is to use the key interfaces to check key types: https://cryptography.io/en/latest/hazmat/primitives/asymmetric/serialization/#module-cryptography.hazmat.primitives.serialization

On Sun, Jun 16, 2024, 3:21 PM Daniel Lenski @.***> wrote:

Can you share a bit more about what you're trying to accomplish?

I'm writing an application for auditing different kinds of keys, so it really wants to be able to handle the common features of different key types in common ways.

there are not standard algorithm names (further complicated by some key types can implement multiple algorithms)

True. "Algorithm name" is too vague. Perhaps something more like key_name, key_type would be better here?

k1 = EllipticCurvePublicKey(...)assert k1.key_name == 'ec'assert k1.key_type == 'public' # Enum this? k2 = RSAPrivateKye(...)assert k2.key_name == 'rsa'assert k2.key_type == 'private'

— Reply to this email directly, view it on GitHub https://github.com/pyca/cryptography/issues/11107#issuecomment-2171822191, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAGBBMUTM2YBJXPBYPHTTZHXQUZAVCNFSM6AAAAABJLITRVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZRHAZDEMJZGE . You are receiving this because you commented.Message ID: @.***>

github-actions[bot] commented 3 months ago

This issue has been waiting for a reporter response for 3 days. It will be auto-closed if no activity occurs in the next 5 days.

dlenskiSB commented 3 months ago

Generally speaking, the approach we'd suggest for this is to use the key interfaces to check key types: https://cryptography.io/en/latest/hazmat/primitives/asymmetric/serialization/#module-cryptography.hazmat.primitives.serialization

Perhaps I'm missing something, but the key dumping methods (e.g. .public_bytes(Encoding.DER, PublicFormat.SubjectPublicKeyInfo) etc) only support dumping keys in their entirety.

In this case, what I'm trying to do is to summarize features that can be given consistent names across multiple asymmetric key types, even if they can't be directly compared (like the key size in bits).

alex commented 3 months ago

The relevant point there is the example that uses the interface to test key types.

On Fri, Jun 21, 2024 at 1:07 AM Daniel Lenski @.***> wrote:

Generally speaking, the approach we'd suggest for this is to use the key interfaces to check key types:

https://cryptography.io/en/latest/hazmat/primitives/asymmetric/serialization/#module-cryptography.hazmat.primitives.serialization

Perhaps I'm missing something, but the key dumping methods (e.g. .public_bytes(Encoding.DER, PublicFormat.SubjectPublicKeyInfo) etc) only support dumping keys in their entirety.

In this case, what I'm trying to do is to summarize features that can be given consistent names across multiple asymmetric key types, even if they can't be directly compared (like the key size in bits).

— Reply to this email directly, view it on GitHub https://github.com/pyca/cryptography/issues/11107#issuecomment-2182008621, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAGBADAWI5AXFIPT2ZULTZIOYH3AVCNFSM6AAAAABJLITRVSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBSGAYDQNRSGE . You are receiving this because you commented.Message ID: @.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

github-actions[bot] commented 3 months ago

This issue has been waiting for a reporter response for 3 days. It will be auto-closed if no activity occurs in the next 5 days.

dlenskiSB commented 3 months ago

The relevant point there is the example that uses the interface to test key types.

I imagine that many developers, like me, use pyca/cryptography to load asymmetric keys of potentially-arbitrary types (whether via load_pem_x509_certificate().public_key() or hazmat methods like load_pem_public_key or load_ssh_public_key).

Many will then want to determine what kind of key has been loaded in a way that is stable across library versions (and ideally across the "implementation detail" of Rust/C/Python implementation).

Are you saying that everyone who wants this should reimplement such logic? (Whether with a bunch of if/elif as in the docs you cite or with some admittedly horrible introspection hacks like my pubkey.__class__.__name__.removesuffix('PublicKey').replace('EllipticCurve', 'ec').lower())

reaperhulk commented 3 months ago

To take any meaningful action with a key you’ll already need to write per type code and an isinstance check is the preferred method to identify between them. Methods that return strings for comparison are not something we’re interested in pursuing at this time.

github-actions[bot] commented 3 months ago

This issue has been waiting for a reporter response for 3 days. It will be auto-closed if no activity occurs in the next 5 days.

github-actions[bot] commented 2 months ago

This issue has not received a reporter response and has been auto-closed. If the issue is still relevant please leave a comment and we can reopen it.