diff --git a/cmd/soundtouch-service/main.go b/cmd/soundtouch-service/main.go index 6cdbdeb..663f86a 100644 --- a/cmd/soundtouch-service/main.go +++ b/cmd/soundtouch-service/main.go @@ -519,6 +519,7 @@ func main() { sm.GetDNSRunning = server.GetDNSRunning server.SetLogBuffer(logBuf) server.SetHTTPServerURL(config.httpsServerURL) + server.SetHTTPSListenAddr(config.httpsAddr) server.SetExpectedHosts(config.domains) server.SetVersionInfo(version, commit, date, repoURL) server.SetDiscoverySettings(config.discoveryInterval, config.discoveryEnabled) diff --git a/pkg/service/handlers/server.go b/pkg/service/handlers/server.go index 7361c66..a42e5e8 100644 --- a/pkg/service/handlers/server.go +++ b/pkg/service/handlers/server.go @@ -41,6 +41,7 @@ type Server struct { mu sync.RWMutex serverURL string httpsServerURL string + httpsListenAddr string discovering bool redactLogs bool logBodies bool @@ -149,6 +150,7 @@ func NewServer(ds *datastore.DataStore, sm *setup.Manager, serverURL string, red _, httpsURL := s.GetSettings() return httpsURL }, + s.actualHTTPSPort, s.loadOwnCACert, ) health.RegisterCACertExpiryCheck(s.healthRegistry, s.loadOwnCACert, s.ownCACertPath) @@ -801,6 +803,35 @@ func (s *Server) SetHTTPServerURL(url string) { s.httpsServerURL = url } +// SetHTTPSListenAddr records the address the HTTPS listener is bound +// to (e.g. ":8443"). The cert-chain health check uses its port to +// detect an advertised-URL/listener port mismatch (issue #355). +func (s *Server) SetHTTPSListenAddr(addr string) { + s.mu.Lock() + defer s.mu.Unlock() + + s.httpsListenAddr = addr +} + +// actualHTTPSPort returns the port the HTTPS listener is bound to, or +// "" if unknown/unparseable. +func (s *Server) actualHTTPSPort() string { + s.mu.RLock() + addr := s.httpsListenAddr + s.mu.RUnlock() + + if addr == "" { + return "" + } + + _, port, err := net.SplitHostPort(addr) + if err != nil { + return "" + } + + return port +} + // SetRecorder sets the recorder for the server. func (s *Server) SetRecorder(r *proxy.Recorder) { s.mu.Lock() diff --git a/pkg/service/health/checks_cert_chain.go b/pkg/service/health/checks_cert_chain.go index 6568a69..fde20bb 100644 --- a/pkg/service/health/checks_cert_chain.go +++ b/pkg/service/health/checks_cert_chain.go @@ -29,27 +29,40 @@ const CheckIDCertChain = "service_cert_chain" // something else → warning with an `openssl s_client` // investigation prompt (foreign chain / reverse proxy / // ingress cert). -// - endpoint not reachable from inside the service (no cert -// presented) → warning, not error: the advertised HTTPS URL -// is often intentionally unreachable from the service itself -// (reverse proxy, Docker-published port, LAN-only hostname). +// - endpoint not reachable AND the advertised URL's port differs +// from the actual listener port → warning naming both ports +// and the fix (issue #355: the advertised URL, invisible in +// the web UI, defaulted to 443 while the listener was on 8443). +// - endpoint not reachable with no port mismatch → warning, not +// error: the advertised HTTPS URL is often intentionally +// unreachable from the service itself (reverse proxy, +// Docker-published port, LAN-only hostname). // - HTTPS URL not configured → skip silently. // // 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) { +// actualHTTPSPortFn returns the port the HTTPS listener is actually +// bound to (e.g. "8443"), or "" if unknown. It lets the check +// distinguish a genuine "advertised URL points at the wrong port" +// misconfiguration from an intentionally-unreachable advertised URL. +func RegisterCertChainCheck(r *Registry, httpsURLFn, actualHTTPSPortFn func() string, caCertFn func() *x509.Certificate) { r.Register(Check{ ID: CheckIDCertChain, Title: "HTTPS endpoint TLS configuration", Run: func() []Finding { - return runCertChainCheck(httpsURLFn(), caCertFn) + actualPort := "" + if actualHTTPSPortFn != nil { + actualPort = actualHTTPSPortFn() + } + + return runCertChainCheck(httpsURLFn(), actualPort, caCertFn) }, }) } -func runCertChainCheck(httpsURL string, caCertFn func() *x509.Certificate) []Finding { +func runCertChainCheck(httpsURL, actualHTTPSPort string, caCertFn func() *x509.Certificate) []Finding { if strings.TrimSpace(httpsURL) == "" { return nil } @@ -87,12 +100,39 @@ func runCertChainCheck(httpsURL string, caCertFn func() *x509.Certificate) []Fin leaf := leafFromVerifyError(err) if leaf == nil { // The dial failed before any certificate was presented - // (connection refused, timeout, handshake reset). From - // inside the service we can't tell "the endpoint is down" - // apart from "the advertised HTTPS URL simply isn't - // reachable from here" — the latter is a normal, healthy - // setup (TLS terminated by a reverse proxy, or a - // Docker-published port / external hostname that only + // (connection refused, timeout, handshake reset). + // + // Special-case the misconfiguration behind issue #355: the + // HTTPS URL the service advertises (and points speakers at) + // carries a different port than the one the listener is + // actually bound to. This is easy to hit because the URL + // comes from --https-server-url / HTTPS_SERVER_URL / the + // settings file and, when it omits the port, defaults to + // 443 here — while the listener stays on --https-port + // (default 8443). The URL isn't editable in the web UI, so + // the mismatch is otherwise invisible. Call it out with the + // fix. (A reachable endpoint never gets here — a working + // reverse proxy on the advertised port dials fine above.) + if actualHTTPSPort != "" && port != actualHTTPSPort { + return []Finding{{ + Severity: SeverityWarning, + Message: fmt.Sprintf("Configured HTTPS URL %s uses port %s, but the service is listening on port %s and nothing answered on port %s.", httpsURL, port, actualHTTPSPort, port), + Details: fmt.Sprintf("Speakers are pointed at the configured HTTPS URL, so if its port doesn't match the listener they connect to the wrong place. This URL is set via --https-server-url / HTTPS_SERVER_URL (or the persisted settings file) and isn't editable in the web UI, which makes the mismatch easy to miss. Set it to include the listener's port, e.g. https://%s:%s. If a reverse proxy intentionally forwards port %s to %s, this is expected and can be ignored. Underlying error: %v.", + host, actualHTTPSPort, port, actualHTTPSPort, err), + ManualCommands: []ManualCommand{{ + Label: "Point the HTTPS URL at the listener's port (restart required):", + Command: fmt.Sprintf("HTTPS_SERVER_URL=https://%s:%s", host, actualHTTPSPort), + Hint: "Or pass --https-server-url. Skip this if a reverse proxy already maps the ports.", + }}, + }} + } + + // Otherwise: a reachability failure with no port mismatch to + // point at. From inside the service we can't tell "the + // endpoint is down" apart from "the advertised HTTPS URL + // simply isn't reachable from here" — the latter is a + // normal, healthy setup (TLS terminated by a reverse proxy, + // or a Docker-published port / external hostname that only // resolves on the LAN). Reporting a hard error there is a // false alarm (issue #355), so this is a warning with the // context to tell the two apart. diff --git a/pkg/service/health/checks_cert_chain_test.go b/pkg/service/health/checks_cert_chain_test.go index 3d70daa..029b57b 100644 --- a/pkg/service/health/checks_cert_chain_test.go +++ b/pkg/service/health/checks_cert_chain_test.go @@ -16,19 +16,40 @@ import ( ) func TestCertChain_EmptyURLSkips(t *testing.T) { - got := runCertChainCheck("", nil) + 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", nil) + got := runCertChainCheck("://nope", "", nil) if len(got) != 1 || got[0].Severity != SeverityWarning { t.Fatalf("expected one warning, got %+v", got) } } +func TestCertChain_PortMismatch_NamesBothPortsAndFix(t *testing.T) { + // Regression for issue #355: the advertised HTTPS URL carries a + // different port (here the 443 default, from a port-less URL) + // than the actual listener (8443). When that endpoint is also + // unreachable, the warning must name both ports and offer the + // corrected HTTPS_SERVER_URL, since the URL isn't editable in + // the web UI. 127.0.0.1:443 is unreachable in the test sandbox. + got := runCertChainCheck("https://127.0.0.1/", "8443", nil) + if len(got) != 1 || got[0].Severity != SeverityWarning { + t.Fatalf("expected one warning for port mismatch, got %+v", got) + } + + if !strings.Contains(got[0].Message, "443") || !strings.Contains(got[0].Message, "8443") { + t.Errorf("expected message to name both the advertised (443) and listener (8443) ports, got %q", got[0].Message) + } + + if len(got[0].ManualCommands) == 0 || !strings.Contains(got[0].ManualCommands[0].Command, "https://127.0.0.1:8443") { + t.Errorf("expected a corrected HTTPS_SERVER_URL suggestion, got %+v", got[0].ManualCommands) + } +} + func TestCertChain_UnreachableEndpoint_IsWarningNotError(t *testing.T) { // Regression for issue #355: the service dialing its own // configured HTTPS URL and finding it unreachable is NOT a hard @@ -39,7 +60,7 @@ func TestCertChain_UnreachableEndpoint_IsWarningNotError(t *testing.T) { // explains the expected case and offers a client-side check. // // 127.0.0.1:1 refuses; using https:// to force the TLS path. - got := runCertChainCheck("https://127.0.0.1:1/", nil) + got := runCertChainCheck("https://127.0.0.1:1/", "", nil) if len(got) != 1 || got[0].Severity != SeverityWarning { t.Fatalf("expected one warning for unreachable endpoint, got %+v", got) } @@ -60,7 +81,7 @@ func TestCertChain_SelfSigned_SubjectEqualsIssuerFallback(t *testing.T) { // No CA provided → fallback to Subject==Issuer heuristic. // This is informational, not a warning — a self-signed // AfterTouch chain is the expected default deployment shape. - got := runCertChainCheck(srv.URL, nil) + got := runCertChainCheck(srv.URL, "", nil) if len(got) != 1 || got[0].Severity != SeverityInfo { t.Fatalf("expected one info finding for self-signed cert, got %+v", got) } @@ -111,7 +132,7 @@ func TestCertChain_LeafSignedByOwnCA_IsInformationalNotAWarning(t *testing.T) { srv.StartTLS() defer srv.Close() - got := runCertChainCheck(srv.URL, func() *x509.Certificate { return ca }) + got := runCertChainCheck(srv.URL, "", func() *x509.Certificate { return ca }) if len(got) != 1 { t.Fatalf("expected one finding, got %+v", got) } @@ -160,7 +181,7 @@ func TestCertChain_ForeignChain_SuggestsOpenSSL(t *testing.T) { // Different CA — pretend it's "our" AfterTouch CA. _, ourCA := generateInternalCA(t) - got := runCertChainCheck(srv.URL, func() *x509.Certificate { return ourCA }) + 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) }