caddytls: Raise TLS alert if no certificate matches SAN (closes #1303) (#2339)

* caddytls: Raise TLS alert if no certificate matches SAN (closes #1303)

I don't love this half-baked solution to the issue raised in #1303 way
more than a year after the original issue was closed (the necro comments
are about an issue separate from the original issue that started it),
but I do like TLS alerts more than wrong certificates.

* Restore test to match

* Restore another previous test

caddytls/certificates_test.go

@@ -43,15 +43,9 @@ 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)
 	}
 
-	// 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 {
-		t.Errorf("Expected matched=false, defaulted=true; but got matched=%v, defaulted=%v (cert: %v)", matched, defaulted, cert)
+	// 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)
 	}
 }
 

caddytls/config.go

@@ -576,7 +576,7 @@ var supportedKeyTypes = map[string]acme.KeyType{
 	"RSA2048": acme.RSA2048,
 }
 
-// Map of supported protocols.
+// SupportedProtocols is a map of supported protocols.
 // HTTP/2 only supports TLS 1.2 and higher.
 // If updating this map, also update tlsProtocolStringToMap in caddyhttp/fastcgi/fastcgi.go
 var SupportedProtocols = map[string]uint16{
@@ -596,7 +596,7 @@ func GetSupportedProtocolName(protocol uint16) (string, error) {
 	return "", errors.New("name: unsuported protocol")
 }
 
-// Map of supported ciphers, used only for parsing config.
+// SupportedCiphersMap has supported ciphers, used only for parsing config.
 //
 // Note that, at time of writing, HTTP/2 blacklists 276 cipher suites,
 // including all but four of the suites below (the four GCM suites).

caddytls/crypto.go

@@ -177,10 +177,7 @@ func stapleOCSP(cert *Certificate, pemBundle []byte) error {
 	return nil
 }
 
-// makeSelfSignedCert makes a self-signed certificate according
-// to the parameters in config. It then caches the certificate
-// in our cache.
-func makeSelfSignedCert(config *Config) error {
+func makeSelfSignedCertWithCustomSAN(sans []string, config *Config) (Certificate, error) {
 	// start by generating private key
 	var privKey interface{}
 	var err error
@@ -196,10 +193,10 @@ func makeSelfSignedCert(config *Config) error {
 	case acme.RSA8192:
 		privKey, err = rsa.GenerateKey(rand.Reader, 8192)
 	default:
-		return fmt.Errorf("cannot generate private key; unknown key type %v", config.KeyType)
+		return Certificate{}, fmt.Errorf("cannot generate private key; unknown key type %v", config.KeyType)
 	}
 	if err != nil {
-		return fmt.Errorf("failed to generate private key: %v", err)
+		return Certificate{}, fmt.Errorf("failed to generate private key: %v", err)
 	}
 
 	// create certificate structure with proper values
@@ -208,7 +205,7 @@ func makeSelfSignedCert(config *Config) error {
 	serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128)
 	serialNumber, err := rand.Int(rand.Reader, serialNumberLimit)
 	if err != nil {
-		return fmt.Errorf("failed to generate serial number: %v", err)
+		return Certificate{}, fmt.Errorf("failed to generate serial number: %v", err)
 	}
 	cert := &x509.Certificate{
 		SerialNumber: serialNumber,
@@ -218,13 +215,18 @@ func makeSelfSignedCert(config *Config) error {
 		KeyUsage:     x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature,
 		ExtKeyUsage:  []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
 	}
+	if len(sans) == 0 {
+		sans = []string{""}
+	}
 	var names []string
-	if ip := net.ParseIP(config.Hostname); ip != nil {
+	for _, san := range sans {
+		if ip := net.ParseIP(san); ip != nil {
 			names = append(names, strings.ToLower(ip.String()))
 			cert.IPAddresses = append(cert.IPAddresses, ip)
 		} else {
-		names = append(names, strings.ToLower(config.Hostname))
-		cert.DNSNames = append(cert.DNSNames, strings.ToLower(config.Hostname))
+			names = append(names, strings.ToLower(san))
+			cert.DNSNames = append(cert.DNSNames, strings.ToLower(san))
+		}
 	}
 
 	publicKey := func(privKey interface{}) interface{} {
@@ -239,12 +241,12 @@ func makeSelfSignedCert(config *Config) error {
 	}
 	derBytes, err := x509.CreateCertificate(rand.Reader, cert, cert, publicKey(privKey), privKey)
 	if err != nil {
-		return fmt.Errorf("could not create certificate: %v", err)
+		return Certificate{}, fmt.Errorf("could not create certificate: %v", err)
 	}
 
 	chain := [][]byte{derBytes}
 
-	config.cacheCertificate(Certificate{
+	return Certificate{
 		Certificate: tls.Certificate{
 			Certificate: chain,
 			PrivateKey:  privKey,
@@ -253,8 +255,17 @@ func makeSelfSignedCert(config *Config) error {
 		Names:    names,
 		NotAfter: cert.NotAfter,
 		Hash:     hashCertificateChain(chain),
-	})
+	}, nil
+}
 
+// makeSelfSignedCertForConfig makes a self-signed certificate according
+// to the parameters in config and caches the new cert in config directly.
+func makeSelfSignedCertForConfig(config *Config) error {
+	cert, err := makeSelfSignedCertWithCustomSAN([]string{config.Hostname}, config)
+	if err != nil {
+		return err
+	}
+	config.cacheCertificate(cert)
 	return nil
 }
 

caddytls/handshake.go

@@ -63,16 +63,12 @@ func (cg configGroup) getConfig(name string) *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")
+	// with only a port, for instance, like ":443") -
+	// also known as the default config
 	if config, ok := cg[""]; ok {
 		return config
 	}
 
-	// no matches, so just serve up a random config
-	for _, config := range cg {
-		return config
-	}
-
 	return nil
 }
 
@@ -187,16 +183,12 @@ func (cfg *Config) getCertificate(name string) (cert Certificate, matched, defau
 		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 {
+	// if nothing matches, use a "default" certificate
+	// (See issues 2035 and 1303; 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.)
+	if certKey, ok := cfg.Certificates[""]; ok {
 		cert = cfg.certCache.cache[certKey]
 		defaulted = true
 		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"} // TODO (see below)
+	helloNoMatch := &tls.ClientHelloInfo{ServerName: "nomatch"} // TODO (see below)
 
 	// When cache is empty
 	if cert, err := cfg.GetCertificate(hello); err == nil {
@@ -69,9 +69,8 @@ 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)
+	}
 }

caddytls/setup.go

@@ -282,7 +282,7 @@ func setupTLS(c *caddy.Controller) error {
 
 	// generate self-signed cert if needed
 	if config.SelfSigned {
-		err := makeSelfSignedCert(config)
+		err := makeSelfSignedCertForConfig(config)
 		if err != nil {
 			return fmt.Errorf("self-signed: %v", err)
 		}