[DRAFT] fix: remove dependency on rsa#1928
Conversation
daniel-sanche
added
the
do not merge
Summary of ChangesHello @daniel-sanche, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the RSA cryptographic signing and verification mechanisms within the library. The primary goal is to remove the mandatory dependency on the deprecated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 removes the hard dependency on the rsa library by making it an optional dependency and introducing wrapper classes for RSASigner and RSAVerifier. These wrappers can delegate to either cryptography or rsa implementations. This is a good step towards completely removing the deprecated rsa library.
My review has found a critical issue in the implementation of the new wrapper classes in google/auth/crypt/rsa.py. The from_string factory methods incorrectly return instances of the underlying implementation classes instead of the wrapper classes themselves, which breaks the abstraction. I've provided suggestions to fix this. Additionally, I've pointed out some related issues in the new tests and a few minor documentation typos.
| def from_string(cls, public_key): | ||
| """Construct an Verifier instance from a public key or public | ||
| certificate string. | ||
|
|
||
| Args: | ||
| public_key (Union[str, bytes]): The public key in PEM format or the | ||
| x509 public key certificate. | ||
|
|
||
| Returns: | ||
| google.auth.crypt.RSAVerifier: The constructed verifier. | ||
|
|
||
| Raises: | ||
| ValueError: If the public_key can't be parsed. | ||
| ImportError: if neither `cryptograhy` or `rsa` is installe | ||
| """ | ||
| if _cryptography_rsa: | ||
| return _cryptography_rsa.RSAVerifier.from_string(public_key) | ||
| elif _python_rsa: | ||
| return _python_rsa.RSAVerifier.from_string(public_key) | ||
| else: | ||
| raise MissingOptionalDependencyError.create(cls, "cryptography", RSA_NOTE) |
There was a problem hiding this comment.
The from_string classmethod returns an instance of the underlying implementation class (e.g., _cryptography_rsa.RSAVerifier) instead of the wrapper class rsa.RSAVerifier. This breaks the abstraction, as callers would expect an instance of rsa.RSAVerifier.
The method should create an instance of the wrapper class and set its internal _impl to the instance created by the underlying library.
Additionally, there are a few typos in the docstring (an Verifier should be a Verifier, cryptograhy should be cryptography, and installe should be installed).
def from_string(cls, public_key):
"""Construct a Verifier instance from a public key or public
certificate string.
Args:
public_key (Union[str, bytes]): The public key in PEM format or the
x509 public key certificate.
Returns:
google.auth.crypt.RSAVerifier: The constructed verifier.
Raises:
ValueError: If the public_key can't be parsed.
ImportError: if neither `cryptography` or `rsa` is installed
"""
if _cryptography_rsa:
impl = _cryptography_rsa.RSAVerifier.from_string(public_key)
verifier = cls.__new__(cls)
verifier._impl = impl
return verifier
elif _python_rsa:
impl = _python_rsa.RSAVerifier.from_string(public_key)
verifier = cls.__new__(cls)
verifier._impl = impl
return verifier
else:
raise MissingOptionalDependencyError.create(cls, "cryptography", RSA_NOTE)| def from_string(cls, key, key_id=None): | ||
| """Construct an Signer instance from a private key in PEM format. | ||
|
|
||
| Args: | ||
| key (str): Private key in PEM format. | ||
| key_id (str): An optional key id used to identify the private key. | ||
|
|
||
| Returns: | ||
| google.auth.crypt.Signer: The constructed signer. | ||
|
|
||
| RSASigner = _python_rsa.RSASigner # type: ignore | ||
| RSAVerifier = _python_rsa.RSAVerifier # type: ignore | ||
| Raises: | ||
| ValueError: If the key cannot be parsed as PKCS#1 or PKCS#8 in | ||
| PEM format. | ||
| ImportError: if neither `cryptograhy` or `rsa` is installe | ||
| """ | ||
| if _cryptography_rsa: | ||
| return _cryptography_rsa.RSASigner.from_string(key, key_id=key_id) | ||
| elif _python_rsa: | ||
| return _python_rsa.RSASigner.from_string(key, key_id=key_id) | ||
| else: | ||
| raise MissingOptionalDependencyError.create(cls, "cryptography", RSA_NOTE) |
There was a problem hiding this comment.
Similar to RSAVerifier.from_string, this method returns an instance of the underlying implementation class instead of the rsa.RSASigner wrapper class. This should be corrected to return an instance of the wrapper.
Also, there are typos in the docstring (cryptograhy, installe).
def from_string(cls, key, key_id=None):
"""Construct an Signer instance from a private key in PEM format.
Args:
key (str): Private key in PEM format.
key_id (str): An optional key id used to identify the private key.
Returns:
google.auth.crypt.Signer: The constructed signer.
Raises:
ValueError: If the key cannot be parsed as PKCS#1 or PKCS#8 in
PEM format.
ImportError: if neither `cryptography` or `rsa` is installed
"""
if _cryptography_rsa:
impl = _cryptography_rsa.RSASigner.from_string(key, key_id=key_id)
signer = cls.__new__(cls)
signer._impl = impl
return signer
elif _python_rsa:
impl = _python_rsa.RSASigner.from_string(key, key_id=key_id)
signer = cls.__new__(cls)
signer._impl = impl
return signer
else:
raise MissingOptionalDependencyError.create(cls, "cryptography", RSA_NOTE)| Raises: | ||
| ImportError: if neither `cryptograhy` or `rsa` is installed | ||
| ValueError: if an unrecognized public key is provided |
There was a problem hiding this comment.
There's a typo in the Raises section of the docstring. cryptograhy should be cryptography.
| Raises: | |
| ImportError: if neither `cryptograhy` or `rsa` is installed | |
| ValueError: if an unrecognized public key is provided | |
| Raises: | |
| ImportError: if neither `cryptography` or `rsa` is installed | |
| ValueError: if an unrecognized public key is provided |
| Raises: | ||
| ImportError: if neither `cryptograhy` or `rsa` is installed | ||
| ValueError: if an unrecognized public key is provided |
There was a problem hiding this comment.
There's a typo in the Raises section of the docstring. cryptograhy should be cryptography.
Raises:
ImportError: if neither `cryptography` or `rsa` is installed
ValueError: if an unrecognized public key is provided| assert isinstance(verifier, _cryptography_rsa.RSAVerifier) | ||
| assert isinstance(signer, _cryptography_rsa.RSASigner) |
There was a problem hiding this comment.
The isinstance checks here are incorrect because rsa.RSAVerifier.from_string and rsa.RSASigner.from_string are returning instances of the implementation class, not the wrapper class.
Once the from_string methods are fixed to return wrapper instances, this test should be updated to check for the wrapper type, and then optionally check the type of the _impl attribute. For example:
assert isinstance(verifier, rsa.RSAVerifier)
assert isinstance(verifier._impl, _cryptography_rsa.RSAVerifier)
assert isinstance(signer, rsa.RSASigner)
assert isinstance(signer._impl, _cryptography_rsa.RSASigner)| assert bool(result) is True | ||
| assert isinstance(verifier, _python_rsa.RSAVerifier) | ||
| assert isinstance(signer, _python_rsa.RSASigner) |
There was a problem hiding this comment.
This test has a few issues:
- The
isinstancechecks are incorrect and should be updated to check for the wrapper type, similar totest_end_to_end_cryptography_lib. assert bool(result) is Trueis inconsistent with the other test and can be simplified toassert result is Truesince theverifymethod returns a boolean.
After fixing the from_string methods in google/auth/crypt/rsa.py, this test should be updated as follows:
...
result = verifier.verify(message, sig)
assert result is True
assert isinstance(verifier, rsa.RSAVerifier)
assert isinstance(verifier._impl, _python_rsa.RSAVerifier)
assert isinstance(signer, rsa.RSASigner)
assert isinstance(signer._impl, _python_rsa.RSASigner)
|
After some discussion, we will make |
1 participant
The rsa library has been deprecated and archived. This PR will remove it as a required dependency. Users are advised to use the optional cryptography dependency (
google-auth[cryptography]) instead.In the short term,
rsawill still be supported as an optional dependency. In the long term, it will eventually be removedAs 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 use one, depending on if cryptography was installed. This simplifies the import structure, and puts
rsaandcryptographyon equal footing as optional dependenciesNote: Do not merge yet. We will do one release giving a warning, and then this change will come in a future release. We plan on doing a pre-release on pypi of this branch, so users can opt-in to the
rsa-free build early