From ce3b0e582ae87c2da95c5673af71233cd59127a6 Mon Sep 17 00:00:00 2001
From: Tobias Gesellchen <tobias@gesellix.de>
Date: Tue, 19 May 2026 23:16:36 +0200
Subject: [PATCH] fix(health): drop InsecureSkipVerify from cert-chain probe
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CodeQL alert 147 flagged the Phase-2 re-dial with
InsecureSkipVerify=true, used to read the served leaf after
Phase 1's strict verification failed.

The leaf is already reachable without a second connection:
tls.CertificateVerificationError carries
UnverifiedCertificates, and the three x509.* verification-
error types each carry the offending Cert. errors.As over
those covers darwin (Security.framework) and linux
(crypto/x509) consistently.

Same three classifier outcomes
(leafFromOwnCA/leafSubjectEqualsIssuer/leafForeign), same
chainContext rendering — the classifier reads only the leaf,
which is byte-identical to the Phase-2 peers[0]. Removes the
only InsecureSkipVerify literal in the tree.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 pkg/service/health/checks_cert_chain.go | 74 ++++++++++++++++++-------
 1 file changed, 53 insertions(+), 21 deletions(-)

diff --git a/pkg/service/health/checks_cert_chain.go b/pkg/service/health/checks_cert_chain.go
index 1cd2006..d27ddc4 100644
--- a/pkg/service/health/checks_cert_chain.go
+++ b/pkg/service/health/checks_cert_chain.go
@@ -3,6 +3,7 @@ package health
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"errors"
 	"fmt"
 	"net"
 	"net/url"
@@ -72,31 +73,21 @@ func runCertChainCheck(httpsURL string, caCertFn func() *x509.Certificate) []Fin
 		return nil // validates against system roots
 	}
 
-	// Phase 2: re-dial with InsecureSkipVerify so we can read the
-	// chain and report what was actually served.
-	insecureConn, insecureErr := tls.DialWithDialer(dialer, "tcp", addr, &tls.Config{
-		ServerName:         host,
-		InsecureSkipVerify: true,
-		MinVersion:         tls.VersionTLS12,
-	})
-	if insecureErr != nil {
+	// Phase 2: extract the leaf cert from Phase 1's verification
+	// error. crypto/x509 attaches the offending certificate to the
+	// three verification-failure error types, which lets us inspect
+	// what the server actually served without ever opening a second
+	// connection with InsecureSkipVerify. If the error is something
+	// else (timeout, TCP reset, protocol mismatch), report it as a
+	// reachability problem.
+	leaf := leafFromVerifyError(err)
+	if leaf == nil {
 		return []Finding{{
 			Severity: SeverityError,
-			Message:  fmt.Sprintf("Could not connect to %s: %v", addr, insecureErr),
-			Details:  "AfterTouch's HTTPS endpoint isn't reachable from inside the service. Check that the listener is bound and the URL host:port resolves correctly.",
+			Message:  fmt.Sprintf("Could not connect to %s: %v", addr, err),
+			Details:  "AfterTouch's HTTPS endpoint isn't reachable from inside the service, or the peer dropped the handshake before presenting a certificate. Check that the listener is bound and the URL host:port resolves correctly.",
 		}}
 	}
-	defer func() { _ = insecureConn.Close() }()
-
-	peers := insecureConn.ConnectionState().PeerCertificates
-	if len(peers) == 0 {
-		return []Finding{{
-			Severity: SeverityWarning,
-			Message:  "HTTPS endpoint connected but presented no certificates.",
-		}}
-	}
-
-	leaf := peers[0]
 
 	dnsNames := strings.Join(leaf.DNSNames, ", ")
 	if dnsNames == "" {
@@ -173,6 +164,47 @@ func splitHTTPSHostPort(raw string) (string, string) {
 	return host, port
 }
 
+// leafFromVerifyError extracts the offending certificate that
+// crypto/tls attaches to a verification-failure error. Returns
+// nil for non-verification errors (network unreachable, timeout,
+// TLS-level handshake failure before any cert was presented).
+//
+// This is the substitute for an InsecureSkipVerify re-dial: the
+// same leaf is reachable from the failed strict-dial error
+// without ever standing up a connection that disables cert
+// validation.
+//
+// tls.CertificateVerificationError carries
+// UnverifiedCertificates across platforms (the underlying error
+// is platform-specific — on darwin it comes from
+// Security.framework, on linux from crypto/x509 — but the
+// wrapping type is the same). The x509.* unwraps are a
+// defensive fallback for the case where a caller hands us a
+// verification error that bypassed the tls layer.
+func leafFromVerifyError(err error) *x509.Certificate {
+	var certErr *tls.CertificateVerificationError
+	if errors.As(err, &certErr) && len(certErr.UnverifiedCertificates) > 0 {
+		return certErr.UnverifiedCertificates[0]
+	}
+
+	var unkAuth x509.UnknownAuthorityError
+	if errors.As(err, &unkAuth) {
+		return unkAuth.Cert
+	}
+
+	var hostnameErr x509.HostnameError
+	if errors.As(err, &hostnameErr) {
+		return hostnameErr.Certificate
+	}
+
+	var invalidErr x509.CertificateInvalidError
+	if errors.As(err, &invalidErr) {
+		return invalidErr.Cert
+	}
+
+	return nil
+}
+
 // leafClassification labels how the leaf relates to AfterTouch's
 // own CA. Drives the install-ca-vs-openssl suggestion branch.
 type leafClassification int