Optionally enforce strict TLS SNI + HTTP Host matching, & misc. cleanup

We should look into a way to enable this by default when TLS client auth
is configured for a server

3 files changed, 47 insertions, 12 deletions

diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go
index 8033b91..00bb8ba 100644
--- a/modules/caddyhttp/routes.go
+++ b/modules/caddyhttp/routes.go
@@ -162,11 +162,11 @@ func (routes RouteList) BuildCompositeRoute(rw http.ResponseWriter, req *http.Re
 }
 
 // wrapMiddleware wraps m such that it can be correctly
-// appended to a list of middleware. This is necessary
-// so that only the last middleware in a loop does not
-// become the only middleware of the stack, repeatedly
-// executed (i.e. it is necessary to keep a reference
-// to this m outside of the scope of a loop)!
+// appended to a list of middleware. This separate closure
+// is necessary so that only the last middleware in a loop
+// does not become the only middleware of the stack,
+// repeatedly executed (i.e. it is necessary to keep a
+// reference to this m outside of the scope of a loop)!
 func wrapMiddleware(m MiddlewareHandler) Middleware {
 	return func(next HandlerFunc) HandlerFunc {
 		return func(w http.ResponseWriter, r *http.Request) error {
diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go
index 05763ba..9f08d1f 100644
--- a/modules/caddyhttp/server.go
+++ b/modules/caddyhttp/server.go
@@ -7,6 +7,7 @@ import (
 	"net"
 	"net/http"
 	"strconv"
+	"strings"
 
 	"github.com/caddyserver/caddy"
 	"github.com/caddyserver/caddy/modules/caddytls"
@@ -25,6 +26,7 @@ type Server struct {
 	TLSConnPolicies   caddytls.ConnectionPolicies `json:"tls_connection_policies,omitempty"`
 	AutoHTTPS         *AutoHTTPSConfig            `json:"automatic_https,omitempty"`
 	MaxRehandles      int                         `json:"max_rehandles,omitempty"`
+	StrictSNIHost     bool                        `json:"strict_sni_host,omitempty"` // TODO: see if we can turn this on by default when clientauth is configured
 
 	tlsApp *caddytls.TLS
 }
@@ -47,8 +49,9 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	addHTTPVarsToReplacer(repl, r, w)
 
 	// build and execute the main handler chain
-	stack, w := s.Routes.BuildCompositeRoute(w, r)
-	err := s.executeCompositeRoute(w, r, stack)
+	stack, wrappedWriter := s.Routes.BuildCompositeRoute(w, r)
+	stack = s.wrapPrimaryRoute(stack)
+	err := s.executeCompositeRoute(wrappedWriter, r, stack)
 	if err != nil {
 		// add the raw error value to the request context
 		// so it can be accessed by error handlers
@@ -66,8 +69,8 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 
 		if s.Errors != nil && len(s.Errors.Routes) > 0 {
-			errStack, w := s.Errors.Routes.BuildCompositeRoute(w, r)
-			err := s.executeCompositeRoute(w, r, errStack)
+			errStack, wrappedWriter := s.Errors.Routes.BuildCompositeRoute(w, r)
+			err := s.executeCompositeRoute(wrappedWriter, r, errStack)
 			if err != nil {
 				// TODO: what should we do if the error handler has an error?
 				log.Printf("[ERROR] [%s %s] handling error: %v", r.Method, r.RequestURI, err)
@@ -104,6 +107,37 @@ func (s *Server) executeCompositeRoute(w http.ResponseWriter, r *http.Request, s
 	return err
 }
 
+// wrapPrimaryRoute wraps stack (a compiled middleware handler chain)
+// in s.enforcementHandler which performs crucial security checks, etc.
+func (s *Server) wrapPrimaryRoute(stack Handler) Handler {
+	return HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
+		return s.enforcementHandler(w, r, stack)
+	})
+}
+
+// enforcementHandler is an implicit middleware which performs
+// standard checks before executing the HTTP middleware chain.
+func (s *Server) enforcementHandler(w http.ResponseWriter, r *http.Request, next Handler) error {
+	// enforce strict host matching, which ensures that the SNI
+	// value (if any), matches the Host header; essential for
+	// servers that rely on TLS ClientAuth sharing a listener
+	// with servers that do not; if not enforced, client could
+	// bypass by sending benign SNI then restricted Host header
+	if s.StrictSNIHost && r.TLS != nil {
+		hostname, _, err := net.SplitHostPort(r.Host)
+		if err != nil {
+			hostname = r.Host // OK; probably lacked port
+		}
+		if strings.ToLower(r.TLS.ServerName) != strings.ToLower(hostname) {
+			err := fmt.Errorf("strict host matching: TLS ServerName (%s) and HTTP Host (%s) values differ",
+				r.TLS.ServerName, hostname)
+			r.Close = true
+			return Error(http.StatusForbidden, err)
+		}
+	}
+	return next.ServeHTTP(w, r)
+}
+
 func (s *Server) listenersUseAnyPortOtherThan(otherPort int) bool {
 	for _, lnAddr := range s.Listen {
 		_, addrs, err := parseListenAddr(lnAddr)
diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go
index 7f5b1e9..7b8e420 100644
--- a/modules/caddytls/tls.go
+++ b/modules/caddytls/tls.go
@@ -88,15 +88,16 @@ func (t *TLS) Provision(ctx caddy.Context) error {
 
 // Start activates the TLS module.
 func (t *TLS) Start() error {
+	magic := certmagic.New(t.certCache, certmagic.Config{
+		Storage: t.ctx.Storage(),
+	})
+
 	// load manual/static (unmanaged) certificates
 	for _, loader := range t.certificateLoaders {
 		certs, err := loader.LoadCertificates()
 		if err != nil {
 			return fmt.Errorf("loading certificates: %v", err)
 		}
-		magic := certmagic.New(t.certCache, certmagic.Config{
-			Storage: t.ctx.Storage(),
-		})
 		for _, cert := range certs {
 			err := magic.CacheUnmanagedTLSCertificate(cert.Certificate, cert.Tags)
 			if err != nil {