caddytls: Fix handling of IP-only TLS configs and empty-SNI handshakes (#2452)

* caddytls: Fix empty SNI handling (new -default-sni flag)

vendor: update certmagic, needed to support this

Hopefully fixes #2451, fixes #2438, and fixes #2414

* caddytls: Don't overwrite certmagic Manager (fixes #2407)

Supersedes #2447

* vendor: Update certmagic to fix nil pointer deref and TLS-ALPN cleanup

* Improve -default-sni flag help text

caddy/caddymain/run.go

@@ -46,6 +46,7 @@ func init() {
 
 	flag.BoolVar(&certmagic.Agreed, "agree", false, "Agree to the CA's Subscriber Agreement")
 	flag.StringVar(&certmagic.CA, "ca", certmagic.CA, "URL to certificate authority's ACME server directory")
+	flag.StringVar(&certmagic.DefaultServerName, "default-sni", certmagic.DefaultServerName, "If a ClientHello ServerName is empty, use this ServerName to choose a TLS certificate")
 	flag.BoolVar(&certmagic.DisableHTTPChallenge, "disable-http-challenge", certmagic.DisableHTTPChallenge, "Disable the ACME HTTP challenge")
 	flag.BoolVar(&certmagic.DisableTLSALPNChallenge, "disable-tls-alpn-challenge", certmagic.DisableTLSALPNChallenge, "Disable the ACME TLS-ALPN challenge")
 	flag.StringVar(&disabledMetrics, "disabled-metrics", "", "Comma-separated list of telemetry metrics to disable")
@@ -108,7 +109,7 @@ func Run() {
 		}
 	}
 
-	//Load all additional envs as soon as possible
+	// load all additional envs as soon as possible
 	if err := LoadEnvFromFile(envFile); err != nil {
 		mustLogFatalf("%v", err)
 	}

caddytls/config.go

@@ -93,7 +93,7 @@ type Config struct {
 }
 
 // NewConfig returns a new Config with a pointer to the instance's
-// certificate cache. You will usually need to set Other fields on
+// certificate cache. You will usually need to set other fields on
 // the returned Config for successful practical use.
 func NewConfig(inst *caddy.Instance) *Config {
 	inst.StorageMu.RLock()
@@ -257,10 +257,14 @@ func MakeTLSConfig(configs []*Config) (*tls.Config, error) {
 		// configs with the same hostname pattern; should
 		// be OK since we already asserted they are roughly
 		// the same); during TLS handshakes, configs are
-		// loaded based on the hostname pattern, according
-		// to client's SNI
+		// loaded based on the hostname pattern according
+		// to client's ServerName (SNI) value
+		if cfg.Hostname == "0.0.0.0" || cfg.Hostname == "::" {
+			configMap[""] = cfg
+		} else {
 			configMap[cfg.Hostname] = cfg
 		}
+	}
 
 	// Is TLS disabled? By now, we know that all
 	// configs agree whether it is or not, so we

caddytls/handshake.go

@@ -17,6 +17,8 @@ package caddytls
 import (
 	"crypto/tls"
 	"fmt"
+	"log"
+	"net"
 	"strings"
 
 	"github.com/mholt/caddy/telemetry"
@@ -38,14 +40,31 @@ type configGroup map[string]*Config
 // This function follows nearly the same logic to lookup
 // a hostname as the getCertificate function uses.
 func (cg configGroup) getConfig(hello *tls.ClientHelloInfo) *Config {
-	name := certmagic.CertNameFromClientHello(hello)
+	name := certmagic.NormalizedName(hello.ServerName)
+	if name == "" {
+		name = certmagic.NormalizedName(certmagic.DefaultServerName)
+	}
+
+	// if SNI is empty, prefer matching IP address (it is
+	// more specific than a "catch-all" configuration)
+	if name == "" && hello.Conn != nil {
+		addr := hello.Conn.LocalAddr().String()
+		ip, _, err := net.SplitHostPort(addr)
+		if err == nil {
+			addr = ip
+		}
+		if config, ok := cg[addr]; ok {
+			return config
+		}
+	}
 
-	// exact match? great, let's use it
+	// otherwise, try an exact match
 	if config, ok := cg[name]; ok {
 		return config
 	}
 
-	// try replacing labels in the name with wildcards until we get a match
+	// then try replacing labels in the name with
+	// wildcards until we get a match
 	labels := strings.Split(name, ".")
 	for i := range labels {
 		labels[i] = "*"
@@ -55,14 +74,25 @@ func (cg configGroup) getConfig(hello *tls.ClientHelloInfo) *Config {
 		}
 	}
 
-	// try a config that serves all names (the above
-	// loop doesn't try empty string; for hosts defined
-	// with only a port, for instance, like ":443") -
-	// also known as the default config
+	// try a config that matches all names - this
+	// is needed to match configs defined without
+	// a specific host, like ":443", when SNI is
+	// a non-empty value
 	if config, ok := cg[""]; ok {
 		return config
 	}
 
+	// failover with a random config: this is necessary
+	// because we might be needing to solve a TLS-ALPN
+	// ACME challenge for a name that we don't have a
+	// TLS configuration for; any config will do for
+	// this purpose
+	for _, config := range cg {
+		return config
+	}
+
+	log.Printf("[ERROR] No TLS configuration available for ClientHello with ServerName: %s", hello.ServerName)
+
 	return nil
 }
 

caddytls/setup.go

@@ -54,18 +54,6 @@ func setupTLS(c *caddy.Controller) error {
 
 	config.Enabled = true
 
-	// a single certificate cache is used by the whole caddy.Instance; get a pointer to it
-	certCache, ok := c.Get(CertCacheInstStorageKey).(*certmagic.Cache)
-	if !ok || certCache == nil {
-		certCache = certmagic.NewCache(certmagic.DefaultStorage)
-		c.OnShutdown(func() error {
-			certCache.Stop()
-			return nil
-		})
-		c.Set(CertCacheInstStorageKey, certCache)
-	}
-	config.Manager = certmagic.NewWithCache(certCache, certmagic.Config{})
-
 	// we use certmagic events to collect metrics for telemetry
 	config.Manager.OnEvent = func(event string, data interface{}) {
 		switch event {

caddytls/setup_test.go

@@ -47,11 +47,18 @@ func TestMain(m *testing.M) {
 }
 
 func TestSetupParseBasic(t *testing.T) {
-	cfg := &Config{Manager: &certmagic.Config{}}
+	tmpdir, err := ioutil.TempDir("", "caddytls_setup_test_")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmpdir)
+
+	certCache := certmagic.NewCache(&certmagic.FileStorage{Path: tmpdir})
+	cfg := &Config{Manager: certmagic.NewWithCache(certCache, certmagic.Config{})}
 	RegisterConfigGetter("", func(c *caddy.Controller) *Config { return cfg })
 	c := caddy.NewTestController("", `tls `+certFile+` `+keyFile+``)
 
-	err := setupTLS(c)
+	err = setupTLS(c)
 	if err != nil {
 		t.Errorf("Expected no errors, got: %v", err)
 	}
@@ -126,11 +133,18 @@ func TestSetupParseWithOptionalParams(t *testing.T) {
             alpn http/1.1
         }`
 
-	cfg := &Config{Manager: &certmagic.Config{}}
+	tmpdir, err := ioutil.TempDir("", "caddytls_setup_test_")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmpdir)
+
+	certCache := certmagic.NewCache(&certmagic.FileStorage{Path: tmpdir})
+	cfg := &Config{Manager: certmagic.NewWithCache(certCache, certmagic.Config{})}
 	RegisterConfigGetter("", func(c *caddy.Controller) *Config { return cfg })
 	c := caddy.NewTestController("", params)
 
-	err := setupTLS(c)
+	err = setupTLS(c)
 	if err != nil {
 		t.Errorf("Expected no errors, got: %v", err)
 	}

vendor/github.com/mholt/certmagic/certificates.go

@@ -315,7 +315,7 @@ func (cfg *Config) cacheCertificate(cert Certificate) Certificate {
 	// (yes, if certs overlap in the names they serve, one will
 	// overwrite another here, but that's just how it goes)
 	for _, name := range cert.Names {
-		cfg.certificates[name] = cert.Hash
+		cfg.certificates[NormalizedName(name)] = cert.Hash
 	}
 
 	// store the certificate

vendor/github.com/mholt/certmagic/certmagic.go

@@ -484,6 +484,12 @@ var (
 	// the risk of rate limiting.
 	CertObtainTimeout time.Duration
 
+	// Set the default server name for clients
+	// not indicating a server name using SNI.
+	// In most cases this will be the primary
+	// domain that is being served.
+	DefaultServerName string
+
 	// The state needed to operate on-demand TLS
 	OnDemand *OnDemandConfig
 

vendor/github.com/mholt/certmagic/client.go

@@ -215,7 +215,7 @@ func (cfg *Config) newACMEClient(interactive bool) (*acmeClient, error) {
 // lockKey returns a key for a lock that is specific to the operation
 // named op being performed related to domainName and this config's CA.
 func (cfg *Config) lockKey(op, domainName string) string {
-	return fmt.Sprintf("%s:%s:%s", op, domainName, cfg.CA)
+	return fmt.Sprintf("%s_%s_%s", op, domainName, cfg.CA)
 }
 
 // Obtain obtains a single certificate for name. It stores the certificate

vendor/github.com/mholt/certmagic/config.go

@@ -101,6 +101,11 @@ type Config struct {
 	// the risk of rate limiting.
 	CertObtainTimeout time.Duration
 
+	// DefaultServerName specifies a server name
+	// to use when choosing a certificate if the
+	// ClientHello's ServerName field is empty
+	DefaultServerName string
+
 	// The state needed to operate on-demand TLS
 	OnDemand *OnDemandConfig
 
@@ -207,6 +212,9 @@ func NewWithCache(certCache *Cache, cfg Config) *Config {
 	if cfg.CertObtainTimeout == 0 {
 		cfg.CertObtainTimeout = CertObtainTimeout
 	}
+	if cfg.DefaultServerName == "" {
+		cfg.DefaultServerName = DefaultServerName
+	}
 	if cfg.OnDemand == nil {
 		cfg.OnDemand = OnDemand
 	}

vendor/github.com/mholt/certmagic/maintain.go

@@ -35,6 +35,8 @@ func (certCache *Cache) maintainAssets() {
 	renewalTicker := time.NewTicker(certCache.RenewInterval)
 	ocspTicker := time.NewTicker(certCache.OCSPInterval)
 
+	log.Printf("[INFO][%s] Started certificate maintenance routine", certCache.storage)
+
 	for {
 		select {
 		case <-renewalTicker.C:

vendor/github.com/mholt/certmagic/solvers.go

@@ -50,7 +50,7 @@ func (s tlsALPNSolver) Present(domain, token, keyAuth string) error {
 // CleanUp removes the challenge certificate from the cache.
 func (s tlsALPNSolver) CleanUp(domain, token, keyAuth string) error {
 	s.certCache.mu.Lock()
-	delete(s.certCache.cache, domain)
+	delete(s.certCache.cache, tlsALPNCertKeyName(domain))
 	s.certCache.mu.Unlock()
 	return nil
 }

vendor/manifest

@@ -138,7 +138,7 @@
 			"importpath": "github.com/mholt/certmagic",
 			"repository": "https://github.com/mholt/certmagic",
 			"vcs": "git",
-			"revision": "c1d472b46046ee329c099086d689ada0c44d56b0",
+			"revision": "a7f18a937c080b88693cd4e14d48e42cc067b268",
 			"branch": "master",
 			"notests": true
 		},