feat: add post-quantum cryptography (PQC) integration tests and fixtures#17586
feat: add post-quantum cryptography (PQC) integration tests and fixtures#17586ohmayr wants to merge 1 commit into
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 adds support for mTLS in REST transport fixtures within conftest.py and introduces a new integration test in test_pqc.py to verify Post-Quantum Cryptography (PQC) negotiation with the Showcase server. The review feedback highlights several critical improvements: resolving a compatibility issue with urllib3 v2.0+ by overriding cert_verify instead of using assert_hostname in PoolManager, adding defensive None checks for gRPC metadata and interceptor attributes to prevent runtime errors, and avoiding shadowing the built-in dir function.
| class HostNameIgnoringAdapter(HTTPAdapter): | ||
| """Custom HTTPAdapter that disables hostname verification for local self-signed certs.""" | ||
| def init_poolmanager(self, connections, maxsize, block=False, **pool_kwargs): | ||
| self.poolmanager = PoolManager( | ||
| num_pools=connections, | ||
| maxsize=maxsize, | ||
| block=block, | ||
| assert_hostname=False, | ||
| **pool_kwargs | ||
| ) |
There was a problem hiding this comment.
In urllib3 v2.0+, the assert_hostname parameter has been removed from PoolManager, which will cause a TypeError at runtime. Instead of overriding init_poolmanager, you can override cert_verify on the HTTPAdapter to disable hostname verification in a way that is compatible with both urllib3 1.x and 2.x.
class HostNameIgnoringAdapter(HTTPAdapter):
"""Custom HTTPAdapter that disables hostname verification for local self-signed certs."""
def cert_verify(self, conn, url, verify, cert):
super().cert_verify(conn, url, verify, cert)
conn.assert_hostname = False| from requests.adapters import HTTPAdapter | ||
| from urllib3.poolmanager import PoolManager |
There was a problem hiding this comment.
If the suggestion to use cert_verify in HostNameIgnoringAdapter is applied, PoolManager is no longer needed and its import can be removed.
| from requests.adapters import HTTPAdapter | |
| from urllib3.poolmanager import PoolManager | |
| from requests.adapters import HTTPAdapter |
| self._add_request_metadata(client_call_details) | ||
| response = continuation(client_call_details, request) | ||
| metadata = [(k, str(v)) for k, v in response.trailing_metadata()] | ||
| metadata = [(k, str(v)) for k, v in response.initial_metadata()] + [(k, str(v)) for k, v in response.trailing_metadata()] |
There was a problem hiding this comment.
In gRPC Python, initial_metadata() and trailing_metadata() can return None in certain circumstances (e.g., if the call failed or in mock/test environments). To prevent a TypeError when iterating or concatenating, add defensive None checks.
| metadata = [(k, str(v)) for k, v in response.initial_metadata()] + [(k, str(v)) for k, v in response.trailing_metadata()] | |
| initial_metadata = response.initial_metadata() or [] | |
| trailing_metadata = response.trailing_metadata() or [] | |
| metadata = [(k, str(v)) for k, v in initial_metadata] + [(k, str(v)) for k, v in trailing_metadata] |
References
- Specifically enforce defensive programming: for languages that support nullable references (e.g., Go, Python, Java), ensure appropriate null/nil/None checks or other language-idiomatic guards exist before object property accesses.
| dir = os.path.dirname(__file__) | ||
| cert_path = os.path.join(dir, "../cert/mtls.crt") | ||
| key_path = os.path.join(dir, "../cert/mtls.key") |
There was a problem hiding this comment.
Avoid using dir as a variable name because it shadows the Python built-in dir() function. Rename it to something more descriptive like current_dir or base_dir.
| dir = os.path.dirname(__file__) | |
| cert_path = os.path.join(dir, "../cert/mtls.crt") | |
| key_path = os.path.join(dir, "../cert/mtls.key") | |
| current_dir = os.path.dirname(__file__) | |
| cert_path = os.path.join(current_dir, "../cert/mtls.crt") | |
| key_path = os.path.join(current_dir, "../cert/mtls.key") |
| dir = os.path.dirname(__file__) | ||
| cert_path = os.path.join(dir, "../cert/mtls.crt") | ||
| key_path = os.path.join(dir, "../cert/mtls.key") |
There was a problem hiding this comment.
Avoid using dir as a variable name because it shadows the Python built-in dir() function. Rename it to something more descriptive like current_dir or base_dir.
| dir = os.path.dirname(__file__) | |
| cert_path = os.path.join(dir, "../cert/mtls.crt") | |
| key_path = os.path.join(dir, "../cert/mtls.key") | |
| current_dir = os.path.dirname(__file__) | |
| cert_path = os.path.join(current_dir, "../cert/mtls.crt") | |
| key_path = os.path.join(current_dir, "../cert/mtls.key") |
| for key, value in interceptor.response_metadata: | ||
| if key.lower() == "x-showcase-tls-group": | ||
| negotiated_group = value | ||
| elif key.lower() == "x-showcase-tls-client-supported-groups": | ||
| supported_groups = value |
There was a problem hiding this comment.
Accessing interceptor.response_metadata directly can raise an AttributeError or TypeError if the attribute is not set or is None (e.g., if the call failed or if the interceptor does not support it). Use getattr with a default value to safely access it.
| for key, value in interceptor.response_metadata: | |
| if key.lower() == "x-showcase-tls-group": | |
| negotiated_group = value | |
| elif key.lower() == "x-showcase-tls-client-supported-groups": | |
| supported_groups = value | |
| response_metadata = getattr(interceptor, "response_metadata", None) or [] | |
| for key, value in response_metadata: | |
| if key.lower() == "x-showcase-tls-group": | |
| negotiated_group = value | |
| elif key.lower() == "x-showcase-tls-client-supported-groups": | |
| supported_groups = value |
References
- Specifically enforce defensive programming: for languages that support nullable references (e.g., Go, Python, Java), ensure appropriate null/nil/None checks or other language-idiomatic guards exist before object property accesses.
1 participant
Test