feat: add cryptography as required dependency #1929
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively transitions the library to use cryptography as the primary backend for RSA operations, deprecating the rsa library. The introduction of RSASigner and RSAVerifier wrapper classes is a good approach to maintain backward compatibility during the transition. The dependency changes in setup.py and the test updates are well-executed. I've identified a potential security issue regarding private key leakage in an exception, along with some suggestions to improve API consistency and code clarity. Overall, this is a solid step forward.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
daniel-sanche
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully transitions the library to use cryptography as a required dependency for RSA operations, moving away from the deprecated rsa library. The introduction of RSASigner and RSAVerifier wrapper classes is a clean approach to maintain backward compatibility while encouraging migration. The dependency updates and test modifications are all well-executed. I have a couple of minor suggestions to improve the docstrings in the new wrapper classes, which will enhance documentation clarity and prevent potential issues with tooling.
setup.py
Outdated
| reauth_extra_require = ["pyu2f>=0.1.5"] | ||
|
|
||
| # TODO(https://github.com/googleapis/google-auth-library-python/issues/1738): Add bounds for cryptography and pyopenssl dependencies. | ||
| enterprise_cert_extra_require = ["cryptography", "pyopenssl"] |
There was a problem hiding this comment.
Do we need cryptography here? Can https://github.com/googleapis/google-auth-library-python/issues/1738 be closed?
There was a problem hiding this comment.
I think we should be able to remove this. I'll make the change
| # TODO(https://github.com/googleapis/google-auth-library-python/issues/1665): Remove the pinned version of pyopenssl | ||
| # once `TestDecryptPrivateKey::test_success` is updated to remove the deprecated `OpenSSL.crypto.sign` and | ||
| # `OpenSSL.crypto.verify` methods. See: https://www.pyopenssl.org/en/latest/changelog.html#id3. | ||
| "pyopenssl < 24.3.0", |
There was a problem hiding this comment.
non-blocking: For testing, we're using pyopenssl < 24.3.0, we should revisit this in the future so that we're able to test the latest version of pyopenssl. Same with aiohttp below
| from cryptography.hazmat import backends | ||
| from cryptography.hazmat.primitives import serialization | ||
| import pytest | ||
| import rsa as rsa_lib |
There was a problem hiding this comment.
We have import rsa here . Did you intend to add more tests that depend on the deprecated library?
There was a problem hiding this comment.
Yes, I think we should still support using rsa keys when the user explicitly installs it, at least for the short term
This PR changed the interface a bit, where there's now one class that handles both key types. So I added a few extra tests to make sure both key types are supported
3 participants
The
rsalibrary has been deprecated and archived. This PR addscryptographyas a the new preferred backend for RSA operationsIn the short term, both
rsaandcryptographywill be listed as dependencies. Soon,rsawill be removed, but still supported as an optional dependency. Eventually, it will be completely removed from the codebase.As a part of this change, I introduced new RSASigner and RSAVerifier wrapper classes, that can use either cryptography or rsa implementations. Previously, the library would only import one or the other, depending on if cryptography was installed. This simplifies the import structure, and puts rsa and cryptography on equal footing
Fixes #912
Towards #1810
Towards #941