From a58f240d3ecbb59285303746406cab50217f8d24 Mon Sep 17 00:00:00 2001
From: Francis Lavoie <lavofr@gmail.com>
Date: Fri, 25 Mar 2022 00:54:03 -0400
Subject: [PATCH] httpcaddyfile: Fix #4640 (auto-HTTPS edgecase) (#4661)

Guh, this is complicated.

Fixes #4640

This also follows up on #4398 (reverting it) which made a change that technically worked, but was incorrect. It changed the condition in `hostsFromKeysNotHTTP` from `&&` to `||`, but then the function no longer did what its name said it would do, and it would return hosts even if they were marked with `http://`, if they used a non-HTTP port. That wasn't the intent of it. The test added in there was kept though, because it is a valid usecase.

The actual fix is to check _earlier_ whether all the addresses explicitly have `http://`, and if so we can short circuit and skip considering the rest.
---
 caddyconfig/httpcaddyfile/directives.go       | 13 ++++-
 caddyconfig/httpcaddyfile/httptype.go         |  2 +-
 caddyconfig/httpcaddyfile/tlsapp.go           |  6 ++
 .../tls_automation_policies_9.txt             | 56 +++++++++++++++++++
 4 files changed, 75 insertions(+), 2 deletions(-)
 create mode 100644 caddytest/integration/caddyfile_adapt/tls_automation_policies_9.txt

diff --git a/caddyconfig/httpcaddyfile/directives.go b/caddyconfig/httpcaddyfile/directives.go
index 425bf192..6b80e346 100644
--- a/caddyconfig/httpcaddyfile/directives.go
+++ b/caddyconfig/httpcaddyfile/directives.go
@@ -494,7 +494,7 @@ func (sb serverBlock) hostsFromKeysNotHTTP(httpPort string) []string {
 		if addr.Host == "" {
 			continue
 		}
-		if addr.Scheme != "http" || addr.Port != httpPort {
+		if addr.Scheme != "http" && addr.Port != httpPort {
 			hostMap[addr.Host] = struct{}{}
 		}
 	}
@@ -519,6 +519,17 @@ func (sb serverBlock) hasHostCatchAllKey() bool {
 	return false
 }
 
+// isAllHTTP returns true if all sb keys explicitly specify
+// the http:// scheme
+func (sb serverBlock) isAllHTTP() bool {
+	for _, addr := range sb.keys {
+		if addr.Scheme != "http" {
+			return false
+		}
+	}
+	return true
+}
+
 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/httptype.go b/caddyconfig/httpcaddyfile/httptype.go
index d7716a42..f5dd68a6 100644
--- a/caddyconfig/httpcaddyfile/httptype.go
+++ b/caddyconfig/httpcaddyfile/httptype.go
@@ -581,7 +581,7 @@ func (st *ServerType) serversFromPairings(
 			}
 
 			for _, addr := range sblock.keys {
-				// if server only uses HTTPS port, auto-HTTPS will not apply
+				// if server only uses HTTP port, auto-HTTPS will not apply
 				if listenersUseAnyPortOtherThan(srv.Listen, httpPort) {
 					// exclude any hosts that were defined explicitly with "http://"
 					// in the key from automated cert management (issue #2998)
diff --git a/caddyconfig/httpcaddyfile/tlsapp.go b/caddyconfig/httpcaddyfile/tlsapp.go
index daaec95d..76d7ebf3 100644
--- a/caddyconfig/httpcaddyfile/tlsapp.go
+++ b/caddyconfig/httpcaddyfile/tlsapp.go
@@ -101,6 +101,12 @@ func (st ServerType) buildTLSApp(
 		}
 
 		for _, sblock := range p.serverBlocks {
+			// check the scheme of all the site addresses,
+			// skip building AP if they all had http://
+			if sblock.isAllHTTP() {
+				continue
+			}
+
 			// get values that populate an automation policy for this block
 			ap, err := newBaseAutomationPolicy(options, warnings, true)
 			if err != nil {
diff --git a/caddytest/integration/caddyfile_adapt/tls_automation_policies_9.txt b/caddytest/integration/caddyfile_adapt/tls_automation_policies_9.txt
new file mode 100644
index 00000000..bd82e96c
--- /dev/null
+++ b/caddytest/integration/caddyfile_adapt/tls_automation_policies_9.txt
@@ -0,0 +1,56 @@
+# example from issue #4640
+http://foo:8447, http://127.0.0.1:8447 {
+	reverse_proxy 127.0.0.1:8080
+}
+----------
+{
+	"apps": {
+		"http": {
+			"servers": {
+				"srv0": {
+					"listen": [
+						":8447"
+					],
+					"routes": [
+						{
+							"match": [
+								{
+									"host": [
+										"foo",
+										"127.0.0.1"
+									]
+								}
+							],
+							"handle": [
+								{
+									"handler": "subroute",
+									"routes": [
+										{
+											"handle": [
+												{
+													"handler": "reverse_proxy",
+													"upstreams": [
+														{
+															"dial": "127.0.0.1:8080"
+														}
+													]
+												}
+											]
+										}
+									]
+								}
+							],
+							"terminal": true
+						}
+					],
+					"automatic_https": {
+						"skip": [
+							"foo",
+							"127.0.0.1"
+						]
+					}
+				}
+			}
+		}
+	}
+}
\ No newline at end of file