httpcaddyfile: Improve AP logic with OnDemand

We have users that have site blocks like *.*.tld with on-demand TLS
enabled. While *.*.tld does not qualify for a publicly-trusted cert due
to its wildcards, On-Demand TLS does not actually obtain a cert with
those wildcards, since it uses the actual hostname on the handshake.

This improves on that logic, but I am still not 100% satisfied with the
result since I think we need to also check if another site block is more
specific, like foo.example.tld, which might not have on-demand TLS
enabled, and make sure an automation policy gets created before the
more general policy with on-demand...

2 files changed, 31 insertions, 5 deletions

diff --git a/caddyconfig/httpcaddyfile/tlsapp.go b/caddyconfig/httpcaddyfile/tlsapp.go
index a721fee..e732957 100644
--- a/caddyconfig/httpcaddyfile/tlsapp.go
+++ b/caddyconfig/httpcaddyfile/tlsapp.go
@@ -213,7 +213,17 @@ func (st ServerType) buildTLSApp(
 					if ap.Issuer == nil {
 						var internal, external []string
 						for _, s := range ap.Subjects {
-							if certmagic.SubjectQualifiesForPublicCert(s) {
+							if !certmagic.SubjectQualifiesForCert(s) {
+								return nil, warnings, fmt.Errorf("subject does not qualify for certificate: '%s'", s)
+							}
+							// we don't use certmagic.SubjectQualifiesForPublicCert() because of one nuance:
+							// names like *.*.tld that may not qualify for a public certificate are actually
+							// fine when used with OnDemand, since OnDemand (currently) does not obtain
+							// wildcards (if it ever does, there will be a separate config option to enable
+							// 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) {
 								external = append(external, s)
 							} else {
 								internal = append(internal, s)
diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go
index 6a635d4..12d25ad 100644
--- a/modules/caddytls/tls.go
+++ b/modules/caddytls/tls.go
@@ -334,10 +334,26 @@ func (t *TLS) AddAutomationPolicy(ap *AutomationPolicy) error {
 	if err != nil {
 		return err
 	}
-	for i, other := range t.Automation.Policies {
-		// if a catch-all policy (or really, any policy with
-		// fewer names) exists, prioritize this new policy
-		if len(other.Subjects) < len(ap.Subjects) {
+	// sort new automation policies just before any other which is a superset
+	// of this one; if we find an existing policy that covers every subject in
+	// ap but less specifically (e.g. a catch-all policy, or one with wildcards
+	// or with fewer subjects), insert ap just before it, otherwise ap would
+	// never be used because the first matching policy is more general
+	for i, existing := range t.Automation.Policies {
+		// first see if existing is superset of ap for all names
+		var otherIsSuperset bool
+	outer:
+		for _, thisSubj := range ap.Subjects {
+			for _, otherSubj := range existing.Subjects {
+				if certmagic.MatchWildcard(thisSubj, otherSubj) {
+					otherIsSuperset = true
+					break outer
+				}
+			}
+		}
+		// if existing AP is a superset or if it contains fewer names (i.e. is
+		// more general), then new AP is more specific, so insert before it
+		if otherIsSuperset || len(existing.Subjects) < len(ap.Subjects) {
 			t.Automation.Policies = append(t.Automation.Policies[:i],
 				append([]*AutomationPolicy{ap}, t.Automation.Policies[i:]...)...)
 			return nil