httpcaddyfile, caddytls: Multiple edge case fixes; add tests

- Create two default automation policies; if the TLS app is used in
  isolation with the 'automate' certificate loader, it will now use
  an internal issuer for internal-only names, and an ACME issuer for
  all other names by default.
- If the HTTP Caddyfile adds an 'automate' loader, it now also adds an
  automation policy for any names in that loader that do not qualify
  for public certificates so that they will be issued internally. (It
  might be nice if this wasn't necessary, but the alternative is to
  either make auto-HTTPS logic way more complex by scanning the names in
  the 'automate' loader, or to have an automation policy without an
  issuer switch between default issuer based on the name being issued
  a certificate - I think I like the latter option better, right now we
  do something kind of like that but at a level above each individual
  automation policies, we do that switch only when no automation
  policies match, rather than when a policy without an issuer does
  match.)
- Set the default LoggerName rather than a LoggerNames with an empty
  host value, which is now taken literally rather than as a catch-all.
- hostsFromKeys, the function that gets a list of hosts from server
  block keys, no longer returns an empty string in its resulting slice,
  ever.

6 files changed, 186 insertions, 39 deletions

diff --git a/caddyconfig/httpcaddyfile/directives.go b/caddyconfig/httpcaddyfile/directives.go
index 8fa48cd..ff248bf 100644
--- a/caddyconfig/httpcaddyfile/directives.go
+++ b/caddyconfig/httpcaddyfile/directives.go
@@ -393,23 +393,30 @@ type serverBlock struct {
 }
 
 // hostsFromKeys returns a list of all the non-empty hostnames found in
-// the keys of the server block sb, unless allowEmpty is true, in which
-// case a key with no host (e.g. ":443") will be added to the list as an
-// empty string. Otherwise, if allowEmpty is false, and if sb has a key
-// that omits the hostname (i.e. is a catch-all/empty host), then the returned
-// list is empty, because the server block effectively matches ALL hosts.
-// The list may not be in a consistent order. If includePorts is true, then
-// any non-empty, non-standard ports will be included.
-func (sb serverBlock) hostsFromKeys(allowEmpty, includePorts bool) []string {
-	// first get each unique hostname
+// the keys of the server block sb. If logger mode is false, a key with
+// an empty hostname portion will return an empty slice, since that
+// server block is interpreted to effectively match all hosts. An empty
+// string is never added to the slice.
+//
+// If loggerMode is true, then the non-standard ports of keys will be
+// joined to the hostnames. This is to effectively match the Host
+// header of requests that come in for that key.
+//
+// The resulting slice is not sorted but will never have duplicates.
+func (sb serverBlock) hostsFromKeys(loggerMode bool) []string {
+	// ensure each entry in our list is unique
 	hostMap := make(map[string]struct{})
 	for _, addr := range sb.keys {
-		if addr.Host == "" && !allowEmpty {
-			// server block contains a key like ":443", i.e. the host portion
-			// is empty / catch-all, which means to match all hosts
-			return []string{}
+		if addr.Host == "" {
+			if !loggerMode {
+				// server block contains a key like ":443", i.e. the host portion
+				// is empty / catch-all, which means to match all hosts
+				return []string{}
+			}
+			// never append an empty string
+			continue
 		}
-		if includePorts &&
+		if loggerMode &&
 			addr.Port != "" &&
 			addr.Port != strconv.Itoa(caddyhttp.DefaultHTTPPort) &&
 			addr.Port != strconv.Itoa(caddyhttp.DefaultHTTPSPort) {
@@ -428,6 +435,17 @@ func (sb serverBlock) hostsFromKeys(allowEmpty, includePorts bool) []string {
 	return sblockHosts
 }
 
+// hasHostCatchAllKey returns true if sb has a key that
+// omits a host portion, i.e. it "catches all" hosts.
+func (sb serverBlock) hasHostCatchAllKey() bool {
+	for _, addr := range sb.keys {
+		if addr.Host == "" {
+			return true
+		}
+	}
+	return false
+}
+
 type (
 	// UnmarshalFunc is a function which can unmarshal Caddyfile
 	// tokens into zero or more config values using a Helper type.
diff --git a/caddyconfig/httpcaddyfile/directives_test.go b/caddyconfig/httpcaddyfile/directives_test.go
new file mode 100644
index 0000000..03768ac
--- /dev/null
+++ b/caddyconfig/httpcaddyfile/directives_test.go
@@ -0,0 +1,94 @@
+package httpcaddyfile
+
+import (
+	"reflect"
+	"sort"
+	"testing"
+)
+
+func TestHostsFromKeys(t *testing.T) {
+	for i, tc := range []struct {
+		keys             []Address
+		expectNormalMode []string
+		expectLoggerMode []string
+	}{
+		{
+			[]Address{
+				Address{Original: "foo", Host: "foo"},
+			},
+			[]string{"foo"},
+			[]string{"foo"},
+		},
+		{
+			[]Address{
+				Address{Original: "foo", Host: "foo"},
+				Address{Original: "bar", Host: "bar"},
+			},
+			[]string{"bar", "foo"},
+			[]string{"bar", "foo"},
+		},
+		{
+			[]Address{
+				Address{Original: ":2015", Port: "2015"},
+			},
+			[]string{}, []string{},
+		},
+		{
+			[]Address{
+				Address{Original: ":443", Port: "443"},
+			},
+			[]string{}, []string{},
+		},
+		{
+			[]Address{
+				Address{Original: "foo", Host: "foo"},
+				Address{Original: ":2015", Port: "2015"},
+			},
+			[]string{}, []string{"foo"},
+		},
+		{
+			[]Address{
+				Address{Original: "example.com:2015", Host: "example.com", Port: "2015"},
+			},
+			[]string{"example.com"},
+			[]string{"example.com:2015"},
+		},
+		{
+			[]Address{
+				Address{Original: "example.com:80", Host: "example.com", Port: "80"},
+			},
+			[]string{"example.com"},
+			[]string{"example.com"},
+		},
+		{
+			[]Address{
+				Address{Original: "https://:2015/foo", Scheme: "https", Port: "2015", Path: "/foo"},
+			},
+			[]string{},
+			[]string{},
+		},
+		{
+			[]Address{
+				Address{Original: "https://example.com:2015/foo", Scheme: "https", Host: "example.com", Port: "2015", Path: "/foo"},
+			},
+			[]string{"example.com"},
+			[]string{"example.com:2015"},
+		},
+	} {
+		sb := serverBlock{keys: tc.keys}
+
+		// test in normal mode
+		actual := sb.hostsFromKeys(false)
+		sort.Strings(actual)
+		if !reflect.DeepEqual(tc.expectNormalMode, actual) {
+			t.Errorf("Test %d (loggerMode=false): Expected: %v Actual: %v", i, tc.expectNormalMode, actual)
+		}
+
+		// test in logger mode
+		actual = sb.hostsFromKeys(true)
+		sort.Strings(actual)
+		if !reflect.DeepEqual(tc.expectLoggerMode, actual) {
+			t.Errorf("Test %d (loggerMode=true): Expected: %v Actual: %v", i, tc.expectLoggerMode, actual)
+		}
+	}
+}
diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go
index a738166..4c5f445 100644
--- a/caddyconfig/httpcaddyfile/httptype.go
+++ b/caddyconfig/httpcaddyfile/httptype.go
@@ -378,7 +378,7 @@ func (st *ServerType) serversFromPairings(
 				return nil, fmt.Errorf("server block %v: compiling matcher sets: %v", sblock.block.Keys, err)
 			}
 
-			hosts := sblock.hostsFromKeys(false, false)
+			hosts := sblock.hostsFromKeys(false)
 
 			// tls: connection policies
 			if cpVals, ok := sblock.pile["tls.connection_policy"]; ok {
@@ -450,9 +450,13 @@ func (st *ServerType) serversFromPairings(
 						LoggerNames: make(map[string]string),
 					}
 				}
-				for _, h := range sblock.hostsFromKeys(true, true) {
-					if ncl.name != "" {
-						srv.Logs.LoggerNames[h] = ncl.name
+				if sblock.hasHostCatchAllKey() {
+					srv.Logs.LoggerName = ncl.name
+				} else {
+					for _, h := range sblock.hostsFromKeys(true) {
+						if ncl.name != "" {
+							srv.Logs.LoggerNames[h] = ncl.name
+						}
 					}
 				}
 			}
diff --git a/caddyconfig/httpcaddyfile/tlsapp.go b/caddyconfig/httpcaddyfile/tlsapp.go
index 2ce7ea3..b31bdc5 100644
--- a/caddyconfig/httpcaddyfile/tlsapp.go
+++ b/caddyconfig/httpcaddyfile/tlsapp.go
@@ -16,6 +16,7 @@ package httpcaddyfile
 
 import (
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"reflect"
 	"sort"
@@ -53,7 +54,7 @@ func (st ServerType) buildTLSApp(
 							continue
 						}
 						if otherAddr.Host != "" {
-							hostsSharedWithHostlessKey[addr.Host] = struct{}{}
+							hostsSharedWithHostlessKey[otherAddr.Host] = struct{}{}
 						}
 					}
 					break
@@ -72,7 +73,7 @@ func (st ServerType) buildTLSApp(
 			// get values that populate an automation policy for this block
 			var ap *caddytls.AutomationPolicy
 
-			sblockHosts := sblock.hostsFromKeys(false, false)
+			sblockHosts := sblock.hostsFromKeys(false)
 			if len(sblockHosts) == 0 {
 				ap = catchAllAP
 			}
@@ -263,6 +264,33 @@ func (st ServerType) buildTLSApp(
 		tlsApp.Automation.OnDemand = onDemand
 	}
 
+	// if any hostnames appear on the same server block as a key with
+	// no host, they will not be used with route matchers because the
+	// hostless key matches all hosts, therefore, it wouldn't be
+	// considered for auto-HTTPS, so we need to make sure those hosts
+	// are manually considered for managed certificates; we also need
+	// to make sure that any of these names which are internal-only
+	// get internal certificates by default rather than ACME
+	var al caddytls.AutomateLoader
+	internalAP := &caddytls.AutomationPolicy{
+		IssuerRaw: json.RawMessage(`{"module":"internal"}`),
+	}
+	for h := range hostsSharedWithHostlessKey {
+		al = append(al, h)
+		if !certmagic.SubjectQualifiesForPublicCert(h) {
+			internalAP.Subjects = append(internalAP.Subjects, h)
+		}
+	}
+	if len(al) > 0 {
+		tlsApp.CertificatesRaw["automate"] = caddyconfig.JSON(al, &warnings)
+	}
+	if len(internalAP.Subjects) > 0 {
+		if tlsApp.Automation == nil {
+			tlsApp.Automation = new(caddytls.AutomationConfig)
+		}
+		tlsApp.Automation.Policies = append(tlsApp.Automation.Policies, internalAP)
+	}
+
 	// if there is a global/catch-all automation policy, ensure it goes last
 	if catchAllAP != nil {
 		// first, encode its issuer
@@ -276,19 +304,6 @@ func (st ServerType) buildTLSApp(
 		tlsApp.Automation.Policies = append(tlsApp.Automation.Policies, catchAllAP)
 	}
 
-	// if any hostnames appear on the same server block as a key with
-	// no host, they will not be used with route matchers because the
-	// hostless key matches all hosts, therefore, it wouldn't be
-	// considered for auto-HTTPS, so we need to make sure those hosts
-	// are manually considered for managed certificates
-	var al caddytls.AutomateLoader
-	for h := range hostsSharedWithHostlessKey {
-		al = append(al, h)
-	}
-	if len(al) > 0 {
-		tlsApp.CertificatesRaw["automate"] = caddyconfig.JSON(al, &warnings)
-	}
-
 	// do a little verification & cleanup
 	if tlsApp.Automation != nil {
 		// ensure automation policies don't overlap subjects (this should be
diff --git a/modules/caddytls/automation.go b/modules/caddytls/automation.go
index 22cf20b..87e6b28 100644
--- a/modules/caddytls/automation.go
+++ b/modules/caddytls/automation.go
@@ -53,7 +53,8 @@ type AutomationConfig struct {
 	// a low value.
 	RenewCheckInterval caddy.Duration `json:"renew_interval,omitempty"`
 
-	defaultAutomationPolicy *AutomationPolicy
+	defaultPublicAutomationPolicy   *AutomationPolicy
+	defaultInternalAutomationPolicy *AutomationPolicy
 }
 
 // AutomationPolicy designates the policy for automating the
@@ -67,7 +68,8 @@ type AutomationPolicy struct {
 	// Which subjects (hostnames or IP addresses) this policy applies to.
 	Subjects []string `json:"subjects,omitempty"`
 
-	// The module that will issue certificates. Default: acme
+	// The module that will issue certificates. Default: internal if all
+	// subjects do not qualify for public certificates; othewise acme.
 	IssuerRaw json.RawMessage `json:"issuer,omitempty" caddy:"namespace=tls.issuance inline_key=module"`
 
 	// If true, certificates will be requested with MustStaple. Not all
diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go
index 54f0e23..1255d3d 100644
--- a/modules/caddytls/tls.go
+++ b/modules/caddytls/tls.go
@@ -93,10 +93,17 @@ func (t *TLS) Provision(ctx caddy.Context) error {
 	if t.Automation == nil {
 		t.Automation = new(AutomationConfig)
 	}
-	t.Automation.defaultAutomationPolicy = new(AutomationPolicy)
-	err := t.Automation.defaultAutomationPolicy.Provision(t)
+	t.Automation.defaultPublicAutomationPolicy = new(AutomationPolicy)
+	err := t.Automation.defaultPublicAutomationPolicy.Provision(t)
 	if err != nil {
-		return fmt.Errorf("provisioning default automation policy: %v", err)
+		return fmt.Errorf("provisioning default public automation policy: %v", err)
+	}
+	t.Automation.defaultInternalAutomationPolicy = &AutomationPolicy{
+		IssuerRaw: json.RawMessage(`{"module":"internal"}`),
+	}
+	err = t.Automation.defaultInternalAutomationPolicy.Provision(t)
+	if err != nil {
+		return fmt.Errorf("provisioning default internal automation policy: %v", err)
 	}
 	for i, ap := range t.Automation.Policies {
 		err := ap.Provision(t)
@@ -318,6 +325,10 @@ func (t *TLS) getConfigForName(name string) *certmagic.Config {
 	return ap.magic
 }
 
+// getAutomationPolicyForName returns the first matching automation policy
+// for the given subject name. If no matching policy can be found, the
+// default policy is used, depending on whether the name qualifies for a
+// public certificate or not.
 func (t *TLS) getAutomationPolicyForName(name string) *AutomationPolicy {
 	for _, ap := range t.Automation.Policies {
 		if len(ap.Subjects) == 0 {
@@ -329,7 +340,10 @@ func (t *TLS) getAutomationPolicyForName(name string) *AutomationPolicy {
 			}
 		}
 	}
-	return t.Automation.defaultAutomationPolicy
+	if certmagic.SubjectQualifiesForPublicCert(name) {
+		return t.Automation.defaultPublicAutomationPolicy
+	}
+	return t.Automation.defaultInternalAutomationPolicy
 }
 
 // AllMatchingCertificates returns the list of all certificates in