From 2b22d2e6ea7ffd17ae769bd8a2adae60e5a7d0bf Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 26 Jun 2019 16:03:29 -0600 Subject: [PATCH] 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 --- context.go | 4 ++-- modules/caddyhttp/routes.go | 10 ++++----- modules/caddyhttp/server.go | 42 +++++++++++++++++++++++++++++++++---- modules/caddytls/tls.go | 7 ++++--- 4 files changed, 49 insertions(+), 14 deletions(-) diff --git a/context.go b/context.go index 9661aca4d..d09a979b3 100644 --- a/context.go +++ b/context.go @@ -13,8 +13,8 @@ import ( // Context is a type which defines the lifetime of modules that // are loaded and provides access to the parent configuration // that spawned the modules which are loaded. It should be used -// with care and only wrapped with derivation functions from -// the standard context package if you don't need the Caddy +// with care and wrapped with derivation functions from the +// standard context package only if you don't need the Caddy // specific features. These contexts are cancelled when the // lifetime of the modules loaded from it are over. // diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go index 8033b914a..00bb8bafe 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 05763ba79..9f08d1ff7 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 7f5b1e911..7b8e420d0 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 {