Fix bugs related to auto HTTPS and alternate port configurations

3 files changed, 70 insertions, 31 deletions

diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index 3ffc989..89ea58f 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -126,7 +126,8 @@ func (app *App) Start() error {
 					return fmt.Errorf("%s: listening on %s: %v", network, addr, err)
 				}
 
-				// enable HTTP/2 by default
+				// enable HTTP/2 (and support for solving the
+				// TLS-ALPN ACME challenge) by default
 				for _, pol := range srv.TLSConnPolicies {
 					if len(pol.ALPN) == 0 {
 						pol.ALPN = append(pol.ALPN, defaultALPN...)
@@ -219,13 +220,38 @@ func (app *App) automaticHTTPS() error {
 				domains = append(domains, d)
 			}
 
+			// ensure that these certificates are managed properly;
+			// for example, it's implied that the HTTPPort should also
+			// be the port the HTTP challenge is solved on, and so
+			// for HTTPS port and TLS-ALPN challenge also - we need
+			// to tell the TLS app to manage these certs by honoring
+			// those port configurations
+			acmeManager := &caddytls.ACMEManagerMaker{
+				Challenges: caddytls.ChallengesConfig{
+					HTTP: caddytls.HTTPChallengeConfig{
+						AlternatePort: app.HTTPPort,
+					},
+					TLSALPN: caddytls.TLSALPNChallengeConfig{
+						AlternatePort: app.HTTPSPort,
+					},
+				},
+			}
+			acmeManager.SetDefaults()
+			tlsApp.Automation.Policies = append(tlsApp.Automation.Policies,
+				caddytls.AutomationPolicy{
+					Hosts:      domains,
+					Management: acmeManager,
+				})
+
 			// manage their certificates
 			err := tlsApp.Manage(domains)
 			if err != nil {
 				return fmt.Errorf("%s: managing certificate for %s: %s", srvName, domains, err)
 			}
 
-			// tell the server to use TLS
+			// tell the server to use TLS by specifying a TLS
+			// connection policy (which supports HTTP/2 and the
+			// TLS-ALPN ACME challenge as well)
 			srv.TLSConnPolicies = caddytls.ConnectionPolicies{
 				{ALPN: defaultALPN},
 			}
@@ -296,6 +322,7 @@ func (app *App) automaticHTTPS() error {
 			Listen:           lnAddrs,
 			Routes:           redirRoutes,
 			DisableAutoHTTPS: true,
+			tlsApp:           tlsApp, // required to solve HTTP challenge
 		}
 	}
 
diff --git a/modules/caddytls/acmemanager.go b/modules/caddytls/acmemanager.go
index 9352d75..9cbf349 100644
--- a/modules/caddytls/acmemanager.go
+++ b/modules/caddytls/acmemanager.go
@@ -15,13 +15,19 @@ import (
 func init() {
 	caddy2.RegisterModule(caddy2.Module{
 		Name: "tls.management.acme",
-		New:  func() interface{} { return new(acmeManagerMaker) },
+		New:  func() interface{} { return new(ACMEManagerMaker) },
 	})
 }
 
-// acmeManagerMaker makes an ACME manager
-// for managinig certificates using ACME.
-type acmeManagerMaker struct {
+// ACMEManagerMaker makes an ACME manager
+// for managing certificates using ACME.
+// If crafting one manually rather than
+// through the config-unmarshal process
+// (provisioning), be sure to call
+// SetDefaults to ensure sane defaults
+// after you have configured this struct
+// to your liking.
+type ACMEManagerMaker struct {
 	CA          string           `json:"ca,omitempty"`
 	Email       string           `json:"email,omitempty"`
 	RenewAhead  caddy2.Duration  `json:"renew_ahead,omitempty"`
@@ -36,19 +42,22 @@ type acmeManagerMaker struct {
 	keyType certcrypto.KeyType
 }
 
-func (m *acmeManagerMaker) newManager(interactive bool) (certmagic.Manager, error) {
+// newManager is a no-op to satisfy the ManagerMaker interface,
+// because this manager type is a special case.
+func (m *ACMEManagerMaker) newManager(interactive bool) (certmagic.Manager, error) {
 	return nil, nil
 }
 
-func (m *acmeManagerMaker) Provision(ctx caddy2.Context) error {
+// Provision sets up m.
+func (m *ACMEManagerMaker) Provision(ctx caddy2.Context) error {
 	// DNS providers
 	if m.Challenges.DNS != nil {
-		val, err := ctx.LoadModuleInline("provider", "tls.dns", m.Challenges.DNS)
+		val, err := ctx.LoadModuleInline("provider", "tls.dns", m.Challenges.DNSRaw)
 		if err != nil {
 			return fmt.Errorf("loading TLS storage module: %s", err)
 		}
-		m.Challenges.dns = val.(challenge.Provider)
-		m.Challenges.DNS = nil // allow GC to deallocate - TODO: Does this help?
+		m.Challenges.DNS = val.(challenge.Provider)
+		m.Challenges.DNSRaw = nil // allow GC to deallocate - TODO: Does this help?
 	}
 
 	// policy-specific storage implementation
@@ -65,14 +74,14 @@ func (m *acmeManagerMaker) Provision(ctx caddy2.Context) error {
 		m.Storage = nil // allow GC to deallocate - TODO: Does this help?
 	}
 
-	m.setDefaults()
+	m.SetDefaults()
 
 	return nil
 }
 
-// setDefaults sets necessary values that are
+// SetDefaults sets necessary values that are
 // currently empty to their default values.
-func (m *acmeManagerMaker) setDefaults() {
+func (m *ACMEManagerMaker) SetDefaults() {
 	if m.CA == "" {
 		m.CA = certmagic.LetsEncryptStagingCA // certmagic.Default.CA // TODO: When not testing, switch to production CA
 	}
@@ -93,7 +102,7 @@ func (m *acmeManagerMaker) setDefaults() {
 // makeCertMagicConfig converts m into a certmagic.Config, because
 // this is a special case where the default manager is the certmagic
 // Config and not a separate manager.
-func (m *acmeManagerMaker) makeCertMagicConfig(ctx caddy2.Context) certmagic.Config {
+func (m *ACMEManagerMaker) makeCertMagicConfig(ctx caddy2.Context) certmagic.Config {
 	storage := m.storage
 	if storage == nil {
 		storage = ctx.Storage()
@@ -115,7 +124,7 @@ func (m *acmeManagerMaker) makeCertMagicConfig(ctx caddy2.Context) certmagic.Con
 		RenewDurationBefore:     time.Duration(m.RenewAhead),
 		AltHTTPPort:             m.Challenges.HTTP.AlternatePort,
 		AltTLSALPNPort:          m.Challenges.TLSALPN.AlternatePort,
-		DNSProvider:             m.Challenges.dns,
+		DNSProvider:             m.Challenges.DNS,
 		KeyType:                 supportedCertKeyTypes[m.KeyType],
 		CertObtainTimeout:       time.Duration(m.ACMETimeout),
 		OnDemand:                ond,
@@ -133,3 +142,6 @@ var supportedCertKeyTypes = map[string]certcrypto.KeyType{
 	"P256":    certcrypto.EC256,
 	"P384":    certcrypto.EC384,
 }
+
+// Interface guard
+var _ managerMaker = (*ACMEManagerMaker)(nil)
diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go
index 38447ba..22749ae 100644
--- a/modules/caddytls/tls.go
+++ b/modules/caddytls/tls.go
@@ -45,12 +45,12 @@ func (t *TLS) Provision(ctx caddy2.Context) error {
 
 	// automation/management policies
 	for i, ap := range t.Automation.Policies {
-		val, err := ctx.LoadModuleInline("module", "tls.management", ap.Management)
+		val, err := ctx.LoadModuleInline("module", "tls.management", ap.ManagementRaw)
 		if err != nil {
 			return fmt.Errorf("loading TLS automation management module: %s", err)
 		}
-		t.Automation.Policies[i].management = val.(ManagerMaker)
-		t.Automation.Policies[i].Management = nil // allow GC to deallocate - TODO: Does this help?
+		t.Automation.Policies[i].Management = val.(managerMaker)
+		t.Automation.Policies[i].ManagementRaw = nil // allow GC to deallocate - TODO: Does this help?
 	}
 
 	// certificate loaders
@@ -164,9 +164,9 @@ func (t *TLS) getAutomationPolicyForName(name string) AutomationPolicy {
 	}
 
 	// default automation policy
-	mgmt := new(acmeManagerMaker)
-	mgmt.setDefaults()
-	return AutomationPolicy{management: mgmt}
+	mgmt := new(ACMEManagerMaker)
+	mgmt.SetDefaults()
+	return AutomationPolicy{Management: mgmt}
 }
 
 // CertificateLoader is a type that can load certificates.
@@ -183,22 +183,22 @@ type AutomationConfig struct {
 // AutomationPolicy designates the policy for automating the
 // management of managed TLS certificates.
 type AutomationPolicy struct {
-	Hosts      []string        `json:"hosts,omitempty"`
-	Management json.RawMessage `json:"management"`
+	Hosts         []string        `json:"hosts,omitempty"`
+	ManagementRaw json.RawMessage `json:"management"`
 
-	management ManagerMaker
+	Management managerMaker `json:"-"`
 }
 
 func (ap AutomationPolicy) makeCertMagicConfig(ctx caddy2.Context) certmagic.Config {
 	// default manager (ACME) is a special case because of how CertMagic is designed
 	// TODO: refactor certmagic so that ACME manager is not a special case by extracting
 	// its config fields out of the certmagic.Config struct, or something...
-	if acmeMgmt, ok := ap.management.(*acmeManagerMaker); ok {
+	if acmeMgmt, ok := ap.Management.(*ACMEManagerMaker); ok {
 		return acmeMgmt.makeCertMagicConfig(ctx)
 	}
 
 	return certmagic.Config{
-		NewManager: ap.management.newManager,
+		NewManager: ap.Management.newManager,
 	}
 }
 
@@ -206,9 +206,9 @@ func (ap AutomationPolicy) makeCertMagicConfig(ctx caddy2.Context) certmagic.Con
 type ChallengesConfig struct {
 	HTTP    HTTPChallengeConfig    `json:"http"`
 	TLSALPN TLSALPNChallengeConfig `json:"tls-alpn"`
-	DNS     json.RawMessage        `json:"dns,omitempty"`
+	DNSRaw  json.RawMessage        `json:"dns,omitempty"`
 
-	dns challenge.Provider
+	DNS challenge.Provider `json:"-"`
 }
 
 // HTTPChallengeConfig configures the ACME HTTP challenge.
@@ -232,8 +232,8 @@ type OnDemandConfig struct {
 	AskStarlark string `json:"ask_starlark,omitempty"`
 }
 
-// ManagerMaker makes a certificate manager.
-type ManagerMaker interface {
+// managerMaker makes a certificate manager.
+type managerMaker interface {
 	newManager(interactive bool) (certmagic.Manager, error)
 }