[Experiment] feat(auth): backport rsa to 2.49.2#16805
[Experiment] feat(auth): backport rsa to 2.49.2#16805daniel-sanche wants to merge 3 commits intomainfrom
Conversation
daniel-sanche
added
the
do not merge
![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 refactors the cryptographic implementation handling within the google-auth library, primarily moving the 'rsa' library from an optional extra to a core dependency and removing its deprecation warnings. It also transitions the RSA signer and verifier from a dynamic runtime wrapper to a static import-time selection. Review feedback identifies several critical issues: a functional regression in rsa.py that breaks support for multiple key types, a bug in noxfile.py causing in-place modification of a global constant, and a syntax error in setup.py involving a nested list. Additionally, the removal of *session.posargs in the test session is noted as a regression in developer tooling, and improvements for type-hinting precision and docstring typos are suggested.
| try: | ||
| # Prefer cryptograph-based RSA implementation. | ||
| from google.auth.crypt import _cryptography_rsa | ||
|
|
||
| from google.auth import _helpers | ||
| from google.auth.crypt import _cryptography_rsa | ||
| from google.auth.crypt import base | ||
| RSASigner = _cryptography_rsa.RSASigner | ||
| RSAVerifier = _cryptography_rsa.RSAVerifier | ||
| except ImportError: # pragma: NO COVER | ||
| # Fallback to pure-python RSA implementation if cryptography is | ||
| # unavailable. | ||
| from google.auth.crypt import _python_rsa | ||
|
|
||
| RSA_KEY_MODULE_PREFIX = "rsa.key" | ||
|
|
||
|
|
||
| class RSAVerifier(base.Verifier): | ||
| """Verifies RSA cryptographic signatures using public keys. | ||
|
|
||
| Args: | ||
| public_key (Union["rsa.key.PublicKey", cryptography.hazmat.primitives.asymmetric.rsa.RSAPublicKey]): | ||
| The public key used to verify signatures. | ||
| Raises: | ||
| ImportError: if called with an rsa.key.PublicKey, when the rsa library is not installed | ||
| ValueError: if an unrecognized public key is provided | ||
| """ | ||
|
|
||
| def __init__(self, public_key): | ||
| module_str = public_key.__class__.__module__ | ||
| if isinstance(public_key, RSAPublicKey): | ||
| impl_lib = _cryptography_rsa | ||
| elif module_str.startswith(RSA_KEY_MODULE_PREFIX): | ||
| from google.auth.crypt import _python_rsa | ||
|
|
||
| impl_lib = _python_rsa | ||
| else: | ||
| raise ValueError(f"unrecognized public key type: {type(public_key)}") | ||
| self._impl = impl_lib.RSAVerifier(public_key) | ||
|
|
||
| @_helpers.copy_docstring(base.Verifier) | ||
| def verify(self, message, signature): | ||
| return self._impl.verify(message, signature) | ||
|
|
||
| @classmethod | ||
| 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.Verifier: The constructed verifier. | ||
|
|
||
| Raises: | ||
| ValueError: If the public_key can't be parsed. | ||
| """ | ||
| instance = cls.__new__(cls) | ||
| instance._impl = _cryptography_rsa.RSAVerifier.from_string(public_key) | ||
| return instance | ||
|
|
||
|
|
||
| class RSASigner(base.Signer, base.FromServiceAccountMixin): | ||
| """Signs messages with an RSA private key. | ||
|
|
||
| Args: | ||
| private_key (Union["rsa.key.PrivateKey", cryptography.hazmat.primitives.asymmetric.rsa.RSAPrivateKey]): | ||
| The private key to sign with. | ||
| key_id (str): Optional key ID used to identify this private key. This | ||
| can be useful to associate the private key with its associated | ||
| public key or certificate. | ||
|
|
||
| Raises: | ||
| ImportError: if called with an rsa.key.PrivateKey, when the rsa library is not installed | ||
| ValueError: if an unrecognized public key is provided | ||
| """ | ||
|
|
||
| def __init__(self, private_key, key_id=None): | ||
| module_str = private_key.__class__.__module__ | ||
| if isinstance(private_key, RSAPrivateKey): | ||
| impl_lib = _cryptography_rsa | ||
| elif module_str.startswith(RSA_KEY_MODULE_PREFIX): | ||
| from google.auth.crypt import _python_rsa | ||
|
|
||
| impl_lib = _python_rsa | ||
| else: | ||
| raise ValueError(f"unrecognized private key type: {type(private_key)}") | ||
| self._impl = impl_lib.RSASigner(private_key, key_id=key_id) | ||
|
|
||
| @property # type: ignore | ||
| @_helpers.copy_docstring(base.Signer) | ||
| def key_id(self): | ||
| return self._impl.key_id | ||
|
|
||
| @_helpers.copy_docstring(base.Signer) | ||
| def sign(self, message): | ||
| return self._impl.sign(message) | ||
|
|
||
| @classmethod | ||
| def from_string(cls, key, key_id=None): | ||
| """Construct a 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. | ||
| """ | ||
| instance = cls.__new__(cls) | ||
| instance._impl = _cryptography_rsa.RSASigner.from_string(key, key_id=key_id) | ||
| return instance | ||
| RSASigner = _python_rsa.RSASigner # type: ignore | ||
| RSAVerifier = _python_rsa.RSAVerifier # type: ignore |
There was a problem hiding this comment.
This change introduces a functional regression. The previous implementation (on the LEFT) was a dynamic wrapper that supported both cryptography and rsa library key objects at runtime. By statically aliasing RSASigner and RSAVerifier to one implementation or the other at import time, the library loses its ability to handle different key types interchangeably if both libraries are present.
| "3.14", | ||
| ] | ||
| ALL_PYTHON = UNIT_TEST_PYTHON_VERSIONS.copy() | ||
| ALL_PYTHON = UNIT_TEST_PYTHON_VERSIONS |
There was a problem hiding this comment.
Removing .copy() causes ALL_PYTHON to reference the same list as the UNIT_TEST_PYTHON_VERSIONS constant. The subsequent .extend(["3.7"]) call will modify the global constant in-place, which is likely unintended and could affect other sessions.
| ALL_PYTHON = UNIT_TEST_PYTHON_VERSIONS | |
| ALL_PYTHON = UNIT_TEST_PYTHON_VERSIONS.copy() |
| enterprise_cert_extra_require = ["cryptography", "pyopenssl"] | ||
|
|
||
| pyopenssl_extra_require = ["pyopenssl>=20.0.0"] | ||
| pyopenssl_extra_require = ["pyopenssl>=20.0.0", cryptography_base_require] |
There was a problem hiding this comment.
cryptography_base_require is a list. Including it directly in pyopenssl_extra_require creates an invalid nested list. It should be unpacked using the * operator, as done elsewhere in this file.
| pyopenssl_extra_require = ["pyopenssl>=20.0.0", cryptography_base_require] | |
| pyopenssl_extra_require = ["pyopenssl>=20.0.0", *cryptography_base_require] |
| es = None # type: ignore | ||
| es256 = None # type: ignore |
There was a problem hiding this comment.
Following the repository's general rules, use the more specific # type: ignore[assignment] when assigning None to optional dependencies in an except ImportError block to improve type checking precision.
| es = None # type: ignore | |
| es256 = None # type: ignore | |
| es = None # type: ignore[assignment] | |
| es256 = None # type: ignore[assignment] |
References
- When handling an optional dependency by assigning None in an except ImportError block, type: ignore[assignment] is necessary on the assignment to prevent a mypy error, as using typing.TYPE_CHECKING alone is insufficient.
| @classmethod | ||
| def from_string(cls, public_key: Union[str, bytes]) -> "EsVerifier": | ||
| """Construct a Verifier instance from a public key or public | ||
| """Construct an Verifier instance from a public key or public |
There was a problem hiding this comment.
Typo in docstring: "an Verifier" should be "a Verifier".
| """Construct an Verifier instance from a public key or public | |
| """Construct a Verifier instance from a public key or public |
| RSASigner = _python_rsa.RSASigner # type: ignore | ||
| RSAVerifier = _python_rsa.RSAVerifier # type: ignore |
There was a problem hiding this comment.
Use a more specific type ignore like # type: ignore[assignment] when assigning classes in an except ImportError block, consistent with the repository's general rules for optional dependencies.
| RSASigner = _python_rsa.RSASigner # type: ignore | |
| RSAVerifier = _python_rsa.RSAVerifier # type: ignore | |
| RSASigner = _python_rsa.RSASigner # type: ignore[assignment] | |
| RSAVerifier = _python_rsa.RSAVerifier # type: ignore[assignment] |
References
- When handling an optional dependency by assigning None in an except ImportError block, type: ignore[assignment] is necessary on the assignment to prevent a mypy error, as using typing.TYPE_CHECKING alone is insufficient.
| "tests", | ||
| "tests_async", | ||
| *session.posargs, | ||
| ) |
There was a problem hiding this comment.
The removal of *session.posargs prevents users from passing additional arguments to pytest from the command line, which is a regression in developer tooling functionality.
| ) | |
| *session.posargs, | |
| ) |
1 participant
Test revert commits that removed rsa, to see if the build passes
This will not be released, but may be useful as a temporary branch