From fa212f0faeb32f6a7cfd207a3d08a5ea72087ed4 Mon Sep 17 00:00:00 2001 From: Pierre-Olivier Mercier Date: Sun, 26 Apr 2026 16:39:22 +0700 Subject: [PATCH] Harden contract validation, STARTTLS edge cases, and rule output --- checker/fetch.go | 2 +- checker/prober.go | 4 ++-- checker/rules_protocol.go | 35 +++++++++++++++++++-------- checker/starttls_imap.go | 4 ++++ checker/starttls_smtp.go | 7 +++++- checker/starttls_xmpp.go | 50 ++++++++++++++++++++++++--------------- checker/types.go | 10 ++++---- contract/contract.go | 23 +++++++++++++++++- main.go | 8 +++++++ 9 files changed, 104 insertions(+), 39 deletions(-) diff --git a/checker/fetch.go b/checker/fetch.go index 1cdd816..1a078d1 100644 --- a/checker/fetch.go +++ b/checker/fetch.go @@ -68,7 +68,7 @@ func FetchChain(ctx context.Context, host string, port uint16, starttls string, tlsConn := tls.Client(conn, &tls.Config{ ServerName: host, - InsecureSkipVerify: true, + InsecureSkipVerify: true, // #nosec G402 -- intentional: caller receives the chain even when PKIX rejects it }) if err := tlsConn.HandshakeContext(dialCtx); err != nil { return nil, fmt.Errorf("tls handshake: %w", err) diff --git a/checker/prober.go b/checker/prober.go index 74a05ef..1aa25ee 100644 --- a/checker/prober.go +++ b/checker/prober.go @@ -172,7 +172,7 @@ func probe(ctx context.Context, ep contract.TLSEndpoint, timeout time.Duration) func handshake(conn net.Conn, ep contract.TLSEndpoint, sni string) (*tls.Conn, error) { cfg := &tls.Config{ ServerName: sni, - InsecureSkipVerify: true, + InsecureSkipVerify: true, // #nosec G402 -- intentional: chain verified separately in probe() } if ep.STARTTLS == "" { @@ -198,7 +198,7 @@ func handshake(conn net.Conn, ep contract.TLSEndpoint, sni string) (*tls.Conn, e } var ( - errStartTLSNotOffered = errors.New("starttls not advertised by server") + errStartTLSNotOffered = errors.New("starttls not advertised by server") errUnsupportedStartTLSProto = errors.New("unsupported starttls protocol") ) diff --git a/checker/rules_protocol.go b/checker/rules_protocol.go index f9e24ad..6153b38 100644 --- a/checker/rules_protocol.go +++ b/checker/rules_protocol.go @@ -4,6 +4,8 @@ import ( "context" "crypto/tls" "fmt" + "sort" + "strings" sdk "git.happydns.org/checker-sdk-go/checker" ) @@ -81,25 +83,38 @@ func (r *cipherSuiteRule) Evaluate(ctx context.Context, obs sdk.ObservationGette return []sdk.CheckState{emptyCaseState("tls.cipher_suite.no_endpoints")} } - var out []sdk.CheckState + // Collapse per-endpoint cipher suites into a single info state. One + // row per endpoint drowns out actionable rules in the UI on domains + // with many endpoints; an aggregated list is enough for visibility. + suites := map[string]int{} + endpoints := map[string][]string{} for _, ref := range sortedRefs(data) { p := data.Probes[ref] if p.CipherSuite == "" { continue } - out = append(out, sdk.CheckState{ - Status: sdk.StatusInfo, - Code: "tls.cipher_suite.negotiated", - Subject: subjectOf(p), - Message: fmt.Sprintf("Cipher suite %s negotiated.", p.CipherSuite), - Meta: metaOf(p), - }) + suites[p.CipherSuite]++ + endpoints[p.CipherSuite] = append(endpoints[p.CipherSuite], p.Endpoint) } - if len(out) == 0 { + if len(suites) == 0 { return []sdk.CheckState{unknownState( "tls.cipher_suite.skipped", "No endpoint completed a TLS handshake.", )} } - return out + names := make([]string, 0, len(suites)) + for s := range suites { + names = append(names, s) + } + sort.Strings(names) + parts := make([]string, 0, len(names)) + for _, n := range names { + parts = append(parts, fmt.Sprintf("%s (%d)", n, suites[n])) + } + return []sdk.CheckState{{ + Status: sdk.StatusInfo, + Code: "tls.cipher_suite.negotiated", + Message: "Negotiated cipher suites: " + strings.Join(parts, ", "), + Meta: map[string]any{"suites": endpoints}, + }} } diff --git a/checker/starttls_imap.go b/checker/starttls_imap.go index 777e38d..4c04010 100644 --- a/checker/starttls_imap.go +++ b/checker/starttls_imap.go @@ -36,6 +36,10 @@ func starttlsIMAP(conn net.Conn, sni string) error { supportsSTARTTLS = true } if strings.HasPrefix(line, "A001 ") { + rest := strings.TrimSpace(line[len("A001 "):]) + if !strings.HasPrefix(strings.ToUpper(rest), "OK") { + return fmt.Errorf("CAPABILITY rejected by server: %s", rest) + } break } } diff --git a/checker/starttls_smtp.go b/checker/starttls_smtp.go index dfbaa19..ccc0211 100644 --- a/checker/starttls_smtp.go +++ b/checker/starttls_smtp.go @@ -7,6 +7,11 @@ import ( "strings" ) +// EHLOHostname is the hostname sent in the SMTP EHLO command during STARTTLS +// negotiation. Override it at startup (e.g. via -ldflags or programmatically) +// to match the identity of the host running the checker. +var EHLOHostname = "checker.localhost" + func init() { registerStartTLS("smtp", starttlsSMTP) registerStartTLS("submission", starttlsSMTP) @@ -20,7 +25,7 @@ func starttlsSMTP(conn net.Conn, sni string) error { return fmt.Errorf("read greeting: %w", err) } - if _, err := rw.WriteString("EHLO checker.happydomain.org\r\n"); err != nil { + if _, err := fmt.Fprintf(rw, "EHLO %s\r\n", EHLOHostname); err != nil { return fmt.Errorf("write ehlo: %w", err) } if err := rw.Flush(); err != nil { diff --git a/checker/starttls_xmpp.go b/checker/starttls_xmpp.go index dfed8f2..22b421a 100644 --- a/checker/starttls_xmpp.go +++ b/checker/starttls_xmpp.go @@ -31,6 +31,9 @@ func starttlsXMPP(conn net.Conn, sni, ns string) error { dec := xml.NewDecoder(conn) // Read the inbound opening and its . + // A peer that opens with (or anything other than features) + // is not going to advertise STARTTLS: surface that immediately rather + // than spinning on tokens until the deadline fires. hasStartTLS := false outer: for { @@ -38,29 +41,38 @@ outer: if err != nil { return fmt.Errorf("read stream features: %w", err) } - if se, ok := tok.(xml.StartElement); ok { - if se.Name.Local == "features" { - // Scan features children. - for { - t2, err := dec.Token() - if err != nil { - return fmt.Errorf("read features body: %w", err) + se, ok := tok.(xml.StartElement) + if !ok { + continue + } + switch se.Name.Local { + case "stream": + // Outer opening. Continue reading children. + continue + case "features": + for { + t2, err := dec.Token() + if err != nil { + return fmt.Errorf("read features body: %w", err) + } + switch ee := t2.(type) { + case xml.StartElement: + if ee.Name.Local == "starttls" { + hasStartTLS = true } - switch ee := t2.(type) { - case xml.StartElement: - if ee.Name.Local == "starttls" { - hasStartTLS = true - } - if err := dec.Skip(); err != nil { - return fmt.Errorf("skip feature %q: %w", ee.Name.Local, err) - } - case xml.EndElement: - if ee.Name.Local == "features" { - break outer - } + if err := dec.Skip(); err != nil { + return fmt.Errorf("skip feature %q: %w", ee.Name.Local, err) + } + case xml.EndElement: + if ee.Name.Local == "features" { + break outer } } } + case "error": + return fmt.Errorf("server returned before features") + default: + return fmt.Errorf("%w: unexpected element %q before features", errStartTLSNotOffered, se.Name.Local) } } if !hasStartTLS { diff --git a/checker/types.go b/checker/types.go index 0dfd8b3..8cbf23d 100644 --- a/checker/types.go +++ b/checker/types.go @@ -78,11 +78,11 @@ type TLSProbe struct { // no certificate. NoPeerCert bool `json:"no_peer_cert,omitempty"` - HostnameMatch *bool `json:"hostname_match,omitempty"` - ChainValid *bool `json:"chain_valid,omitempty"` - ChainVerifyErr string `json:"chain_verify_err,omitempty"` - NotAfter time.Time `json:"not_after,omitempty"` - Issuer string `json:"issuer,omitempty"` + HostnameMatch *bool `json:"hostname_match,omitempty"` + ChainValid *bool `json:"chain_valid,omitempty"` + ChainVerifyErr string `json:"chain_verify_err,omitempty"` + NotAfter time.Time `json:"not_after,omitempty"` + Issuer string `json:"issuer,omitempty"` // IssuerDN is the leaf's issuer as an RFC 2253 DN string, suitable for // matching the CCADB CAA Identifiers CSV "Subject" column when the AKI // lookup misses. diff --git a/contract/contract.go b/contract/contract.go index 52f1be1..76f7e07 100644 --- a/contract/contract.go +++ b/contract/contract.go @@ -16,6 +16,7 @@ import ( "encoding/hex" "encoding/json" "fmt" + "strings" sdk "git.happydns.org/checker-sdk-go/checker" ) @@ -58,10 +59,27 @@ type TLSEndpoint struct { RequireSTARTTLS bool `json:"require,omitempty"` } +// Validate rejects endpoints that cannot be probed: empty Host or zero Port. +// STARTTLS dialect is intentionally not checked here (the checker surfaces +// unsupported dialects at runtime via the tls.starttls_dialect_supported +// rule), and SNI defaults to Host downstream. +func (ep TLSEndpoint) Validate() error { + if strings.TrimSpace(strings.TrimSuffix(ep.Host, ".")) == "" { + return fmt.Errorf("contract: TLSEndpoint.Host is required") + } + if ep.Port == 0 { + return fmt.Errorf("contract: TLSEndpoint.Port must be 1-65535") + } + return nil +} + // NewEntry wraps ep in an sdk.DiscoveryEntry with Type, a deterministic Ref // derived from ep, and a marshaled Payload. The returned entry can be // returned as-is from a DiscoveryPublisher implementation. func NewEntry(ep TLSEndpoint) (sdk.DiscoveryEntry, error) { + if err := ep.Validate(); err != nil { + return sdk.DiscoveryEntry{}, err + } payload, err := json.Marshal(ep) if err != nil { return sdk.DiscoveryEntry{}, fmt.Errorf("contract: marshal TLSEndpoint: %w", err) @@ -95,7 +113,7 @@ func Ref(ep TLSEndpoint) string { req = "1" } canonical := fmt.Sprintf("%s|%d|%s|%s|%s", ep.Host, ep.Port, sni, ep.STARTTLS, req) - sum := sha1.Sum([]byte(canonical)) + sum := sha1.Sum([]byte(canonical)) // #nosec G401 G505 -- non-cryptographic stable key; see doc comment above return hex.EncodeToString(sum[:8]) } @@ -109,6 +127,9 @@ func ParseEntry(e sdk.DiscoveryEntry) (TLSEndpoint, error) { if err := json.Unmarshal(e.Payload, &ep); err != nil { return TLSEndpoint{}, fmt.Errorf("contract: unmarshal TLSEndpoint: %w", err) } + if err := ep.Validate(); err != nil { + return TLSEndpoint{}, err + } return ep, nil } diff --git a/main.go b/main.go index ae5167c..c967401 100644 --- a/main.go +++ b/main.go @@ -10,11 +10,19 @@ import ( var Version = "custom-build" +// EHLOHostname is set via -ldflags to identify this checker instance in SMTP +// EHLO greetings. Falls back to the package default ("checker.localhost") when +// left empty. +var EHLOHostname = "" + var listenAddr = flag.String("listen", ":8080", "HTTP listen address") func main() { flag.Parse() tls.Version = Version + if EHLOHostname != "" { + tls.EHLOHostname = EHLOHostname + } srv := server.New(tls.Provider()) if err := srv.ListenAndServe(*listenAddr); err != nil {