tls: Fall back to certificate keyed by empty name (fixes #2035) (#2037)

* tls: Fall back to certificate keyed by empty name (fixes #2035)

This should only happen for sites defined with an empty hostname (like
":8080") and which are using self-signed certificates or some other
funky self-managed certificate. But that certificate should arguably
be used for all incoming SNI names.

* tls: Revert to serving any certificate if no match, regardless of SNI

Also fix self-signed certs to include IP addresses in their name
if they are configured to serve an IP address

* Remove tests which are now irrelevant (behavior reverted)

It would be good to revisit this in the future.

caddytls/certificates.go

@@ -261,21 +261,21 @@ func fillCertFromLeaf(cert *Certificate, tlsCert tls.Certificate) error {
 		return err
 	}
 
-	if leaf.Subject.CommonName != "" {
+	if leaf.Subject.CommonName != "" { // TODO: CommonName is deprecated
 		cert.Names = []string{strings.ToLower(leaf.Subject.CommonName)}
 	}
 	for _, name := range leaf.DNSNames {
-		if name != leaf.Subject.CommonName {
+		if name != leaf.Subject.CommonName { // TODO: CommonName is deprecated
 			cert.Names = append(cert.Names, strings.ToLower(name))
 		}
 	}
 	for _, ip := range leaf.IPAddresses {
-		if ipStr := ip.String(); ipStr != leaf.Subject.CommonName {
+		if ipStr := ip.String(); ipStr != leaf.Subject.CommonName { // TODO: CommonName is deprecated
 			cert.Names = append(cert.Names, strings.ToLower(ipStr))
 		}
 	}
 	for _, email := range leaf.EmailAddresses {
-		if email != leaf.Subject.CommonName {
+		if email != leaf.Subject.CommonName { // TODO: CommonName is deprecated
 			cert.Names = append(cert.Names, strings.ToLower(email))
 		}
 	}

caddytls/certificates_test.go

@@ -43,10 +43,11 @@ func TestUnexportedGetCertificate(t *testing.T) {
 		t.Errorf("Didn't get wildcard cert for 'sub.example.com' or got the wrong one: %v, matched=%v, defaulted=%v", cert, matched, defaulted)
 	}
 
-	// When no certificate matches and SNI is provided, return no certificate (should be TLS alert)
-	if cert, matched, defaulted := cfg.getCertificate("nomatch"); matched || defaulted {
-		t.Errorf("Expected matched=false, defaulted=false; but got matched=%v, defaulted=%v (cert: %v)", matched, defaulted, cert)
-	}
+	// TODO: Re-implement this behavior when I'm not in the middle of upgrading for ACMEv2 support. :) (it was reverted in #2037)
+	// // When no certificate matches and SNI is provided, return no certificate (should be TLS alert)
+	// if cert, matched, defaulted := cfg.getCertificate("nomatch"); matched || defaulted {
+	// 	t.Errorf("Expected matched=false, defaulted=false; but got matched=%v, defaulted=%v (cert: %v)", matched, defaulted, cert)
+	// }
 
 	// When no certificate matches and SNI is NOT provided, a random is returned
 	if cert, matched, defaulted := cfg.getCertificate(""); matched || !defaulted {

caddytls/crypto.go

@@ -35,6 +35,7 @@ import (
 	"net"
 	"os"
 	"path/filepath"
+	"strings"
 	"sync"
 	"time"
 
@@ -216,10 +217,13 @@ func makeSelfSignedCert(config *Config) error {
 		KeyUsage:     x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
 		ExtKeyUsage:  []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
 	}
+	var names []string
 	if ip := net.ParseIP(config.Hostname); ip != nil {
+		names = append(names, strings.ToLower(ip.String()))
 		cert.IPAddresses = append(cert.IPAddresses, ip)
 	} else {
-		cert.DNSNames = append(cert.DNSNames, config.Hostname)
+		names = append(names, strings.ToLower(config.Hostname))
+		cert.DNSNames = append(cert.DNSNames, strings.ToLower(config.Hostname))
 	}
 
 	publicKey := func(privKey interface{}) interface{} {
@@ -245,7 +249,7 @@ func makeSelfSignedCert(config *Config) error {
 			PrivateKey:  privKey,
 			Leaf:        cert,
 		},
-		Names:    cert.DNSNames,
+		Names:    names,
 		NotAfter: cert.NotAfter,
 		Hash:     hashCertificateChain(chain),
 	})

caddytls/handshake.go

@@ -59,10 +59,9 @@ func (cg configGroup) getConfig(name string) *Config {
 		}
 	}
 
-	// try a config that serves all names (this
-	// is basically the same as a config defined
-	// for "*" -- I think -- but the above loop
-	// doesn't try an empty string)
+	// 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")
 	if config, ok := cg[""]; ok {
 		return config
 	}
@@ -166,17 +165,19 @@ func (cfg *Config) getCertificate(name string) (cert Certificate, matched, defau
 		return
 	}
 
-	// if nothing matches and SNI was not provided, use a random
-	// certificate; at least there's a chance this older client
-	// can connect, and in the future we won't need this provision
-	// (if SNI is present, it's probably best to just raise a TLS
-	// alert by not serving a certificate)
-	if name == "" {
-		for _, certKey := range cfg.Certificates {
-			defaulted = true
-			cert = cfg.certCache.cache[certKey]
-			return
-		}
+	// if nothing matches, use a random certificate
+	// TODO: This is not my favorite behavior; I would rather serve
+	// no certificate if SNI is provided and cause a TLS alert, than
+	// serve the wrong certificate (but sometimes the 'wrong' cert
+	// is what is wanted, but in those cases I would prefer that the
+	// site owner explicitly configure a "default" certificate).
+	// (See issue 2035; any change to this behavior must account for
+	// hosts defined like ":443" or "0.0.0.0:443" where the hostname
+	// is empty or a catch-all IP or something.)
+	for _, certKey := range cfg.Certificates {
+		cert = cfg.certCache.cache[certKey]
+		defaulted = true
+		return
 	}
 
 	return

caddytls/handshake_test.go

@@ -27,7 +27,7 @@ func TestGetCertificate(t *testing.T) {
 	hello := &tls.ClientHelloInfo{ServerName: "example.com"}
 	helloSub := &tls.ClientHelloInfo{ServerName: "sub.example.com"}
 	helloNoSNI := &tls.ClientHelloInfo{}
-	helloNoMatch := &tls.ClientHelloInfo{ServerName: "nomatch"}
+	// helloNoMatch := &tls.ClientHelloInfo{ServerName: "nomatch"} // TODO (see below)
 
 	// When cache is empty
 	if cert, err := cfg.GetCertificate(hello); err == nil {
@@ -69,8 +69,9 @@ func TestGetCertificate(t *testing.T) {
 		t.Errorf("Expected random cert with no matches, got: %v", cert)
 	}
 
+	// TODO: Re-implement this behavior (it was reverted in #2037)
 	// When no certificate matches, raise an alert
-	if _, err := cfg.GetCertificate(helloNoMatch); err == nil {
-		t.Errorf("Expected an error when no certificate matched the SNI, got: %v", err)
-	}
+	// if _, err := cfg.GetCertificate(helloNoMatch); err == nil {
+	// 	t.Errorf("Expected an error when no certificate matched the SNI, got: %v", err)
+	// }
 }