Support for copy.deepcopy keys #13759
Conversation
src/rust/src/backend/dh.rs
Outdated
| slf: pyo3::PyRef<'p, Self>, | ||
| _memo: &pyo3::Bound<'p, pyo3::types::PyDict>, | ||
| ) -> CryptographyResult<Self> { | ||
| let new_key = Self { |
There was a problem hiding this comment.
You shouldn't need to do this, because keys are immutable a version of return self (like __copy__) should be enough.
There was a problem hiding this comment.
Ok I didn't know that, I'll basically use the same as __copy__ then. Thanks for the quick review !
src/rust/src/backend/dsa.rs
Outdated
| } | ||
|
|
||
| fn __eq__(&self, other: pyo3::PyRef<'_, Self>) -> bool { |
There was a problem hiding this comment.
What's going on here? Why is this needed?
There was a problem hiding this comment.
I forgot to uncommit it. I removed it.
src/rust/src/backend/dh.rs
Outdated
|
|
||
| fn __deepcopy__<'p>( | ||
| slf: pyo3::PyRef<'p, Self>, | ||
| _memo: &pyo3::Bound<'p, pyo3::types::PyDict>, |
There was a problem hiding this comment.
nit: given these are unused, it's probably better to just type them as PyAny
tests/hazmat/primitives/test_dsa.py
Outdated
| key2 = copy.deepcopy(key1) | ||
| assert key1 == key2 | ||
|
|
||
| key1 = dsa.generate_private_key(2048).public_key() |
There was a problem hiding this comment.
IIRC dsa key generation is quite slow, is there some other key we can load to test here?
There was a problem hiding this comment.
I replaced the generated key by a key in an already existing PEM file (this one : vectors/cryptography_vectors/asymmetric/Traditional_OpenSSL_Serialization/dsa.1024.pem). The goal of this test is mainly to make sure that deepcopy really creates a deep copy of the key. But if I understand correctly, @joakimnordling in issue #13588 is just asking for copy.deepcopy to work on keys.
tests/hazmat/primitives/test_rsa.py
Outdated
| key1 = generate_private_key( | ||
| public_exponent=65537, key_size=2048 | ||
| ).public_key() |
There was a problem hiding this comment.
same, RSA key gen also takes appreciable time, there should be other vectors available
There was a problem hiding this comment.
For this I replaced the generated key with the test fixture rsa_key_512.
Signed-off-by: Courtcircuits <[email protected]>
Signed-off-by: Courtcircuits <[email protected]>
| """ | ||
|
|
||
| @abc.abstractmethod | ||
| def __deepcopy__(self) -> DHPublicKey: |
There was a problem hiding this comment.
Isn't the __deepcopy__ expected to take in the memo as a second parameter as described on https://docs.python.org/3/library/copy.html#object.__deepcopy__ ?
There was a problem hiding this comment.
oof yes, not sure how I missed this. Will send a fix.
There was a problem hiding this comment.
In case you missed it, the version in docs/hazmat/primitives/asymmetric/cloudhsm.rst has a memodict (not sure if anything cares about if it has that name of memo, but I'd at least change it to match the name in the documentation. And also note that the parameter is missing in not just this one I commented on, but all the *.py files.
3 participants
This adds support for deep copying keys. This resolves issue #13588.
I tried to follow as close as possible what was done in PR #12110. Hope it fits what was expected for this issue !
Basically, this PR adds deep copy support for :