From 77dd12cc785990c5c5da947b4e883029ab8bd552 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 30 Jun 2025 19:58:16 -0400 Subject: [PATCH] httpcaddyfile: Validates TLS DNS challenge options (#7099) * httpcaddyfile: Validates TLS DNS challenge options Adds validation to the TLS Caddyfile adapter to ensure that when DNS challenge options (such as propagation_delay or dns_ttl) are specified, a DNS provider is also configured. Adds new integration tests to verify this validation logic, and implements a new mechanism for adapt tests to assert a config adapt error. * Add some more AI-generated tests asserting config errors * Parallel doesn't work here, we use global variables * Windows fix --- caddyconfig/caddyfile/lexer.go | 3 +- caddyconfig/httpcaddyfile/builtins.go | 20 +++++++ .../ambiguous_site_definition.caddyfiletest | 12 ++++ ...ite_definition_duplicate_key.caddyfiletest | 9 +++ .../directive_as_site_address.caddyfiletest | 5 ++ ...cate_listener_address_global.caddyfiletest | 12 ++++ .../heredoc_extra_indentation.caddyfiletest | 41 +++++++++++++ .../heredoc_incomplete.caddyfiletest | 9 +++ .../heredoc_invalid_marker.caddyfiletest | 9 +++ ...eredoc_mismatched_whitespace.caddyfiletest | 10 ++++ .../heredoc_missing_marker.caddyfiletest | 9 +++ ...edoc_too_many_angle_brackets.caddyfiletest | 9 +++ .../import_cycle.caddyfiletest | 12 ++++ ...invoke_undefined_named_route.caddyfiletest | 5 ++ .../matcher_outside_site_block.caddyfiletest | 9 +++ .../site_address_invalid_port.caddyfiletest | 7 +++ .../site_address_negative_port.caddyfiletest | 7 +++ ...e_address_unsupported_scheme.caddyfiletest | 7 +++ ...ite_address_wss_invalid_port.caddyfiletest | 7 +++ .../site_address_wss_scheme.caddyfiletest | 7 +++ ...ple_options_without_provider.caddyfiletest | 9 +++ ...ion_timeout_without_provider.caddyfiletest | 7 +++ ...propagation_without_provider.caddyfiletest | 7 +++ .../caddyfile_adapt/tls_dns_ttl.caddyfiletest | 4 ++ .../tls_propagation_options.caddyfiletest | 6 +- caddytest/integration/caddyfile_adapt_test.go | 59 ++++++++++++------- 26 files changed, 279 insertions(+), 22 deletions(-) create mode 100644 caddytest/integration/caddyfile_adapt/ambiguous_site_definition.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/ambiguous_site_definition_duplicate_key.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/directive_as_site_address.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/duplicate_listener_address_global.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/heredoc_extra_indentation.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/heredoc_incomplete.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/heredoc_invalid_marker.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/heredoc_mismatched_whitespace.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/heredoc_missing_marker.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/heredoc_too_many_angle_brackets.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/import_cycle.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/invoke_undefined_named_route.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/matcher_outside_site_block.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/site_address_invalid_port.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/site_address_negative_port.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/site_address_unsupported_scheme.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/site_address_wss_invalid_port.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/site_address_wss_scheme.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/tls_dns_multiple_options_without_provider.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/tls_dns_propagation_timeout_without_provider.caddyfiletest create mode 100644 caddytest/integration/caddyfile_adapt/tls_dns_propagation_without_provider.caddyfiletest diff --git a/caddyconfig/caddyfile/lexer.go b/caddyconfig/caddyfile/lexer.go index a3b1fc7db..60dabe43d 100644 --- a/caddyconfig/caddyfile/lexer.go +++ b/caddyconfig/caddyfile/lexer.go @@ -323,7 +323,8 @@ func (l *lexer) finalizeHeredoc(val []rune, marker string) ([]rune, error) { // if the padding doesn't match exactly at the start then we can't safely strip if index != 0 { - return nil, fmt.Errorf("mismatched leading whitespace in heredoc <<%s on line #%d [%s], expected whitespace [%s] to match the closing marker", marker, l.line+lineNum+1, lineText, paddingToStrip) + cleanLineText := strings.TrimRight(lineText, "\r\n") + return nil, fmt.Errorf("mismatched leading whitespace in heredoc <<%s on line #%d [%s], expected whitespace [%s] to match the closing marker", marker, l.line+lineNum+1, cleanLineText, paddingToStrip) } // strip, then append the line, with the newline, to the output. diff --git a/caddyconfig/httpcaddyfile/builtins.go b/caddyconfig/httpcaddyfile/builtins.go index fb05d29f1..71aa0c2b8 100644 --- a/caddyconfig/httpcaddyfile/builtins.go +++ b/caddyconfig/httpcaddyfile/builtins.go @@ -130,6 +130,9 @@ func parseTLS(h Helper) ([]ConfigValue, error) { var reusePrivateKeys bool var forceAutomate bool + // Track which DNS challenge options are set + var dnsOptionsSet []string + firstLine := h.RemainingArgs() switch len(firstLine) { case 0: @@ -350,6 +353,7 @@ func parseTLS(h Helper) ([]ConfigValue, error) { if acmeIssuer.Challenges.DNS == nil { acmeIssuer.Challenges.DNS = new(caddytls.DNSChallengeConfig) } + dnsOptionsSet = append(dnsOptionsSet, "resolvers") acmeIssuer.Challenges.DNS.Resolvers = args case "propagation_delay": @@ -371,6 +375,7 @@ func parseTLS(h Helper) ([]ConfigValue, error) { if acmeIssuer.Challenges.DNS == nil { acmeIssuer.Challenges.DNS = new(caddytls.DNSChallengeConfig) } + dnsOptionsSet = append(dnsOptionsSet, "propagation_delay") acmeIssuer.Challenges.DNS.PropagationDelay = caddy.Duration(delay) case "propagation_timeout": @@ -398,6 +403,7 @@ func parseTLS(h Helper) ([]ConfigValue, error) { if acmeIssuer.Challenges.DNS == nil { acmeIssuer.Challenges.DNS = new(caddytls.DNSChallengeConfig) } + dnsOptionsSet = append(dnsOptionsSet, "propagation_timeout") acmeIssuer.Challenges.DNS.PropagationTimeout = caddy.Duration(timeout) case "dns_ttl": @@ -419,6 +425,7 @@ func parseTLS(h Helper) ([]ConfigValue, error) { if acmeIssuer.Challenges.DNS == nil { acmeIssuer.Challenges.DNS = new(caddytls.DNSChallengeConfig) } + dnsOptionsSet = append(dnsOptionsSet, "dns_ttl") acmeIssuer.Challenges.DNS.TTL = caddy.Duration(ttl) case "dns_challenge_override_domain": @@ -435,6 +442,7 @@ func parseTLS(h Helper) ([]ConfigValue, error) { if acmeIssuer.Challenges.DNS == nil { acmeIssuer.Challenges.DNS = new(caddytls.DNSChallengeConfig) } + dnsOptionsSet = append(dnsOptionsSet, "dns_challenge_override_domain") acmeIssuer.Challenges.DNS.OverrideDomain = arg[0] case "ca_root": @@ -470,6 +478,18 @@ func parseTLS(h Helper) ([]ConfigValue, error) { } } + // Validate DNS challenge config: any DNS challenge option except "dns" requires a DNS provider + if acmeIssuer != nil && acmeIssuer.Challenges != nil && acmeIssuer.Challenges.DNS != nil { + dnsCfg := acmeIssuer.Challenges.DNS + providerSet := dnsCfg.ProviderRaw != nil || h.Option("dns") != nil + if len(dnsOptionsSet) > 0 && !providerSet { + return nil, h.Errf( + "setting DNS challenge options [%s] requires a DNS provider (set with the 'dns' subdirective or 'acme_dns' global option)", + strings.Join(dnsOptionsSet, ", "), + ) + } + } + // a naked tls directive is not allowed if len(firstLine) == 0 && !hasBlock { return nil, h.ArgErr() diff --git a/caddytest/integration/caddyfile_adapt/ambiguous_site_definition.caddyfiletest b/caddytest/integration/caddyfile_adapt/ambiguous_site_definition.caddyfiletest new file mode 100644 index 000000000..bd62d3c4e --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/ambiguous_site_definition.caddyfiletest @@ -0,0 +1,12 @@ +example.com +handle { + respond "one" +} + +example.com +handle { + respond "two" +} +---------- +Caddyfile:6: unrecognized directive: example.com +Did you mean to define a second site? If so, you must use curly braces around each site to separate their configurations. \ No newline at end of file diff --git a/caddytest/integration/caddyfile_adapt/ambiguous_site_definition_duplicate_key.caddyfiletest b/caddytest/integration/caddyfile_adapt/ambiguous_site_definition_duplicate_key.caddyfiletest new file mode 100644 index 000000000..d182b8ffd --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/ambiguous_site_definition_duplicate_key.caddyfiletest @@ -0,0 +1,9 @@ +:8080 { + respond "one" +} + +:8080 { + respond "two" +} +---------- +ambiguous site definition: :8080 \ No newline at end of file diff --git a/caddytest/integration/caddyfile_adapt/directive_as_site_address.caddyfiletest b/caddytest/integration/caddyfile_adapt/directive_as_site_address.caddyfiletest new file mode 100644 index 000000000..7245fd984 --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/directive_as_site_address.caddyfiletest @@ -0,0 +1,5 @@ +handle + +respond "should not work" +---------- +Caddyfile:1: parsed 'handle' as a site address, but it is a known directive; directives must appear in a site block \ No newline at end of file diff --git a/caddytest/integration/caddyfile_adapt/duplicate_listener_address_global.caddyfiletest b/caddytest/integration/caddyfile_adapt/duplicate_listener_address_global.caddyfiletest new file mode 100644 index 000000000..5287557b3 --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/duplicate_listener_address_global.caddyfiletest @@ -0,0 +1,12 @@ +{ + servers { + srv0 { + listen :8080 + } + srv1 { + listen :8080 + } + } +} +---------- +parsing caddyfile tokens for 'servers': unrecognized servers option 'srv0', at Caddyfile:3 \ No newline at end of file diff --git a/caddytest/integration/caddyfile_adapt/heredoc_extra_indentation.caddyfiletest b/caddytest/integration/caddyfile_adapt/heredoc_extra_indentation.caddyfiletest new file mode 100644 index 000000000..1b71b91fc --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/heredoc_extra_indentation.caddyfiletest @@ -0,0 +1,41 @@ +:80 + +handle { + respond < 0 && expected[0] == '{' { + ok := caddytest.CompareAdapt(t, filename, caddyfile, "caddyfile", expected) + if !ok { + t.Errorf("failed to adapt %s", filename) + } + return + } + + // otherwise, adapt the Caddyfile and check for errors + cfgAdapter := caddyconfig.GetAdapter("caddyfile") + _, _, err = cfgAdapter.Adapt([]byte(caddyfile), nil) + if err == nil { + t.Errorf("expected error for %s but got none", filename) + } else { + normalizedErr := winNewlines.ReplaceAllString(err.Error(), "\n") + if !strings.Contains(normalizedErr, expected) { + t.Errorf("expected error for %s to contain:\n%s\nbut got:\n%s", filename, expected, normalizedErr) + } + } + }) } }