fix(health): classify self-signed leaves via real CA signature check

The Subject==Issuer heuristic for "this is AfterTouch's self-signed cert" misses the common case: AfterTouch's internal CA has CN="SoundTouch Local Root CA" while leaves it issues have CN="soundtouch" — different Subject and Issuer strings, so the classifier was falling through to "foreign chain" and suggesting openssl s_client when install-ca was actually the right fix. Replace the heuristic with a definitive check: load AfterTouch's own CA leaf via setup.Manager.Crypto.GetCACertPath() and call x509.Certificate.CheckSignatureFrom(ca). When that succeeds we *know* the leaf came from our own CA. The Subject==Issuer heuristic stays as a fallback for environments where the CA isn't loadable (with a clarifying note in the hint). Server.loadOwnCACert caches the parsed CA via sync.Once so repeated Health polls don't re-read the PEM. Fixes the case shown in soundtouch.fritz.box deployments where Subject=CN=soundtouch,O=AfterTouch and Issuer=CN=SoundTouch Local Root CA,O=SoundTouch Local Service confused the classifier. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-08-19 00:56:16 +00:00 · 2026-05-19 23:20:40 +02:00
parent 147a69d1c3
commit c90fdf234f
3 changed files with 257 additions and 39 deletions
@@ -3,12 +3,15 @@ package handlers
 import (
 	"bytes"
 	"context"
+	"crypto/x509"
+	"encoding/pem"
 	"errors"
 	"fmt"
 	"log"
 	"net"
 	"net/http"
 	"net/url"
+	"os"
 	"strconv"
 	"strings"
 	"sync"
@@ -67,6 +70,10 @@ type Server struct {
 	healthRegistry      *health.Registry
 	logBuf              *logbuf.Buffer
 	expectedHosts       []string
+	ownCACache          struct {
+		once sync.Once
+		cert *x509.Certificate
+	}
 }

 // RequestSnapshot represents an immutable snapshot of an HTTP request.
@@ -109,10 +116,14 @@ func NewServer(ds *datastore.DataStore, sm *setup.Manager, serverURL string, red
 	health.RegisterSpeakerInfoReachable(s.healthRegistry, ds)
 	health.RegisterSourcesXMLDiff(s.healthRegistry, ds)
 	health.RegisterSpeakerMargeURLCheck(s.healthRegistry, ds, s.ExpectedHosts)
-	health.RegisterCertChainCheck(s.healthRegistry, func() string {
-		_, httpsURL := s.GetSettings()
-		return httpsURL
-	})
+	health.RegisterCertChainCheck(
+		s.healthRegistry,
+		func() string {
+			_, httpsURL := s.GetSettings()
+			return httpsURL
+		},
+		s.loadOwnCACert,
+	)
 	health.RegisterTestPlaybackCheck(s.healthRegistry, ds, func() string {
 		serverURL, _ := s.GetSettings()
 		return serverURL
@@ -163,6 +174,49 @@ func (s *Server) ExpectedHosts() []string {
 	return out
 }

+// loadOwnCACert parses AfterTouch's own CA leaf from disk. Used
+// by the Health-tab cert-chain check to definitively classify
+// whether the HTTPS endpoint is serving a cert issued by this
+// service's built-in CA (as opposed to a public CA or a foreign
+// chain from a reverse proxy). Returns nil when the CA isn't
+// configured or fails to parse — the caller falls back to a
+// Subject==Issuer heuristic in that case.
+//
+// The parse is cached in ownCACache so repeated Health polls
+// don't re-read the PEM. Restart-based config changes are
+// picked up because Server itself is reconstructed.
+func (s *Server) loadOwnCACert() *x509.Certificate {
+	s.ownCACache.once.Do(func() {
+		if s.sm == nil || s.sm.Crypto == nil {
+			return
+		}
+
+		path := s.sm.Crypto.GetCACertPath()
+		if path == "" {
+			return
+		}
+
+		data, err := os.ReadFile(path)
+		if err != nil {
+			return
+		}
+
+		block, _ := pem.Decode(data)
+		if block == nil {
+			return
+		}
+
+		cert, err := x509.ParseCertificate(block.Bytes)
+		if err != nil {
+			return
+		}
+
+		s.ownCACache.cert = cert
+	})
+
+	return s.ownCACache.cert
+}
+
 // TrustedRealIPMiddleware returns a chi middleware that rewrites
 // r.RemoteAddr from X-Real-IP / X-Forwarded-For / True-Client-IP, but only
 // when the immediate TCP peer is in the configured trusted-proxy list.
@@ -20,25 +20,31 @@ const CheckIDCertChain = "service_cert_chain"
 //   - validates against system roots → no finding (the
 //     speaker's firmware ships with the major roots, so a public-
 //     CA chain such as Let's Encrypt is usable directly).
-//   - chain doesn't validate → warning, with a note that
-//     `install-ca` is the fix when AfterTouch is using its own
-//     self-signed CA, or "review the proxy / ingress cert" when
-//     the chain looks foreign.
+//   - chain doesn't validate but the served leaf was issued by
+//     our own AfterTouch CA → warning with an `install-ca`
+//     suggestion (definitive: we checked the signature against
+//     our CA, not a Subject==Issuer heuristic).
+//   - chain doesn't validate and the served leaf was issued by
+//     something else → warning with an `openssl s_client`
+//     investigation prompt (foreign chain / reverse proxy /
+//     ingress cert).
 //   - HTTPS URL not configured → skip silently.
 //
-// httpsURLFn is a closure so config changes are picked up at run
-// time (today only at restart, but cheap to keep flexible).
-func RegisterCertChainCheck(r *Registry, httpsURLFn func() string) {
+// caCertFn returns AfterTouch's own CA leaf certificate (nil if
+// unavailable). It's called per check run; the handler-side
+// implementation caches the parse via sync.Once so we don't
+// re-read the PEM on every poll.
+func RegisterCertChainCheck(r *Registry, httpsURLFn func() string, caCertFn func() *x509.Certificate) {
 	r.Register(Check{
 		ID:    CheckIDCertChain,
 		Title: "HTTPS endpoint certificate validates",
 		Run: func() []Finding {
-			return runCertChainCheck(httpsURLFn())
+			return runCertChainCheck(httpsURLFn(), caCertFn)
 		},
 	})
 }

-func runCertChainCheck(httpsURL string) []Finding {
+func runCertChainCheck(httpsURL string, caCertFn func() *x509.Certificate) []Finding {
 	if strings.TrimSpace(httpsURL) == "" {
 		return nil
 	}
@@ -108,17 +114,25 @@ func runCertChainCheck(httpsURL string) []Finding {

 	var hints []ManualCommand

-	if leafLooksSelfSigned(leaf) {
+	classification := classifyLeaf(leaf, caCertFn)
+	switch classification {
+	case leafFromOwnCA:
 		hints = append(hints, ManualCommand{
-			Label:   "If this is AfterTouch's built-in CA, install it on each speaker:",
+			Label:   "Install AfterTouch's CA on each speaker:",
 			Command: "soundtouch-cli --host=<speaker-ip> setup install-ca --service-url=" + httpsURL,
-			Hint:    "Requires SSH on the speaker. After install, re-run this check.",
+			Hint:    "The served leaf was issued by AfterTouch's own CA (verified by signature). Requires SSH on the speaker. After install, re-run this check.",
 		})
-	} else {
+	case leafSubjectEqualsIssuer:
+		hints = append(hints, ManualCommand{
+			Label:   "If this is a self-signed cert from AfterTouch, install its CA on each speaker:",
+			Command: "soundtouch-cli --host=<speaker-ip> setup install-ca --service-url=" + httpsURL,
+			Hint:    "Heuristic match (Subject == Issuer) — AfterTouch's own CA wasn't loadable, so this is a best guess. If wrong, treat the chain as foreign.",
+		})
+	default:
 		hints = append(hints, ManualCommand{
 			Label:   "Investigate the chain manually:",
 			Command: fmt.Sprintf("openssl s_client -connect %s -servername %s -showcerts </dev/null", addr, host),
-			Hint:    "Run from the same host as the service. Shows the full chain the peer is serving.",
+			Hint:    "Run from the same host as the service. Shows the full chain the peer is serving — likely a reverse proxy or ingress cert.",
 		})
 	}

@@ -146,14 +160,36 @@ func splitHTTPSHostPort(raw string) (string, string) {
 	return host, port
 }

-// leafLooksSelfSigned reports whether the leaf certificate's
-// Subject and Issuer match — a strong hint that we're looking at
-// AfterTouch's own self-signed CA-issued cert rather than a public
-// CA chain. This is intentionally a heuristic, not a guarantee.
-func leafLooksSelfSigned(leaf *x509.Certificate) bool {
+// leafClassification labels how the leaf relates to AfterTouch's
+// own CA. Drives the install-ca-vs-openssl suggestion branch.
+type leafClassification int
+
+const (
+	leafForeign             leafClassification = iota // chain we don't recognise
+	leafFromOwnCA                                     // signature verified by AfterTouch's CA
+	leafSubjectEqualsIssuer                           // fallback heuristic when CA isn't loadable
+)
+
+// classifyLeaf returns leafFromOwnCA when caCertFn returns a CA
+// cert that signed `leaf` (verified by CheckSignatureFrom). When
+// the CA cert isn't available, falls back to the
+// Subject==Issuer heuristic. Anything else is leafForeign.
+func classifyLeaf(leaf *x509.Certificate, caCertFn func() *x509.Certificate) leafClassification {
 	if leaf == nil {
-		return false
+		return leafForeign
 	}

-	return leaf.Subject.String() == leaf.Issuer.String()
+	if caCertFn != nil {
+		if ca := caCertFn(); ca != nil {
+			if err := leaf.CheckSignatureFrom(ca); err == nil {
+				return leafFromOwnCA
+			}
+		}
+	}
+
+	if leaf.Subject.String() == leaf.Issuer.String() {
+		return leafSubjectEqualsIssuer
+	}
+
+	return leafForeign
 }
@@ -16,14 +16,14 @@ import (
 )

 func TestCertChain_EmptyURLSkips(t *testing.T) {
-	got := runCertChainCheck("")
+	got := runCertChainCheck("", nil)
 	if len(got) != 0 {
 		t.Errorf("expected no findings for empty URL, got %+v", got)
 	}
 }

 func TestCertChain_UnparseableURLWarns(t *testing.T) {
-	got := runCertChainCheck("://nope")
+	got := runCertChainCheck("://nope", nil)
 	if len(got) != 1 || got[0].Severity != SeverityWarning {
 		t.Fatalf("expected one warning, got %+v", got)
 	}
@@ -31,20 +31,18 @@ func TestCertChain_UnparseableURLWarns(t *testing.T) {

 func TestCertChain_UnreachableEndpoint(t *testing.T) {
 	// 127.0.0.1:1 refuses; using https:// to force TLS path.
-	got := runCertChainCheck("https://127.0.0.1:1/")
+	got := runCertChainCheck("https://127.0.0.1:1/", nil)
 	if len(got) != 1 || got[0].Severity != SeverityError {
 		t.Fatalf("expected one error for unreachable endpoint, got %+v", got)
 	}
 }

-func TestCertChain_SelfSignedFlagsAndSuggestsInstallCA(t *testing.T) {
+func TestCertChain_SelfSigned_SubjectEqualsIssuerFallback(t *testing.T) {
 	srv := newSelfSignedTLSServer(t)
 	defer srv.Close()

-	// httptest's TLS URL uses 127.0.0.1; the test cert below uses
-	// "127.0.0.1" as SAN, so SNI matches but the chain is
-	// self-signed and won't validate against system roots.
-	got := runCertChainCheck(srv.URL)
+	// No CA provided → fallback to Subject==Issuer heuristic.
+	got := runCertChainCheck(srv.URL, nil)
 	if len(got) != 1 || got[0].Severity != SeverityWarning {
 		t.Fatalf("expected one warning for self-signed cert, got %+v", got)
 	}
@@ -54,12 +52,79 @@ func TestCertChain_SelfSignedFlagsAndSuggestsInstallCA(t *testing.T) {
 	}

 	if len(got[0].ManualCommands) == 0 {
-		t.Fatalf("expected at least one manual command, got none")
+		t.Fatalf("expected at least one manual command")
 	}

 	cmd := got[0].ManualCommands[0].Command
 	if !strings.Contains(cmd, "install-ca") {
-		t.Errorf("expected install-ca suggestion for self-signed cert, got %q", cmd)
+		t.Errorf("expected install-ca suggestion via Subject==Issuer heuristic, got %q", cmd)
+	}
+
+	hint := got[0].ManualCommands[0].Hint
+	if !strings.Contains(hint, "Heuristic") {
+		t.Errorf("expected hint to disclose the heuristic match, got %q", hint)
+	}
+}
+
+func TestCertChain_LeafSignedByOwnCA_PrefersInstallCA(t *testing.T) {
+	// Construct a CA + leaf signed by it. Leaf has SAN 127.0.0.1
+	// so SNI works; Subject != Issuer (different CommonNames),
+	// which would have fooled the old heuristic.
+	caTLS, ca := generateInternalCA(t)
+	leafTLS := generateLeafSignedBy(t, ca, caTLS.PrivateKey)
+
+	srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(200)
+	}))
+	srv.TLS = &tls.Config{Certificates: []tls.Certificate{leafTLS}}
+	srv.StartTLS()
+	defer srv.Close()
+
+	got := runCertChainCheck(srv.URL, func() *x509.Certificate { return ca })
+	if len(got) != 1 || got[0].Severity != SeverityWarning {
+		t.Fatalf("expected one warning, got %+v", got)
+	}
+
+	if len(got[0].ManualCommands) == 0 {
+		t.Fatalf("expected a manual command")
+	}
+
+	cmd := got[0].ManualCommands[0]
+	if !strings.Contains(cmd.Command, "install-ca") {
+		t.Errorf("expected install-ca, got %q", cmd.Command)
+	}
+
+	if !strings.Contains(cmd.Hint, "verified by signature") {
+		t.Errorf("expected signature-verified hint, got %q", cmd.Hint)
+	}
+}
+
+func TestCertChain_ForeignChain_SuggestsOpenSSL(t *testing.T) {
+	// Build an "external" CA + leaf, then provide a *different*
+	// CA via caCertFn. Signature check fails → classifier returns
+	// leafForeign → openssl suggestion (since Subject==Issuer
+	// would also fail for a properly chained leaf).
+	externalCATLS, externalCA := generateInternalCA(t)
+	leafTLS := generateLeafSignedBy(t, externalCA, externalCATLS.PrivateKey)
+
+	srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(200)
+	}))
+	srv.TLS = &tls.Config{Certificates: []tls.Certificate{leafTLS}}
+	srv.StartTLS()
+	defer srv.Close()
+
+	// Different CA — pretend it's "our" AfterTouch CA.
+	_, ourCA := generateInternalCA(t)
+
+	got := runCertChainCheck(srv.URL, func() *x509.Certificate { return ourCA })
+	if len(got) != 1 || got[0].Severity != SeverityWarning {
+		t.Fatalf("expected one warning, got %+v", got)
+	}
+
+	cmd := got[0].ManualCommands[0]
+	if !strings.Contains(cmd.Command, "openssl s_client") {
+		t.Errorf("expected openssl suggestion for foreign chain, got %q", cmd.Command)
 	}
 }

@@ -104,10 +169,6 @@ func newSelfSignedTLSServer(t *testing.T) *httptest.Server {
 func generateSelfSignedCert(t *testing.T) tls.Certificate {
 	t.Helper()

-	// Use ecdsa via x509 helpers — but keep it simple with a tiny
-	// RSA key from the test. Actually use crypto/rand + ed25519
-	// would be cleaner; for parity with stdlib examples, use the
-	// built-in helper path.
 	template := &x509.Certificate{
 		SerialNumber: big.NewInt(1),
 		Subject:      pkix.Name{CommonName: "aftertouch-test"},
@@ -135,3 +196,70 @@ func generateSelfSignedCert(t *testing.T) tls.Certificate {
 		PrivateKey:  key,
 	}
 }
+
+// generateInternalCA returns a self-signed CA suitable for
+// signing leaves. The returned tls.Certificate carries the CA
+// key (needed to sign leaves below); the *x509.Certificate is
+// the parsed CA leaf.
+func generateInternalCA(t *testing.T) (tls.Certificate, *x509.Certificate) {
+	t.Helper()
+
+	template := &x509.Certificate{
+		SerialNumber:          big.NewInt(2026),
+		Subject:               pkix.Name{CommonName: "AfterTouch Test CA", Organization: []string{"AfterTouch Test"}},
+		NotBefore:             time.Now().Add(-time.Hour),
+		NotAfter:              time.Now().Add(24 * time.Hour),
+		IsCA:                  true,
+		BasicConstraintsValid: true,
+		KeyUsage:              x509.KeyUsageCertSign | x509.KeyUsageDigitalSignature,
+	}
+	template.Issuer = template.Subject
+
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("rsa key: %v", err)
+	}
+
+	derBytes, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key)
+	if err != nil {
+		t.Fatalf("create CA cert: %v", err)
+	}
+
+	caParsed, err := x509.ParseCertificate(derBytes)
+	if err != nil {
+		t.Fatalf("parse CA cert: %v", err)
+	}
+
+	return tls.Certificate{Certificate: [][]byte{derBytes}, PrivateKey: key}, caParsed
+}
+
+// generateLeafSignedBy issues a TLS leaf cert (CN=leaf) signed
+// by ca/caKey, with SAN 127.0.0.1 so httptest's loopback SNI
+// matches. Subject != Issuer by construction — the case that
+// caught my old heuristic.
+func generateLeafSignedBy(t *testing.T, ca *x509.Certificate, caKey any) tls.Certificate {
+	t.Helper()
+
+	leafKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		t.Fatalf("leaf rsa key: %v", err)
+	}
+
+	template := &x509.Certificate{
+		SerialNumber: big.NewInt(42),
+		Subject:      pkix.Name{CommonName: "soundtouch", Organization: []string{"AfterTouch"}},
+		NotBefore:    time.Now().Add(-time.Hour),
+		NotAfter:     time.Now().Add(time.Hour),
+		IPAddresses:  []net.IP{net.ParseIP("127.0.0.1")},
+		DNSNames:     []string{"127.0.0.1"},
+		KeyUsage:     x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment,
+		ExtKeyUsage:  []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+	}
+
+	derBytes, err := x509.CreateCertificate(rand.Reader, template, ca, &leafKey.PublicKey, caKey)
+	if err != nil {
+		t.Fatalf("create leaf cert: %v", err)
+	}
+
+	return tls.Certificate{Certificate: [][]byte{derBytes}, PrivateKey: leafKey}
+}