From 1f8dab572ca9681464fdadc65bfb5f250fc496c3 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 12 Mar 2025 16:33:03 -0600 Subject: [PATCH] caddytls: Don't publish ECH configs if other records don't exist Publishing a DNS record for a name that doesn't have any could make wildcards ineffective, which would be surprising for site owners and could lead to downtime. --- modules/caddyhttp/autohttps.go | 17 ++++++++++----- modules/caddytls/ech.go | 38 ++++++++++++++++++++++++++-------- modules/caddytls/tls.go | 2 +- 3 files changed, 42 insertions(+), 15 deletions(-) diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go index dce21a721..769cfd4ef 100644 --- a/modules/caddyhttp/autohttps.go +++ b/modules/caddyhttp/autohttps.go @@ -163,6 +163,7 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er } } + // trim the list of domains covered by wildcards, if configured if srv.AutoHTTPS.PreferWildcard { wildcards := make(map[string]struct{}) for d := range serverDomainSet { @@ -184,6 +185,17 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er } } + // build the list of domains that could be used with ECH (if enabled) + // so the TLS app can know to publish ECH configs for them; we do this + // after trimming domains covered by wildcards because, presumably, + // if the user wants to use wildcard certs, they also want to use the + // wildcard for ECH, rather than individual subdomains + echDomains := make([]string, 0, len(serverDomainSet)) + for d := range serverDomainSet { + echDomains = append(echDomains, d) + } + app.tlsApp.RegisterServerNames(echDomains) + // nothing more to do here if there are no domains that qualify for // automatic HTTPS and there are no explicit TLS connection policies: // if there is at least one domain but no TLS conn policy (F&&T), we'll @@ -205,7 +217,6 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er // for all the hostnames we found, filter them so we have // a deduplicated list of names for which to obtain certs // (only if cert management not disabled for this server) - var echDomains []string if srv.AutoHTTPS.DisableCerts { logger.Warn("skipping automated certificate management for server because it is disabled", zap.String("server_name", srvName)) } else { @@ -232,14 +243,10 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er } uniqueDomainsForCerts[d] = struct{}{} - echDomains = append(echDomains, d) } } } - // let the TLS server know we have some hostnames that could be protected behind ECH - app.tlsApp.RegisterServerNames(echDomains) - // tell the server to use TLS if it is not already doing so if srv.TLSConnPolicies == nil { srv.TLSConnPolicies = caddytls.ConnectionPolicies{new(caddytls.ConnectionPolicy)} diff --git a/modules/caddytls/ech.go b/modules/caddytls/ech.go index 63b872260..a192c3390 100644 --- a/modules/caddytls/ech.go +++ b/modules/caddytls/ech.go @@ -639,8 +639,16 @@ func (dnsPub *ECHDNSPublisher) PublishECHConfigList(ctx context.Context, innerNa continue } - // get any existing HTTPS record for this domain, and augment - // our ech SvcParamKey with any other existing SvcParams + relName := libdns.RelativeName(domain+".", zone) + // TODO: libdns.RelativeName should probably return "@" instead of "". (The latest commits of libdns do this, so remove this logic once upgraded.) + if relName == "" { + relName = "@" + } + + // get existing records for this domain; we need to make sure another + // record exists for it so we don't accidentally trample a wildcard; we + // also want to get any HTTPS record that may already exist for it so + // we can augment the ech SvcParamKey with any other existing SvcParams recs, err := dnsPub.provider.GetRecords(ctx, zone) if err != nil { dnsPub.logger.Error("unable to get existing DNS records to publish ECH data to HTTPS DNS record", @@ -648,17 +656,29 @@ func (dnsPub *ECHDNSPublisher) PublishECHConfigList(ctx context.Context, innerNa zap.Error(err)) continue } - relName := libdns.RelativeName(domain+".", zone) - // TODO: libdns.RelativeName should probably return "@" instead of "". - if relName == "" { - relName = "@" - } var httpsRec libdns.Record + var nameHasExistingRecord bool for _, rec := range recs { - if rec.Name == relName && rec.Type == "HTTPS" && (rec.Target == "" || rec.Target == ".") { - httpsRec = rec + if rec.Name == relName { + nameHasExistingRecord = true + if rec.Type == "HTTPS" && (rec.Target == "" || rec.Target == ".") { + httpsRec = rec + break + } } } + if !nameHasExistingRecord { + // Turns out if you publish a DNS record for a name that doesn't have any DNS record yet, + // any wildcard records won't apply for the name anymore, meaning if a wildcard A/AAAA record + // is used to resolve the domain to a server, publishing an HTTPS record could break resolution! + // In theory, this should be a non-issue, at least for A/AAAA records, if the HTTPS record + // includes ipv[4|6]hint SvcParamKeys, + dnsPub.logger.Warn("domain does not have any existing records, so skipping publication of HTTPS record", + zap.String("domain", domain), + zap.String("relative_name", relName), + zap.String("zone", zone)) + continue + } params := make(svcParams) if httpsRec.Value != "" { params, err = parseSvcParams(httpsRec.Value) diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go index 3b04b8785..089794efb 100644 --- a/modules/caddytls/tls.go +++ b/modules/caddytls/tls.go @@ -563,7 +563,7 @@ func (t *TLS) Manage(names []string) error { // (keeping only the hotsname) and filters IP addresses, which can't be // used with ECH. // -// EXPERIMENTAL: This function and its behavior are subject to change. +// EXPERIMENTAL: This function and its semantics/behavior are subject to change. func (t *TLS) RegisterServerNames(dnsNames []string) { t.serverNamesMu.Lock() for _, name := range dnsNames {