httpcaddyfile: Add nil check to prevent panic, fix validation logic

Panic would happen if an automation policy was specified in a singular
server block that had no hostnames in its address. Definitely an edge
case.

Fixed a bug related to checking for server blocks with a host-less key
that tried to make an automation policy. Previously if you had only two
server blocks like ":443" and another one at ":80", the one at ":443"
could not create a TLS automation policy because it thought it would
interfere with TLS automation for the block at ":80", but obviously that
key doesn't enable TLS because it is on the HTTP port. So now we are a
little smarter and count only non-HTTP-empty-hostname keys.

Also fixed a bug so that a key like "https://:1234" is sure to have TLS
enabled by giving it a TLS connection policy. (Relaxed conditions
slightly; the previous conditions were too strict, requiring there to be
a TLS conn policy already or a default SNI to be non-empty.)

Also clarified a comment thanks to feedback from @Mohammed90

3 files changed, 61 insertions, 27 deletions

diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go
index a9ee838..a22dd40 100644
--- a/caddyconfig/httpcaddyfile/httptype.go
+++ b/caddyconfig/httpcaddyfile/httptype.go
@@ -324,6 +324,10 @@ func (st *ServerType) serversFromPairings(
 	if hp, ok := options["http_port"].(int); ok {
 		httpPort = strconv.Itoa(hp)
 	}
+	httpsPort := strconv.Itoa(caddyhttp.DefaultHTTPSPort)
+	if hsp, ok := options["https_port"].(int); ok {
+		httpsPort = strconv.Itoa(hsp)
+	}
 
 	for i, p := range pairings {
 		srv := &caddyhttp.Server{
@@ -362,7 +366,8 @@ func (st *ServerType) serversFromPairings(
 			return specificity(iLongestHost) > specificity(jLongestHost)
 		})
 
-		var hasCatchAllTLSConnPolicy, usesTLS bool
+		var hasCatchAllTLSConnPolicy, addressQualifiesForTLS bool
+		autoHTTPSWillAddConnPolicy := true
 
 		// create a subroute for each site in the server block
 		for _, sblock := range p.serverBlocks {
@@ -401,9 +406,9 @@ func (st *ServerType) serversFromPairings(
 				}
 			}
 
-			// exclude any hosts that were defined explicitly with
-			// "http://" in the key from automated cert management (issue #2998)
 			for _, addr := range sblock.keys {
+				// exclude any hosts that were defined explicitly with "http://"
+				// in the key from automated cert management (issue #2998)
 				if addr.Scheme == "http" && addr.Host != "" {
 					if srv.AutoHTTPS == nil {
 						srv.AutoHTTPS = new(caddyhttp.AutoHTTPSConfig)
@@ -412,9 +417,16 @@ func (st *ServerType) serversFromPairings(
 						srv.AutoHTTPS.Skip = append(srv.AutoHTTPS.Skip, addr.Host)
 					}
 				}
-				if addr.Scheme != "http" && addr.Host != "" && addr.Port != httpPort {
-					usesTLS = true
+				// we'll need to remember if the address qualifies for auto-HTTPS, so we
+				// can add a TLS conn policy if necessary
+				if addr.Scheme == "https" ||
+					(addr.Scheme != "http" && addr.Host != "" && addr.Port != httpPort) {
+					addressQualifiesForTLS = true
 				}
+				// predict whether auto-HTTPS will add the conn policy for us; if so, we
+				// may not need to add one for this server
+				autoHTTPSWillAddConnPolicy = autoHTTPSWillAddConnPolicy &&
+					(addr.Port == httpsPort || (addr.Port != httpPort && addr.Host != ""))
 			}
 
 			// set up each handler directive, making sure to honor directive order
@@ -477,9 +489,9 @@ func (st *ServerType) serversFromPairings(
 		// TODO: maybe a smarter way to handle this might be to just make the
 		// auto-HTTPS logic at provision-time detect if there is any connection
 		// policy missing for any HTTPS-enabled hosts, if so, add it... maybe?
-		if usesTLS &&
+		if addressQualifiesForTLS &&
 			!hasCatchAllTLSConnPolicy &&
-			(len(srv.TLSConnPolicies) > 0 || defaultSNI != "") {
+			(len(srv.TLSConnPolicies) > 0 || !autoHTTPSWillAddConnPolicy || defaultSNI != "") {
 			srv.TLSConnPolicies = append(srv.TLSConnPolicies, &caddytls.ConnectionPolicy{DefaultSNI: defaultSNI})
 		}
 
@@ -539,7 +551,7 @@ func detectConflictingSchemes(srv *caddyhttp.Server, serverBlocks []serverBlock,
 				if err := checkAndSetHTTP(addr); err != nil {
 					return err
 				}
-			} else if addr.Scheme == "https" || addr.Port == httpsPort {
+			} else if addr.Scheme == "https" || addr.Port == httpsPort || len(srv.TLSConnPolicies) > 0 {
 				if err := checkAndSetHTTPS(addr); err != nil {
 					return err
 				}
diff --git a/caddyconfig/httpcaddyfile/tlsapp.go b/caddyconfig/httpcaddyfile/tlsapp.go
index 5c62233..f66fcae 100644
--- a/caddyconfig/httpcaddyfile/tlsapp.go
+++ b/caddyconfig/httpcaddyfile/tlsapp.go
@@ -20,9 +20,11 @@ import (
 	"fmt"
 	"reflect"
 	"sort"
+	"strconv"
 
 	"github.com/caddyserver/caddy/v2"
 	"github.com/caddyserver/caddy/v2/caddyconfig"
+	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
 	"github.com/caddyserver/caddy/v2/modules/caddytls"
 	"github.com/caddyserver/certmagic"
 )
@@ -36,17 +38,26 @@ func (st ServerType) buildTLSApp(
 	tlsApp := &caddytls.TLS{CertificatesRaw: make(caddy.ModuleMap)}
 	var certLoaders []caddytls.CertificateLoader
 
-	// count how many server blocks have a key with no host,
-	// and find all hosts that share a server block with a
-	// hostless key, so that they don't get forgotten/omitted
+	httpsPort := strconv.Itoa(caddyhttp.DefaultHTTPSPort)
+	if hsp, ok := options["https_port"].(int); ok {
+		httpsPort = strconv.Itoa(hsp)
+	}
+
+	// count how many server blocks have a TLS-enabled key with
+	// no host, and find all hosts that share a server block with
+	// a hostless key, so that they don't get forgotten/omitted
 	// by auto-HTTPS (since they won't appear in route matchers)
-	var serverBlocksWithHostlessKey int
+	var serverBlocksWithTLSHostlessKey int
 	hostsSharedWithHostlessKey := make(map[string]struct{})
 	for _, pair := range pairings {
 		for _, sb := range pair.serverBlocks {
 			for _, addr := range sb.keys {
 				if addr.Host == "" {
-					serverBlocksWithHostlessKey++
+					// this address has no hostname, but if it's explicitly set
+					// to HTTPS, then we need to count it as being TLS-enabled
+					if addr.Scheme == "https" || addr.Port == httpsPort {
+						serverBlocksWithTLSHostlessKey++
+					}
 					// this server block has a hostless key, now
 					// go through and add all the hosts to the set
 					for _, otherAddr := range sb.keys {
@@ -149,9 +160,11 @@ func (st ServerType) buildTLSApp(
 			}
 
 			if ap != nil {
-				// encode issuer now that it's all set up
-				issuerName := ap.Issuer.(caddy.Module).CaddyModule().ID.Name()
-				ap.IssuerRaw = caddyconfig.JSONModuleObject(ap.Issuer, "module", issuerName, &warnings)
+				if ap.Issuer != nil {
+					// encode issuer now that it's all set up
+					issuerName := ap.Issuer.(caddy.Module).CaddyModule().ID.Name()
+					ap.IssuerRaw = caddyconfig.JSONModuleObject(ap.Issuer, "module", issuerName, &warnings)
+				}
 
 				// first make sure this block is allowed to create an automation policy;
 				// doing so is forbidden if it has a key with no host (i.e. ":443")
@@ -163,12 +176,12 @@ func (st ServerType) buildTLSApp(
 				// this is an example of a poor mapping from Caddyfile to JSON but that's
 				// the least-leaky abstraction I could figure out
 				if len(sblockHosts) == 0 {
-					if serverBlocksWithHostlessKey > 1 {
+					if serverBlocksWithTLSHostlessKey > 1 {
 						// this server block and at least one other has a key with no host,
 						// making the two indistinguishable; it is misleading to define such
 						// a policy within one server block since it actually will apply to
 						// others as well
-						return nil, warnings, fmt.Errorf("cannot make a TLS automation policy from a server block that has a host-less address when there are other server block addresses lacking a host")
+						return nil, warnings, fmt.Errorf("cannot make a TLS automation policy from a server block that has a host-less address when there are other TLS-enabled server block addresses lacking a host")
 					}
 					if catchAllAP == nil {
 						// this server block has a key with no hosts, but there is not yet
@@ -293,9 +306,11 @@ func (st ServerType) buildTLSApp(
 
 	// if there is a global/catch-all automation policy, ensure it goes last
 	if catchAllAP != nil {
-		// first, encode its issuer
-		issuerName := catchAllAP.Issuer.(caddy.Module).CaddyModule().ID.Name()
-		catchAllAP.IssuerRaw = caddyconfig.JSONModuleObject(catchAllAP.Issuer, "module", issuerName, &warnings)
+		// first, encode its issuer, if there is one
+		if catchAllAP.Issuer != nil {
+			issuerName := catchAllAP.Issuer.(caddy.Module).CaddyModule().ID.Name()
+			catchAllAP.IssuerRaw = caddyconfig.JSONModuleObject(catchAllAP.Issuer, "module", issuerName, &warnings)
+		}
 
 		// then append it to the end of the policies list
 		if tlsApp.Automation == nil {
diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go
index ad0a716..f62543b 100644
--- a/modules/caddyhttp/autohttps.go
+++ b/modules/caddyhttp/autohttps.go
@@ -152,12 +152,12 @@ func (app *App) automaticHTTPSPhase1(ctx caddy.Context, repl *caddy.Replacer) er
 		}
 
 		// nothing more to do here if there are no domains that qualify for
-		// automatic HTTPS or there are no explicit TLS connection policies;
-		// if there is at least one domain but no TLS conn policy, we'll add
-		// one below; if there is a TLS conn policy (meaning TLS is enabled)
-		// and no domains, it could be a catch-all with on-demand TLS, and
-		// in that case we would still need HTTP->HTTPS redirects, which we
-		// do below
+		// 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
+		// add one below; if there are no domains but at least one TLS conn
+		// policy (meaning TLS is enabled) (T&&F), it could be a catch-all with
+		// on-demand TLS -- and in that case we would still need HTTP->HTTPS
+		// redirects, which we set up below; hence these two conditions
 		if len(serverDomainSet) == 0 && len(srv.TLSConnPolicies) == 0 {
 			continue
 		}
@@ -345,6 +345,13 @@ uniqueDomainsLoop:
 	// not entirely clear what the redirect destination should be,
 	// so I'm going to just hard-code the app's HTTPS port and call
 	// it good for now...
+	// TODO: This implies that all plaintext requests will be blindly
+	// redirected to their HTTPS equivalent, even if this server
+	// doesn't handle that hostname at all; I don't think this is a
+	// bad thing, and it also obscures the actual hostnames that this
+	// server is configured to match on, which may be desirable, but
+	// it's not something that should be relied on. We can change this
+	// if we want to.
 	appendCatchAll := func(routes []Route) []Route {
 		redirTo := "https://{http.request.host}"
 		if app.httpsPort() != DefaultHTTPSPort {