fix: async httpx TLS pinning via wrap_bio interception#61
Conversation
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tinfoil/client.py">
<violation number="1" location="src/tinfoil/client.py:100">
P2: `TLSBoundHTTPSHandler._get_connection` (lines 55-66) contains the same fingerprint verification logic that `_verify_peer_fingerprint` now encapsulates. Consider updating `_get_connection` to call `SecureClient._verify_peer_fingerprint(cert_binary, self.expected_pubkey)` to eliminate the remaining duplication and ensure all verification paths stay in sync.</violation>
</file>
<file name="tests/test_verification_failures.py">
<violation number="1" location="tests/test_verification_failures.py:417">
P2: This assertion doesn’t prove wrap_bio was monkey-patched because bound method objects are never identical to the class function; the test will pass even when no patch occurred. Check for an instance-level override (e.g., presence in `__dict__`) or compare the underlying function to make the test meaningful.</violation>
</file>
<file name="tests/test_attestation_flow.py">
<violation number="1" location="tests/test_attestation_flow.py:134">
P3: The async pinning rejection test is overly permissive; it will pass on any network failure. Assert the specific fingerprint-mismatch error so the test only passes when TLS pinning actually rejects the cert.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ctx.wrap_socket = wrap_socket | ||
| return httpx.Client(verify=ctx, follow_redirects=True) | ||
| @staticmethod | ||
| def _verify_peer_fingerprint(cert_binary: Optional[bytes], expected_fp: str) -> None: |
There was a problem hiding this comment.
P2: TLSBoundHTTPSHandler._get_connection (lines 55-66) contains the same fingerprint verification logic that _verify_peer_fingerprint now encapsulates. Consider updating _get_connection to call SecureClient._verify_peer_fingerprint(cert_binary, self.expected_pubkey) to eliminate the remaining duplication and ensure all verification paths stay in sync.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tinfoil/client.py, line 100:
<comment>`TLSBoundHTTPSHandler._get_connection` (lines 55-66) contains the same fingerprint verification logic that `_verify_peer_fingerprint` now encapsulates. Consider updating `_get_connection` to call `SecureClient._verify_peer_fingerprint(cert_binary, self.expected_pubkey)` to eliminate the remaining duplication and ensure all verification paths stay in sync.</comment>
<file context>
@@ -96,16 +96,18 @@ def ground_truth(self) -> Optional[GroundTruth]:
- ctx.wrap_socket = wrap_socket
- return httpx.Client(verify=ctx, follow_redirects=True)
+ @staticmethod
+ def _verify_peer_fingerprint(cert_binary: Optional[bytes], expected_fp: str) -> None:
+ """Verify that a certificate's public key fingerprint matches the expected value."""
+ if not cert_binary:
</file context>
| # The underlying SSL context's wrap_bio should no longer be the | ||
| # original C-level method — it should be our pinned_wrap_bio closure. | ||
| ssl_ctx = self._get_ssl_context(async_http) | ||
| assert ssl_ctx.wrap_bio is not ssl.SSLContext.wrap_bio |
There was a problem hiding this comment.
P2: This assertion doesn’t prove wrap_bio was monkey-patched because bound method objects are never identical to the class function; the test will pass even when no patch occurred. Check for an instance-level override (e.g., presence in __dict__) or compare the underlying function to make the test meaningful.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_verification_failures.py, line 417:
<comment>This assertion doesn’t prove wrap_bio was monkey-patched because bound method objects are never identical to the class function; the test will pass even when no patch occurred. Check for an instance-level override (e.g., presence in `__dict__`) or compare the underlying function to make the test meaningful.</comment>
<file context>
@@ -294,5 +296,213 @@ def test_snp_measurement_mismatch_raises(self, mock_fetch):
+ # The underlying SSL context's wrap_bio should no longer be the
+ # original C-level method — it should be our pinned_wrap_bio closure.
+ ssl_ctx = self._get_ssl_context(async_http)
+ assert ssl_ctx.wrap_bio is not ssl.SSLContext.wrap_bio
+
+ def test_do_handshake_verifies_fingerprint_match(self):
</file context>
| assert ssl_ctx.wrap_bio is not ssl.SSLContext.wrap_bio | |
| assert "wrap_bio" in ssl_ctx.__dict__ |
| http_client = client.make_secure_http_client() | ||
|
|
||
| try: | ||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
P3: The async pinning rejection test is overly permissive; it will pass on any network failure. Assert the specific fingerprint-mismatch error so the test only passes when TLS pinning actually rejects the cert.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_attestation_flow.py, line 134:
<comment>The async pinning rejection test is overly permissive; it will pass on any network failure. Assert the specific fingerprint-mismatch error so the test only passes when TLS pinning actually rejects the cert.</comment>
<file context>
@@ -93,5 +93,70 @@ def test_secure_http_client():
+ http_client = client.make_secure_http_client()
+
+ try:
+ with pytest.raises(Exception):
+ http_client.get("https://google.com")
+ finally:
</file context>
Summary by cubic
Fixes async TLS pinning in httpx. We now intercept wrap_bio and verify the server’s public-key fingerprint after the TLS handshake, matching the sync client and blocking mismatched hosts.
Bug Fixes
Refactors
Written for commit a8a901e. Summary will update on new commits.