fix(ENGHLP-1703): keep TLS when DIODE_SKIP_TLS_VERIFY is set - #114
jajeffries wants to merge 10 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 |
There was a problem hiding this comment.
Request changes
The is_plaintext / tls_verify split is right and grpcs:// no longer downgrades to plaintext. Keep that. The skip-verify path still cannot connect in the case this PR is meant to fix. Reproduced on this head against a real gRPC server.
Blocker: hostname verification is still enforced
After a CERT_NONE handshake getpeercert() returns {}, so _tls_server_name_from_peercert() always falls back to the target host and gRPC checks the pinned cert against that host.
Repro: gRPC server with a self-signed cert (SAN diode.internal), target 127.0.0.1:<port>, skip_tls_verify=True:
UNAVAILABLE: failed to connect to all addresses; last error: ... Custom verification check failed with error: UNAUTHENTICATED: Hostname Verification Check failed.
The same pinned leaf with grpc.ssl_target_name_override=diode.internal connects. Fix the name extraction:
- Decode the name from the fetched cert without
_test_decode_cert. A second probe handshake withCERT_REQUIRED,cadata=<leaf PEM>,verify_flags |= VERIFY_X509_PARTIAL_CHAIN,check_hostname=Falsereturns the decoded dict (verified for self-signed and CA-signed leaves). Or take acryptographydependency. - Override precedence: first DNS SAN, else an IP SAN, else CN only when the cert has no SANs. gRPC ignores CN whenever a SAN is present, and an IP-looking override must match an IP SAN exactly.
- Add a test against a real TLS server (
grpc.server+ssl_server_credentials) whose cert name does not match the target. Every skip-verify test today patches_fetch_peer_leaf_certificate, so CI never runs the probe.
Proxy path
- Authenticated proxies. gRPC honours
http://user:pw@proxyand sendsProxy-Authorization: Basic …. The probe sends nothing, gets 407, andDiodeClient()raises. Add the header from the URL userinfo. https://proxy. grpc-core rejects non-httpproxy schemes ('https' scheme not supported in proxy URI) and connects direct, so the waiver rationale does not hold: the probe would tunnel through a proxy the channel never uses. Rejecthttps://in_validate_proxy_url.- IPv6. The CONNECT line becomes
CONNECT ::1:443; use the bracketed authority.grpcs://[::1]with no port raises a bareValueErrorin_authority_host_port.
Pinning is not InsecureSkipVerify; document the gaps
- The pin is captured once at construction. Traefik regenerates its default cert on restart, so a long-lived client fails with
CERTIFICATE_VERIFY_FAILEDuntil recreated. - gRPC sends the override as SNI. The channel handshakes with SNI=cert name while the probe used SNI=host, so SNI-based cert selection (or load-balanced backends with distinct leaves) can serve a cert that was never pinned.
DIODE_CERT_FILEis ignored under skip-verify, for both gRPC and the OAuth session. Fine as a hard override, but say so in the README and drop the Go parity claim.
Nit: .gitignore adds .worktrees/ twice.
Edited 2026-09-21: removed the claim that gRPC rejects a pinned CA:FALSE leaf (BoringSSL accepts it; only Python's ssl needs VERIFY_X509_PARTIAL_CHAIN), fixed the IPv6 wording, added the live repro and the proxy-auth / SNI findings.
🤖 Generated with Claude Code
Stale: superseded by the changes-requested review on the same head.
Probe with a partial-chain second handshake so SAN/CN drive grpc.ssl_target_name_override instead of the dialed host, and add a live gRPC server test for mismatched SAN. Co-authored-by: Cursor <cursoragent@cursor.com>
Send Proxy-Authorization from URL userinfo, reject https:// proxies, and use bracketed IPv6 CONNECT targets plus default ports for grpcs://[::1]. Co-authored-by: Cursor <cursoragent@cursor.com>
Clarify leaf pinning, SNI override behavior, and cert_file when TLS verification is disabled, and remove the duplicate .worktrees/ gitignore entry. Co-authored-by: Cursor <cursoragent@cursor.com>
4 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