copy.copy for private keys

[joakimnordling]

Private keys (at least RSAPrivateKey) can not be copied in cryptography 43.0.3:

>>> import copy
>>> from cryptography.hazmat.primitives.asymmetric import rsa
>>> private_key = rsa.generate_private_key(
...     public_exponent=65537,
...     key_size=2048,
... )
>>> copied_key = copy.copy(private_key)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jocke/.pyenv/versions/3.11.7/lib/python3.11/copy.py", line 92, in copy
    rv = reductor(4)
         ^^^^^^^^^^^
TypeError: cannot pickle 'cryptography.hazmat.bindings._rust.openssl.rsa.RSAPrivateKey' object

There was a similar issue for public keys reported in https://github.com/pyca/cryptography/issues/9403 and it seems like the fix to that issue only made it possible to copy all the public keys, not any of the private ones.

[alex]

As I noted there, we've never intentionally supported copying keys. I'm frankly not even sure how Python's default copy implementation could work correctly.

I'm a bit ambivalent about supporting copy with private keys. The same logic about them being immutable holds. Though copy may be more difficult to support for out of process keys, and we certainly don't want to preclude those.

What is the actual use case here?

WDYT @reaperhulk?

[joakimnordling]

The actual use case I had was to make my own RsaPrivateKey (note the lower case sa) subclass of the SecretStr from pydantic so that I could have an environment variable with a private key in PEM format and get it parsed to this "RsaPrivateKey" subclass when loading the settings for my app. My thought was that I would in the __init__ use the load_pem_private_key to parse the key to the RSAPrivateKey (the one from cryptography with capital SA) and store it as an instance variable for later use, similar to how for example the last4 is done in the PaymentCardNumber. The problem is that pydantic decides that it wants to copy the object for some reason when initing the settings (I didn't investigate deeply why it wants to do that). And the reason I want to do it in the init and save it is to avoid doing the load_pem_private_key multiple times, as it's somewhat slow on larger keys. I hope this was not too confusing and I can try to explain in more detail if needed. But in short it's somewhat similar to wrapping it into a dataclass.

[alex]

I'm not familiar with pydantic, but it's surprising to me that it'd be invoking copy on arbitrary types. To me, that seems like the point worth investigating.

[github-actions[bot]]

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.

[joakimnordling]

I agree it's weird that it does that deepcopy, but still I don't see that as a reason why cryptography shouldn't support copying the key. There's likely sooner or later some other use case for it and shouldn't be many lines of change to support it, if it's immutable. So I'd rather ask the question as why should the RSAPrivateKey not support copying?

As a temporary workaround for my particular use case I implemented a __deepcopy__ on the pydantic model itself where I pass the same RSAPrivateKey forward to the new instance by adding an optional extra field to the __init__. A bit of an ugly solution, but it works. And if it was unclear, the stuff it does when initializing the class does a deepcopy on the whole class (and then recursively would have copied the key), not just the key itself.

[alex]

Just as a framing matter, we tend not to ask the question "why should [type X] not support [some operation]", but rather why should it do so. This is because, all other things being equal, narrower interfaces are better: they give users clearer glide paths, and provide more flexibility for implementers down the road.

But, obviously we add new APIs all the time, and we do that via a balancing test. What value is added by the new API, weighed against risks/costs.

In this case, the value is that it'll let private keys be attributes in pydantic types which are copied for unspecified reasons. And the costs/risks are that we need to assess whether there are legitimate reasons that a private key implementor might not be able to provide __deepcopy__ behavior, and thus adding it to the interface would preclude otherwise valid implementors.

So to do this, we need to do that homework of thinking hard about whether there's types of private keys that wouldn't be able to implement this. Once we satisfy ourselves that there's not, we could go ahead with this.

I realize this seems pretty pedantic, but we try hard to avoid boxing ourselves into a corner where we'd be forced to make backwards incompatible changes in the future. (Particularly given that the reason the keys are being copied is a bit unclear and that there is a workaround.)