Partially fix raw IP request regression (#2356)

caddyhttp/httpserver/plugin.go

@@ -391,6 +391,12 @@ func groupSiteConfigsByListenAddr(configs []*SiteConfig) (map[string][]*SiteConf
 // parts of an address. The component parts may be
 // updated to the correct values as setup proceeds,
 // but the original value should never be changed.
+//
+// Important: In order for the fix for #2356 to work,
+// if Host is an IP address, it must be in a
+// normalized form (as presented by net.IP.String()).
+// For SNI matching in general, it needs to be
+// lowercase.
 type Address struct {
 	Original, Scheme, Host, Port, Path string
 }
@@ -439,10 +445,17 @@ func (a Address) Normalize() Address {
 	if !CaseSensitivePath {
 		path = strings.ToLower(path)
 	}
+
+	host := a.Host
+	if ip := net.ParseIP(host); ip != nil {
+		// Ensure that it's in a normalized form if it's an IP address.
+		host = ip.String()
+	}
+
 	return Address{
 		Original: a.Original,
 		Scheme:   strings.ToLower(a.Scheme),
-		Host:     strings.ToLower(a.Host),
+		Host:     strings.ToLower(host),
 		Port:     a.Port,
 		Path:     path,
 	}

caddytls/config.go

@@ -35,6 +35,12 @@ type Config struct {
 	// designated for; can contain wildcard characters
 	// according to RFC 6125 §6.4.3 - this field MUST
 	// be set in order for things to work as expected
+	//
+	// Important: In order for the fix for #2356 to work,
+	// if Host is an IP address, it must be in a
+	// normalized form (as presented by net.IP.String()).
+	// For SNI matching in general, it needs to be
+	// lowercase.
 	Hostname string
 
 	// Whether TLS is enabled

caddytls/handshake.go

@@ -19,6 +19,7 @@ import (
 	"errors"
 	"fmt"
 	"log"
+	"net"
 	"net/http"
 	"net/url"
 	"strings"
@@ -34,18 +35,28 @@ import (
 // method to get a config by matching its hostname).
 type configGroup map[string]*Config
 
-// getConfig gets the config by the first key match for name.
-// In other words, "sub.foo.bar" will get the config for "*.foo.bar"
-// if that is the closest match. If no match is found, the first
-// (random) config will be loaded, which will defer any TLS alerts
-// to the certificate validation (this may or may not be ideal;
-// let's talk about it if this becomes problematic).
-//
-// This function follows nearly the same logic to lookup
-// a hostname as the getCertificate function uses.
-func (cg configGroup) getConfig(name string) *Config {
+func certificateIDFromHello(clientHello *tls.ClientHelloInfo) (name string) {
+	name = strings.TrimSpace(clientHello.ServerName)
+
+	// Per #2356, if lacking SNI, attempt to find certificate
+	// that matches local IP address.
+	if name == "" {
+		addr := clientHello.Conn.LocalAddr()
+		if s, ok := addr.(*net.TCPAddr); ok {
+			name = s.IP.String()
+		} else if s, ok := addr.(*net.UDPAddr); ok {
+			name = s.IP.String()
+		} else if s, ok := addr.(*net.IPAddr); ok {
+			name = s.IP.String()
+		}
+	}
+
+	// IP addresses should be lowercased, too
 	name = strings.ToLower(name)
+	return
+}
 
+func (cg configGroup) getConfig(name string) *Config {
 	// exact match? great, let's use it
 	if config, ok := cg[name]; ok {
 		return config
@@ -79,8 +90,8 @@ func (cg configGroup) getConfig(name string) *Config {
 //
 // This method is safe for use as a tls.Config.GetConfigForClient callback.
 func (cg configGroup) GetConfigForClient(clientHello *tls.ClientHelloInfo) (*tls.Config, error) {
-	config := cg.getConfig(clientHello.ServerName)
-	if config != nil {
+	name := certificateIDFromHello(clientHello)
+	if config := cg.getConfig(name); config != nil {
 		return config.tlsConfig, nil
 	}
 	return nil, nil
@@ -112,7 +123,8 @@ func (cfg *Config) GetCertificate(clientHello *tls.ClientHelloInfo) (*tls.Certif
 	}
 
 	// get the certificate and serve it up
-	cert, err := cfg.getCertDuringHandshake(strings.ToLower(clientHello.ServerName), true, true)
+	name := certificateIDFromHello(clientHello)
+	cert, err := cfg.getCertDuringHandshake(name, true, true)
 	if err == nil {
 		go telemetry.Increment("tls_handshake_count") // TODO: This is a "best guess" for now, we need something listener-level
 	}

caddytls/handshake_test.go

@@ -17,17 +17,61 @@ package caddytls
 import (
 	"crypto/tls"
 	"crypto/x509"
+	"errors"
+	"net"
 	"testing"
+	"time"
 )
 
+// Mock net.Conn. Only job is to expose a local IP address.
+type mockConn struct {
+	Addr net.Addr
+}
+
+func (conn mockConn) Read(b []byte) (n int, err error) {
+	return -1, errors.New("not implemented")
+}
+
+func (conn mockConn) Write(b []byte) (n int, err error) {
+	return -1, errors.New("not implemented")
+}
+
+func (conn mockConn) Close() error {
+	return errors.New("not implemented")
+}
+
+func (conn mockConn) LocalAddr() net.Addr {
+	return conn.Addr
+}
+
+func (conn mockConn) RemoteAddr() net.Addr {
+	return nil
+}
+
+func (conn mockConn) SetDeadline(t time.Time) error {
+	return errors.New("not implemented")
+}
+
+func (conn mockConn) SetReadDeadline(t time.Time) error {
+	return errors.New("not implemented")
+}
+
+func (conn mockConn) SetWriteDeadline(t time.Time) error {
+	return errors.New("not implemented")
+}
+
 func TestGetCertificate(t *testing.T) {
 	certCache := &certificateCache{cache: make(map[string]Certificate)}
 	cfg := &Config{Certificates: make(map[string]string), certCache: certCache}
 
-	hello := &tls.ClientHelloInfo{ServerName: "example.com"}
-	helloSub := &tls.ClientHelloInfo{ServerName: "sub.example.com"}
-	helloNoSNI := &tls.ClientHelloInfo{}
-	helloNoMatch := &tls.ClientHelloInfo{ServerName: "nomatch"} // TODO (see below)
+	var conn1 net.Conn = &mockConn{Addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 60001, Zone: ""}}
+	var conn2 net.Conn = &mockConn{Addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.2"), Port: 60001, Zone: ""}}
+
+	hello := &tls.ClientHelloInfo{Conn: conn1, ServerName: "example.com"}
+	helloSub := &tls.ClientHelloInfo{Conn: conn1, ServerName: "sub.example.com"}
+	helloNoSNI := &tls.ClientHelloInfo{Conn: conn1}
+	helloNoSNIFallback := &tls.ClientHelloInfo{Conn: conn2}
+	helloNoMatch := &tls.ClientHelloInfo{Conn: conn1, ServerName: "nomatch"} // TODO (see below)
 
 	// When cache is empty
 	if cert, err := cfg.GetCertificate(hello); err == nil {
@@ -45,8 +89,8 @@ func TestGetCertificate(t *testing.T) {
 	} else if cert.Leaf.DNSNames[0] != "example.com" {
 		t.Errorf("Got wrong certificate with exact match; expected 'example.com', got: %v", cert)
 	}
-	if _, err := cfg.GetCertificate(helloNoSNI); err != nil {
-		t.Errorf("Got an error with no SNI but shouldn't have, when cert exists in cache: %v", err)
+	if _, err := cfg.GetCertificate(helloNoSNI); err == nil {
+		t.Error("Expected error with no SNI and single cert in cache")
 	}
 
 	// When retrieving wildcard certificate
@@ -63,14 +107,30 @@ func TestGetCertificate(t *testing.T) {
 	}
 
 	// When cache is NOT empty but there's no SNI
-	if cert, err := cfg.GetCertificate(helloNoSNI); err != nil {
-		t.Errorf("Expected random certificate with no error when no SNI, got err: %v", err)
-	} else if cert == nil || len(cert.Leaf.DNSNames) == 0 {
-		t.Errorf("Expected random cert with no matches, got: %v", cert)
+	if _, err := cfg.GetCertificate(helloNoSNI); err == nil {
+		t.Error("Expected error with no SNI multiple certs in cache")
 	}
 
 	// 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)
+		t.Error("Expected an error when no certificate matched the SNI")
+	}
+
+	// When no SNI, find a certificate with a matching IP address
+	ipCert := Certificate{
+		Names:       []string{"127.0.0.1"},
+		Certificate: tls.Certificate{Leaf: &x509.Certificate{IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}}},
+		Hash:        "127.0.0.1",
+	}
+	cfg.cacheCertificate(ipCert)
+	if cert, err := cfg.GetCertificate(helloNoSNI); err != nil {
+		t.Errorf("Got an error but shouldn't have, when no SNI and cert for IP address exists in cache: %v", err)
+	} else if cert == nil || len(cert.Leaf.IPAddresses) == 0 || !cert.Leaf.IPAddresses[0].Equal(net.ParseIP("127.0.0.1")) {
+		t.Errorf("Got wrong cert when no SNI and cert for IP address exists in cache: %v", err)
+	}
+
+	// Raise an alert when no SNI and no matching IP address.
+	if _, err := cfg.GetCertificate(helloNoSNIFallback); err == nil {
+		t.Error("Expected an error when no certificate matched the IP address with no SNI")
 	}
 }