From 797c1cf9f0244275cf70aa9bf8cf0d25e7d2aba6 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Sun, 23 Feb 2025 10:44:17 +0100 Subject: [PATCH] do not log an error for tls requests with ipv6 addresses as sni server name ip addresses are invalid in server names. for ipv6 addresses, the autocert.GetCertificate calls would return an error, which we logged, and increased a metric about. but the alerts for this situation aren't helpful. so recognize ip addresses early. if we are lenient about unknown server names (for incoming smtp deliveries), we switch to the fallback hostname, otherwise we return an error. this was the error logged: l=error m="requesting certificate" err="acme/autocert: server name component count invalid" for ipv4 addresses, the name wouldn't be in our allowlist and should already have caused us to switch to the fallback hostname. --- autotls/autotls.go | 42 ++++++++++++++++++++++++++++++++++-------- rfc/index.txt | 3 +++ 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/autotls/autotls.go b/autotls/autotls.go index f120b9b..e711952 100644 --- a/autotls/autotls.go +++ b/autotls/autotls.go @@ -193,21 +193,41 @@ func Load(name, acmeDir, contactEmail, directoryURL string, eabKeyID string, eab // optionally falling back to a certificate for fallbackHostname in case SNI is // absent or for an unknown hostname. func (m *Manager) loggingGetCertificate(hello *tls.ClientHelloInfo, fallbackHostname dns.Domain, fallbackNoSNI, fallbackUnknownSNI bool) (*tls.Certificate, error) { - log := mlog.New("autotls", nil).WithContext(hello.Context()) + log := mlog.New("autotls", nil).WithContext(hello.Context()).With( + slog.Any("localaddr", hello.Conn.LocalAddr()), + slog.Any("supportedprotos", hello.SupportedProtos), + slog.String("servername", hello.ServerName), + ) // If we can't find a certificate (depending on fallback parameters), we return a // nil certificate and nil error, which crypto/tls turns into a TLS alert // "unrecognized name", which can be interpreted by clients as a hint that they are - // using the wrong hostname, or a certificate is missing. + // using the wrong hostname, or a certificate is missing. ../rfc/9325:578 + + // IP addresses for ServerName are not allowed, but happen in practice. If we + // should be lenient (fallbackUnknownSNI), we switch to the fallback hostname, + // otherwise we return an error. We don't want to pass IP addresses to + // GetCertificate because it will return an error for IPv6 addresses. + // ../rfc/6066:367 ../rfc/4366:535 + if net.ParseIP(hello.ServerName) != nil { + if fallbackUnknownSNI { + hello.ServerName = fallbackHostname.ASCII + log = log.With(slog.String("servername", hello.ServerName)) + } else { + log.Debug("tls request with ip for server name, rejecting") + return nil, fmt.Errorf("invalid ip address for sni server name") + } + } if hello.ServerName == "" && fallbackNoSNI { hello.ServerName = fallbackHostname.ASCII + log = log.With(slog.String("servername", hello.ServerName)) } // Handle missing SNI to prevent logging an error below. if hello.ServerName == "" { metricMissingServerName.Inc() - log.Debug("tls request without sni servername, rejecting", slog.Any("localaddr", hello.Conn.LocalAddr()), slog.Any("supportedprotos", hello.SupportedProtos)) + log.Debug("tls request without sni server name, rejecting") return nil, nil } @@ -215,23 +235,29 @@ func (m *Manager) loggingGetCertificate(hello *tls.ClientHelloInfo, fallbackHost if err != nil && errors.Is(err, errHostNotAllowed) { if !fallbackUnknownSNI { metricUnknownServerName.Inc() - log.Debugx("requesting certificate", err, slog.String("host", hello.ServerName)) + log.Debugx("requesting certificate", err) return nil, nil } - log.Debug("certificate for unknown hostname, using fallback hostname", slog.String("host", hello.ServerName)) + // Some legitimate email deliveries over SMTP use an unknown SNI, e.g. a bare + // domain instead of the MX hostname. We "should" return an error, but that would + // break email delivery, so we use the fallback name if it is configured. + // ../rfc/9325:589 + + log = log.With(slog.String("servername", hello.ServerName)) + log.Debug("certificate for unknown hostname, using fallback hostname") hello.ServerName = fallbackHostname.ASCII cert, err = m.Manager.GetCertificate(hello) if err != nil { metricCertRequestErrors.Inc() - log.Errorx("requesting certificate for fallback hostname", err, slog.String("host", hello.ServerName)) + log.Errorx("requesting certificate for fallback hostname", err) } else { - log.Debug("using certificate for fallback hostname", slog.String("host", hello.ServerName)) + log.Debug("using certificate for fallback hostname") } return cert, err } else if err != nil { metricCertRequestErrors.Inc() - log.Errorx("requesting certificate", err, slog.String("host", hello.ServerName)) + log.Errorx("requesting certificate", err) } return cert, err } diff --git a/rfc/index.txt b/rfc/index.txt index 378892c..027b645 100644 --- a/rfc/index.txt +++ b/rfc/index.txt @@ -365,9 +365,11 @@ See implementation guide, https://jmap.io/server.html 8616 Yes - Email Authentication for Internationalized Mail # TLS +4366 - Obs (RFC 6066) Transport Layer Security (TLS) Extensions 5056 Yes - On the Use of Channel Bindings to Secure Channels 5705 Yes - Keying Material Exporters for Transport Layer Security (TLS) 5929 Yes - Channel Bindings for TLS +6066 - - Transport Layer Security (TLS) Extensions: Extension Definitions 6125 -? - Representation and Verification of Domain-Based Application Service Identity within Internet Public Key Infrastructure Using X.509 (PKIX) Certificates in the Context of Transport Layer Security (TLS) 7250 -No - Using Raw Public Keys in Transport Layer Security (TLS) and Datagram Transport Layer Security (DTLS) 7525 -? - Recommendations for Secure Use of Transport Layer Security (TLS) and Datagram Transport Layer Security (DTLS) @@ -377,6 +379,7 @@ See implementation guide, https://jmap.io/server.html 8996 Yes - Deprecating TLS 1.0 and TLS 1.1 8997 Yes - Deprecation of TLS 1.1 for Email Submission and Access 9266 Yes - Channel Bindings for TLS 1.3 +9325 -? - Recommendations for Secure Use of Transport Layer Security (TLS) and Datagram Transport Layer Security (DTLS) # ACME 8555 Yes - Automatic Certificate Management Environment (ACME)