Skip to content

Conversation

@harkamaljot
Copy link
Contributor

This test is to test if the auth lib can use the encrypted private key, however this path is being tested using other test cases, as by removing this test case the coverage doesn't change. So removing this test case as it is using deprecated code.

@harkamaljot harkamaljot requested review from a team as code owners March 4, 2025 00:53
sai-sunder-s
sai-sunder-s previously approved these changes Mar 4, 2025
@chalmerlowe
Copy link
Contributor

I am concerned that this PR has unstated side effects.

While not stated in the PR description, it appears that this PR is ultimately about clearing a blocker that prevents resolving a TODO:

# setup.py

# 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",

By removing the entire test, we do indeed eliminate the .sign() and .verify() function calls.

HOWEVER, we end up in a situation whereby we do not fully/properly exercise the _mtls_helper.decrypt_private_key() method, which looses it's only positive test case (and thus we can't confirm that our implementation of decrypt_private_key() is correct). The remaining test of _mtls_helper.decrypt_private_key() only checks the negative case (i.e. when the password is incorrect).

Thus, the statement: "as this path is being tested using other test cases" is only partially correct.

In addition, this PR includes a potentially unneeded commit that updates the system_tests/secrets.tar.enc. Ideally that should be in a separate PR OR the rationale for the update should be included for clarity.

Rather than deleting the test, I suggest that we:

  • refactor the test to verify the key loads correctly using primitives from cryptography instead of using the outdated OpenSSL.crypto.sign and OpenSSL.crypto.verify methods.
  • eliminate the commit that includes secrets.tar.enc
  • update the affected statements in setup.py

To update the test:

# Add these imports at the top
from cryptography.hazmat.primitives import hashes
from cryptography.hazmat.primitives.asymmetric import ec

# Update the test class
class TestDecryptPrivateKey(object):
    def test_success(self):
        decrypted_key = _mtls_helper.decrypt_private_key(
            ENCRYPTED_EC_PRIVATE_KEY, PASSPHRASE_VALUE
        )
        private_key = crypto.load_privatekey(crypto.FILETYPE_PEM, decrypted_key)
        public_key_openssl = crypto.load_publickey(crypto.FILETYPE_PEM, EC_PUBLIC_KEY)

        # Test the decrypted key works by signing and verification.
        # Use cryptography library for signing and verification instead of the deprecated
        # OpenSSL.crypto.sign and OpenSSL.crypto.verify.
        private_key_cryptography = private_key.to_cryptography_key()
        public_key_cryptography = public_key_openssl.to_cryptography_key()
        signature = private_key_cryptography.sign(
            b"data", ec.ECDSA(hashes.SHA256())
        )
        public_key_cryptography.verify(
            signature, b"data", ec.ECDSA(hashes.SHA256())
        )

To update setup.py, remove the bounds and the comment for pyopenssl:

    ...
    "aioresponses",
    "pytest-asyncio",
    "pyopenssl",
    # TODO(https://github.com/googleapis/google-auth-library-python/issues/1722): `test_aiohttp_requests` depend on
    # aiohttp < 3.10.0 which is a bug. Investigate and remove the pinned aiohttp version.
    "aiohttp < 3.10.0",
    ...

@chalmerlowe
Copy link
Contributor

chalmerlowe commented Dec 26, 2025

@harkamaljot

I am concerned that this PR has unstated side effects.

While not stated in the PR description, it appears that this PR is ultimately about clearing a blocker that prevents resolving a TODO:

# setup.py

# 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",

By removing the entire test, we do indeed eliminate the .sign() and .verify() function calls.

HOWEVER, we end up in a situation whereby we do not fully/properly exercise the _mtls_helper.decrypt_private_key() method, which loses it's only positive test case (and thus we can't confirm that our implementation of decrypt_private_key() is correct). The remaining test of _mtls_helper.decrypt_private_key() only checks the negative case (i.e. when the password is incorrect).

Thus, the statement: "as this path is being tested using other test cases" is only partially correct.

In addition, this PR includes a potentially unneeded commit that updates the system_tests/secrets.tar.enc. Ideally that should be in a separate PR OR the rationale for the update should be included for clarity.

Rather than deleting the test, I suggest that we

  • refactor the test to verify the key loads correctly using primitives from cryptography instead of using the outdated OpenSSL.crypto.sign and OpenSSL.crypto.verify methods.
  • eliminate the commit that includes secrets.tar.enc
  • update the affected statements in setup.py

To update the test:

# Add these imports at the top
from cryptography.hazmat.primitives import hashes
from cryptography.hazmat.primitives.asymmetric import ec

# Update the test class
class TestDecryptPrivateKey(object):
    def test_success(self):
        decrypted_key = _mtls_helper.decrypt_private_key(
            ENCRYPTED_EC_PRIVATE_KEY, PASSPHRASE_VALUE
        )
        private_key = crypto.load_privatekey(crypto.FILETYPE_PEM, decrypted_key)
        public_key_openssl = crypto.load_publickey(crypto.FILETYPE_PEM, EC_PUBLIC_KEY)

        # Test the decrypted key works by signing and verification.
        # Use cryptography library for signing and verification instead of the deprecated
        # OpenSSL.crypto.sign and OpenSSL.crypto.verify.
        private_key_cryptography = private_key.to_cryptography_key()
        public_key_cryptography = public_key_openssl.to_cryptography_key()
        signature = private_key_cryptography.sign(
            b"data", ec.ECDSA(hashes.SHA256())
        )
        public_key_cryptography.verify(
            signature, b"data", ec.ECDSA(hashes.SHA256())
        )

To update setup.py, remove the bounds and the comment for pyopenssl:

    ...
    "aioresponses",
    "pytest-asyncio",
    "pyopenssl",
    # TODO(https://github.com/googleapis/google-auth-library-python/issues/1722): `test_aiohttp_requests` depend on
    # aiohttp < 3.10.0 which is a bug. Investigate and remove the pinned aiohttp version.
    "aiohttp < 3.10.0",
    ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants