fix(ENGHLP-1703): keep TLS when DIODE_SKIP_TLS_VERIFY is set - #114
jajeffries wants to merge 7 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||
|
AI Code Review — risk tier: Advisory only. A human owns the merge decision. Summary — Net positive. This head reverts the nested-TLS wrap on the proxy CONNECT socket, fixing the high-severity bug that broke the skip-verify probe entirely when combined with an Resolved since last review
Previously raised — waived, not re-raised
🤖 AI Code Review · run · prompts: Previous review · 2026-09-18 · 46c67ddAI Code Review — risk tier: Advisory only. A human owns the merge decision. Summary — Net positive overall: this head fixes the two low-severity items from the last review (bracketed IPv6-literal probe targets, plaintext CONNECT to an Findings
Resolved since last review
🤖 AI Code Review · run · prompts: Previous review · 2026-09-18 · 0c40e2eAI Code Review — risk tier: Advisory only. A human owns the merge decision. Summary — Net positive: this follow-up commit wraps the remaining unwrapped Findings
Resolved since last review
🤖 AI Code Review · run · prompts: Previous review · 2026-09-18 · ec6399fAI Code Review — risk tier: Advisory only. A human owns the merge decision. Summary — Net positive: the skip-verify probe now pins TLS 1.2 minimum (fixing the CodeQL flag) and keeps the gRPC channel encrypted instead of falling back to plaintext. One of the three previously-raised correctness issues is only partially addressed — the TLS handshake step in the probe can still leak a raw, unwrapped exception on a transient network failure. Previously raised — still open
Resolved since last review
🤖 AI Code Review · run · prompts: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Probe up to three times and pin all distinct peer leaf certs for gRPC skip-verify, wrap connect/TLS failures in DiodeConfigError, derive SNI override from getpeercert(), and require TLS 1.2+ on the probe handshake. Co-authored-by: Cursor <cursoragent@cursor.com>
AI review follow-up (pushed
|
Inline PROTOCOL_TLS_CLIENT setup at wrap_socket so CodeQL sees TLS 1.2+ on the peer-cert probe handshake (addresses PR review discussion r4046196340). Co-authored-by: Cursor <cursoragent@cursor.com>
|
/ai-review |
Widen _fetch_peer_leaf_certificate handshake handling to catch OSError alongside ssl.SSLError so transient network failures during skip-verify probes surface as DiodeConfigError instead of raw exceptions. Co-authored-by: Cursor <cursoragent@cursor.com>
AI review follow-up (open finding: unwrapped TLS probe errors)Pushed fix for the remaining Medium item on Verification: /ai-review |
Parse bracketed IPv6 authorities for the peer-cert probe and wrap HTTPS_PROXY CONNECT in TLS before sending the tunnel request. Co-authored-by: Cursor <cursoragent@cursor.com>
AI review follow-up (low findings on
|
Use PROTOCOL_TLS_CLIENT with minimum_version TLSv1_2 when wrapping the skip-verify probe's HTTPS_PROXY socket so CodeQL matches the peer handshake path. Co-authored-by: Cursor <cursoragent@cursor.com>
The peer-cert probe issues CONNECT over a plain TCP socket to the proxy, matching grpc.http_proxy, so the target TLS handshake is not stacked on an existing proxy SSLSocket. Co-authored-by: Cursor <cursoragent@cursor.com>
AI review follow-up (nested TLS on
|
|
🔁 AI Code Review updated for |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 197c0b75a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| f"No peer certificate returned from {authority}" | ||
| ) | ||
| pem = ssl.DER_cert_to_PEM_cert(der_cert).encode() | ||
| server_name = _tls_server_name_from_peercert(tls_sock.getpeercert(), host) |
There was a problem hiding this comment.
When skip verification is used against a server whose certificate name differs from the target, such as a default Traefik self-signed certificate, verify_mode = ssl.CERT_NONE causes the non-binary SSLSocket.getpeercert() call to return an empty dictionary. This line therefore falls back to the target host, and gRPC still checks that hostname against the pinned certificate, causing the channel to fail even though certificate verification was disabled. Decode the already-fetched DER/PEM certificate or use credentials that genuinely disable hostname verification.
Useful? React with 👍 / 👎.
| for _ in range(_SKIP_VERIFY_PEER_PROBE_ATTEMPTS): | ||
| try: | ||
| pem, server_name = _fetch_peer_leaf_certificate(authority, proxy_url) |
There was a problem hiding this comment.
When the target resolves to multiple backends with independently signed leaf certificates, these three probes can repeatedly select only a subset of the backends. The subsequent grpc.secure_channel connection may then reach an unsampled backend whose certificate is absent from root_certificates, producing CERTIFICATE_VERIFY_FAILED despite skip-verify being enabled. A fixed number of preflight probes cannot reliably emulate disabled verification; use a stable issuer or a channel credential mechanism that does not require sampling peer leaves.
Useful? React with 👍 / 👎.
| connect_request = ( | ||
| f"CONNECT {host}:{port} HTTP/1.1\r\n" | ||
| f"Host: {host}:{port}\r\n\r\n" | ||
| ) | ||
| sock.sendall(connect_request.encode()) |
There was a problem hiding this comment.
When the configured proxy URL itself uses https://, this code opens a raw TCP socket and immediately sends a plaintext HTTP CONNECT request. An HTTPS proxy expects a TLS handshake first, so the skip-verify certificate probe fails before the gRPC channel can be created, even though _validate_proxy_url accepts HTTPS proxy URLs. Either establish a TLS transport to the proxy before tunneling or reject this unsupported proxy scheme.
Useful? React with 👍 / 👎.
|
P1: I don't think this fully matches Go's In context.check_hostname = False
context.verify_mode = ssl.CERT_NONE
tls_sock = context.wrap_socket(raw_sock, server_hostname=host)
der_cert = tls_sock.getpeercert(binary_form=True)
server_name = _tls_server_name_from_peercert(
tls_sock.getpeercert(),
host,
)The problem is that Python returns an empty decoded dict from We then pin the peer leaf cert as a trusted root and set: ("grpc.ssl_target_name_override", host)which means gRPC can still reject the connection if the certificate SAN/CN doesn't match the target hostname/IP. For example: Go's Can we add an integration test with a real self-signed TLS server where the target hostname deliberately does not match the cert SAN, and make sure |
3 participants
Summary
DIODE_SKIP_TLS_VERIFY: the connection stays ongrpcs/httpsand only certificate validation is skipped.h2c) and surface as 404 or protocol errors.What changed
parse_targetnow returns separateis_plaintextandtls_verifyflags instead of treating skip-verify as plaintext._open_grpc_channeluses TLS credentials with an optional peer certificate pin when verification is disabled; auth token URL scheme followsis_plaintext.skip_tls_verifyconstructor parameter and documentedDIODE_SKIP_TLS_VERIFYin the README.How tested
pytest tests/test_client.py -q(133 passed)ruff check netboxlabs/diode/sdk/client.py tests/test_client.pyLinear
Made with Cursor