From b3d35a4995c98cfadeb0c3a356025dbd3984caef Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Fri, 25 Jun 2021 11:28:32 -0600 Subject: [PATCH] httpcaddyfile: Don't put localhost in public APs (fix #4220) If an email is specified in global options, a site called 'localhost' shouldn't be bunched together with public DNS names in the automation policies, which get the default, public-CA issuers. Fix old test that did this. I also noticed that these two: localhost { } example.com { } and localhost, example.com { } produce slightly different TLS automation policies. The former is what the new test case covers, and we have logic that removes the empty automation policy for localhost so that auto-HTTPS can implicitly create one. (We prefer that whenever possible.) But the latter case produces two automation policies, with the second one being for localhost, with an explicit internal issuer. It's not wrong, just more explicit than it needs to be. I'd really like to completely rewrite the code from scratch that generates automation policies, hopefully there is a simpler, more correct algorithm. --- caddyconfig/httpcaddyfile/tlsapp.go | 37 +++++++++-- .../global_options_preferred_chains.txt | 6 +- ...mation_policies_global_email_localhost.txt | 66 +++++++++++++++++++ 3 files changed, 102 insertions(+), 7 deletions(-) create mode 100644 caddytest/integration/caddyfile_adapt/tls_automation_policies_global_email_localhost.txt diff --git a/caddyconfig/httpcaddyfile/tlsapp.go b/caddyconfig/httpcaddyfile/tlsapp.go index 1bfddda1..0fe1fc5f 100644 --- a/caddyconfig/httpcaddyfile/tlsapp.go +++ b/caddyconfig/httpcaddyfile/tlsapp.go @@ -211,7 +211,7 @@ func (st ServerType) buildTLSApp( // it that we would need to check here) since the hostname is known at handshake; // and it is unexpected to switch to internal issuer when the user wants to get // regular certificates on-demand for a class of certs like *.*.tld. - if !certmagic.SubjectIsIP(s) && !certmagic.SubjectIsInternal(s) && (strings.Count(s, "*.") < 2 || ap.OnDemand) { + if subjectQualifiesForPublicCert(ap, s) { external = append(external, s) } else { internal = append(internal, s) @@ -324,8 +324,12 @@ func (st ServerType) buildTLSApp( globalPreferredChains := options["preferred_chains"] hasGlobalACMEDefaults := globalEmail != nil || globalACMECA != nil || globalACMECARoot != nil || globalACMEDNS != nil || globalACMEEAB != nil || globalPreferredChains != nil if hasGlobalACMEDefaults { - for _, ap := range tlsApp.Automation.Policies { - if len(ap.Issuers) == 0 { + // for _, ap := range tlsApp.Automation.Policies { + for i := 0; i < len(tlsApp.Automation.Policies); i++ { + ap := tlsApp.Automation.Policies[i] + if len(ap.Issuers) == 0 && automationPolicyHasAllPublicNames(ap) { + // for public names, create default issuers which will later be filled in with configured global defaults + // (internal names will implicitly use the internal issuer at auto-https time) ap.Issuers = caddytls.DefaultIssuers() // if a specific endpoint is configured, can't use multiple default issuers @@ -494,16 +498,23 @@ func consolidateAutomationPolicies(aps []*caddytls.AutomationPolicy) []*caddytls }) emptyAPCount := 0 + origLenAPs := len(aps) // compute the number of empty policies (disregarding subjects) - see #4128 emptyAP := new(caddytls.AutomationPolicy) for i := 0; i < len(aps); i++ { emptyAP.Subjects = aps[i].Subjects if reflect.DeepEqual(aps[i], emptyAP) { emptyAPCount++ + if !automationPolicyHasAllPublicNames(aps[i]) { + // if this automation policy has internal names, we might as well remove it + // so auto-https can implicitly use the internal issuer + aps = append(aps[:i], aps[i+1:]...) + i-- + } } } // If all policies are empty, we can return nil, as there is no need to set any policy - if emptyAPCount == len(aps) { + if emptyAPCount == origLenAPs { return nil } @@ -601,3 +612,21 @@ func automationPolicyShadows(i int, aps []*caddytls.AutomationPolicy) int { } return -1 } + +// subjectQualifiesForPublicCert is like certmagic.SubjectQualifiesForPublicCert() except +// that this allows domains with multiple wildcard levels like '*.*.example.com' to qualify +// if the automation policy has OnDemand enabled (i.e. this function is more lenient). +func subjectQualifiesForPublicCert(ap *caddytls.AutomationPolicy, subj string) bool { + return !certmagic.SubjectIsIP(subj) && + !certmagic.SubjectIsInternal(subj) && + (strings.Count(subj, "*.") < 2 || ap.OnDemand) +} + +func automationPolicyHasAllPublicNames(ap *caddytls.AutomationPolicy) bool { + for _, subj := range ap.Subjects { + if !subjectQualifiesForPublicCert(ap, subj) { + return false + } + } + return true +} diff --git a/caddytest/integration/caddyfile_adapt/global_options_preferred_chains.txt b/caddytest/integration/caddyfile_adapt/global_options_preferred_chains.txt index 893b34b4..9173b26b 100644 --- a/caddytest/integration/caddyfile_adapt/global_options_preferred_chains.txt +++ b/caddytest/integration/caddyfile_adapt/global_options_preferred_chains.txt @@ -2,7 +2,7 @@ preferred_chains smallest } -localhost +example.com ---------- { "apps": { @@ -17,7 +17,7 @@ localhost "match": [ { "host": [ - "localhost" + "example.com" ] } ], @@ -32,7 +32,7 @@ localhost "policies": [ { "subjects": [ - "localhost" + "example.com" ], "issuers": [ { diff --git a/caddytest/integration/caddyfile_adapt/tls_automation_policies_global_email_localhost.txt b/caddytest/integration/caddyfile_adapt/tls_automation_policies_global_email_localhost.txt new file mode 100644 index 00000000..e8ef3a7e --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/tls_automation_policies_global_email_localhost.txt @@ -0,0 +1,66 @@ +{ + email foo@bar +} + +localhost { +} + +example.com { +} +---------- +{ + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":443" + ], + "routes": [ + { + "match": [ + { + "host": [ + "example.com" + ] + } + ], + "terminal": true + }, + { + "match": [ + { + "host": [ + "localhost" + ] + } + ], + "terminal": true + } + ] + } + } + }, + "tls": { + "automation": { + "policies": [ + { + "subjects": [ + "example.com" + ], + "issuers": [ + { + "email": "foo@bar", + "module": "acme" + }, + { + "email": "foo@bar", + "module": "zerossl" + } + ] + } + ] + } + } + } +} \ No newline at end of file