Merge branch 'remove-ntlm'

4 files changed, 120 insertions, 291 deletions

diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go
index cefb5b6..9636936 100644
--- a/modules/caddyhttp/reverseproxy/caddyfile.go
+++ b/modules/caddyhttp/reverseproxy/caddyfile.go
@@ -551,26 +551,20 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 
 	// verify transport configuration, and finally encode it
 	if transport != nil {
-		// TODO: these two cases are identical, but I don't know how to reuse the code
-		switch ht := transport.(type) {
-		case *HTTPTransport:
-			if commonScheme == "https" && ht.TLS == nil {
-				ht.TLS = new(TLSConfig)
-			}
-			if ht.TLS != nil && commonScheme == "http" {
-				return d.Errf("upstream address scheme is HTTP but transport is configured for HTTP+TLS (HTTPS)")
-			}
-
-		case *NTLMTransport:
-			if commonScheme == "https" && ht.TLS == nil {
-				ht.TLS = new(TLSConfig)
+		if te, ok := transport.(TLSTransport); ok {
+			if commonScheme == "https" && !te.TLSEnabled() {
+				err := te.EnableTLS(new(TLSConfig))
+				if err != nil {
+					return err
+				}
 			}
-			if ht.TLS != nil && commonScheme == "http" {
+			if commonScheme == "http" && te.TLSEnabled() {
 				return d.Errf("upstream address scheme is HTTP but transport is configured for HTTP+TLS (HTTPS)")
 			}
+		} else if commonScheme == "https" {
+			return d.Errf("upstreams are configured for HTTPS but transport module does not support TLS: %T", transport)
 		}
-
-		if !reflect.DeepEqual(transport, new(HTTPTransport)) {
+		if !reflect.DeepEqual(transport, reflect.New(reflect.TypeOf(transport).Elem()).Interface()) {
 			h.TransportRaw = caddyconfig.JSONModuleObject(transport, "protocol", transportModuleName, nil)
 		}
 	}
diff --git a/modules/caddyhttp/reverseproxy/httptransport.go b/modules/caddyhttp/reverseproxy/httptransport.go
index fa73f9d..c0d9c8d 100644
--- a/modules/caddyhttp/reverseproxy/httptransport.go
+++ b/modules/caddyhttp/reverseproxy/httptransport.go
@@ -42,19 +42,50 @@ type HTTPTransport struct {
 	// able to borrow/use at least some of these config fields; if so,
 	// maybe move them into a type called CommonTransport and embed it?
 
-	TLS                   *TLSConfig     `json:"tls,omitempty"`
-	KeepAlive             *KeepAlive     `json:"keep_alive,omitempty"`
-	Compression           *bool          `json:"compression,omitempty"`
-	MaxConnsPerHost       int            `json:"max_conns_per_host,omitempty"`
-	DialTimeout           caddy.Duration `json:"dial_timeout,omitempty"`
-	FallbackDelay         caddy.Duration `json:"dial_fallback_delay,omitempty"`
+	// Configures TLS to the upstream. Setting this to an empty struct
+	// is sufficient to enable TLS with reasonable defaults.
+	TLS *TLSConfig `json:"tls,omitempty"`
+
+	// Configures HTTP Keep-Alive (enabled by default). Should only be
+	// necessary if rigorous testing has shown that tuning this helps
+	// improve performance.
+	KeepAlive *KeepAlive `json:"keep_alive,omitempty"`
+
+	// Whether to enable compression to upstream. Default: true
+	Compression *bool `json:"compression,omitempty"`
+
+	// Maximum number of connections per host. Default: 0 (no limit)
+	MaxConnsPerHost int `json:"max_conns_per_host,omitempty"`
+
+	// How long to wait before timing out trying to connect to
+	// an upstream.
+	DialTimeout caddy.Duration `json:"dial_timeout,omitempty"`
+
+	// How long to wait before spawning an RFC 6555 Fast Fallback
+	// connection. A negative value disables this.
+	FallbackDelay caddy.Duration `json:"dial_fallback_delay,omitempty"`
+
+	// How long to wait for reading response headers from server.
 	ResponseHeaderTimeout caddy.Duration `json:"response_header_timeout,omitempty"`
+
+	// The length of time to wait for a server's first response
+	// headers after fully writing the request headers if the
+	// request has a header "Expect: 100-continue".
 	ExpectContinueTimeout caddy.Duration `json:"expect_continue_timeout,omitempty"`
-	MaxResponseHeaderSize int64          `json:"max_response_header_size,omitempty"`
-	WriteBufferSize       int            `json:"write_buffer_size,omitempty"`
-	ReadBufferSize        int            `json:"read_buffer_size,omitempty"`
-	Versions              []string       `json:"versions,omitempty"`
 
+	// The maximum bytes to read from response headers.
+	MaxResponseHeaderSize int64 `json:"max_response_header_size,omitempty"`
+
+	// The size of the write buffer in bytes.
+	WriteBufferSize int `json:"write_buffer_size,omitempty"`
+
+	// The size of the read buffer in bytes.
+	ReadBufferSize int `json:"read_buffer_size,omitempty"`
+
+	// The versions of HTTP to support. Default: ["1.1", "2"]
+	Versions []string `json:"versions,omitempty"`
+
+	// The pre-configured underlying HTTP transport.
 	Transport *http.Transport `json:"-"`
 }
 
@@ -73,7 +104,7 @@ func (h *HTTPTransport) Provision(_ caddy.Context) error {
 		h.Versions = []string{"1.1", "2"}
 	}
 
-	rt, err := h.newTransport()
+	rt, err := h.NewTransport()
 	if err != nil {
 		return err
 	}
@@ -82,7 +113,9 @@ func (h *HTTPTransport) Provision(_ caddy.Context) error {
 	return nil
 }
 
-func (h *HTTPTransport) newTransport() (*http.Transport, error) {
+// NewTransport builds a standard-lib-compatible
+// http.Transport value from h.
+func (h *HTTPTransport) NewTransport() (*http.Transport, error) {
 	dialer := &net.Dialer{
 		Timeout:       time.Duration(h.DialTimeout),
 		FallbackDelay: time.Duration(h.FallbackDelay),
@@ -148,14 +181,14 @@ func (h *HTTPTransport) newTransport() (*http.Transport, error) {
 
 // RoundTrip implements http.RoundTripper.
 func (h *HTTPTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	h.setScheme(req)
+	h.SetScheme(req)
 	return h.Transport.RoundTrip(req)
 }
 
-// setScheme ensures that the outbound request req
+// SetScheme ensures that the outbound request req
 // has the scheme set in its URL; the underlying
 // http.Transport requires a scheme to be set.
-func (h *HTTPTransport) setScheme(req *http.Request) {
+func (h *HTTPTransport) SetScheme(req *http.Request) {
 	if req.URL.Scheme == "" {
 		req.URL.Scheme = "http"
 		if h.TLS != nil {
@@ -164,6 +197,17 @@ func (h *HTTPTransport) setScheme(req *http.Request) {
 	}
 }
 
+// TLSEnabled returns true if TLS is enabled.
+func (h HTTPTransport) TLSEnabled() bool {
+	return h.TLS != nil
+}
+
+// EnableTLS enables TLS on the transport.
+func (h *HTTPTransport) EnableTLS(base *TLSConfig) error {
+	h.TLS = base
+	return nil
+}
+
 // Cleanup implements caddy.CleanerUpper and closes any idle connections.
 func (h HTTPTransport) Cleanup() error {
 	if h.Transport == nil {
@@ -173,18 +217,32 @@ func (h HTTPTransport) Cleanup() error {
 	return nil
 }
 
-// TLSConfig holds configuration related to the
-// TLS configuration for the transport/client.
+// TLSConfig holds configuration related to the TLS configuration for the
+// transport/client.
 type TLSConfig struct {
+	// Optional list of base64-encoded DER-encoded CA certificates to trust.
 	RootCAPool []string `json:"root_ca_pool,omitempty"`
-	// Added to the same pool as above, but brought in from files
+
+	// List of PEM-encoded CA certificate files to add to the same trust
+	// store as RootCAPool (or root_ca_pool in the JSON).
 	RootCAPEMFiles []string `json:"root_ca_pem_files,omitempty"`
-	// TODO: Should the client cert+key config use caddytls.CertificateLoader modules?
-	ClientCertificateFile    string         `json:"client_certificate_file,omitempty"`
-	ClientCertificateKeyFile string         `json:"client_certificate_key_file,omitempty"`
-	InsecureSkipVerify       bool           `json:"insecure_skip_verify,omitempty"`
-	HandshakeTimeout         caddy.Duration `json:"handshake_timeout,omitempty"`
-	ServerName               string         `json:"server_name,omitempty"`
+
+	// PEM-encoded client certificate filename to present to servers.
+	ClientCertificateFile string `json:"client_certificate_file,omitempty"`
+
+	// PEM-encoded key to use with the client certificate.
+	ClientCertificateKeyFile string `json:"client_certificate_key_file,omitempty"`
+
+	// If true, TLS verification of server certificates will be disabled.
+	// This is insecure and may be removed in the future. Do not use this
+	// option except in testing or local development environments.
+	InsecureSkipVerify bool `json:"insecure_skip_verify,omitempty"`
+
+	// The duration to allow a TLS handshake to a server.
+	HandshakeTimeout caddy.Duration `json:"handshake_timeout,omitempty"`
+
+	// The server name (SNI) to use in TLS handshakes.
+	ServerName string `json:"server_name,omitempty"`
 }
 
 // MakeTLSClientConfig returns a tls.Config usable by a client to a backend.
@@ -244,11 +302,20 @@ func (t TLSConfig) MakeTLSClientConfig() (*tls.Config, error) {
 
 // KeepAlive holds configuration pertaining to HTTP Keep-Alive.
 type KeepAlive struct {
-	Enabled             *bool          `json:"enabled,omitempty"`
-	ProbeInterval       caddy.Duration `json:"probe_interval,omitempty"`
-	MaxIdleConns        int            `json:"max_idle_conns,omitempty"`
-	MaxIdleConnsPerHost int            `json:"max_idle_conns_per_host,omitempty"`
-	IdleConnTimeout     caddy.Duration `json:"idle_timeout,omitempty"` // how long should connections be kept alive when idle
+	// Whether HTTP Keep-Alive is enabled. Default: true
+	Enabled *bool `json:"enabled,omitempty"`
+
+	// How often to probe for liveness.
+	ProbeInterval caddy.Duration `json:"probe_interval,omitempty"`
+
+	// Maximum number of idle connections.
+	MaxIdleConns int `json:"max_idle_conns,omitempty"`
+
+	// Maximum number of idle connections per upstream host.
+	MaxIdleConnsPerHost int `json:"max_idle_conns_per_host,omitempty"`
+
+	// How long connections should be kept alive when idle.
+	IdleConnTimeout caddy.Duration `json:"idle_timeout,omitempty"`
 }
 
 // decodeBase64DERCert base64-decodes, then DER-decodes, certStr.
@@ -278,4 +345,5 @@ var (
 	_ caddy.Provisioner  = (*HTTPTransport)(nil)
 	_ http.RoundTripper  = (*HTTPTransport)(nil)
 	_ caddy.CleanerUpper = (*HTTPTransport)(nil)
+	_ TLSTransport       = (*HTTPTransport)(nil)
 )
diff --git a/modules/caddyhttp/reverseproxy/ntlm.go b/modules/caddyhttp/reverseproxy/ntlm.go
deleted file mode 100644
index 270135a..0000000
--- a/modules/caddyhttp/reverseproxy/ntlm.go
+++ /dev/null
@@ -1,244 +0,0 @@
-// Copyright 2015 Matthew Holt and The Caddy Authors
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package reverseproxy
-
-import (
-	"context"
-	"fmt"
-	"net"
-	"net/http"
-	"strings"
-	"sync"
-
-	"github.com/caddyserver/caddy/v2"
-	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
-)
-
-func init() {
-	caddy.RegisterModule(NTLMTransport{})
-}
-
-// NTLMTransport proxies HTTP with NTLM authentication.
-// It basically wraps HTTPTransport so that it is compatible with
-// NTLM's HTTP-hostile requirements. Specifically, it will use
-// HTTPTransport's single, default *http.Transport for all requests
-// (unless the client's connection is already mapped to a different
-// transport) until a request comes in with an Authorization header
-// that has "NTLM" or "Negotiate"; when that happens, NTLMTransport
-// maps the client's connection (by its address, req.RemoteAddr)
-// to a new transport that is used only by that downstream conn.
-// When the upstream connection is closed, the mapping is deleted.
-// This preserves NTLM authentication contexts by ensuring that
-// client connections use the same upstream connection. It does
-// hurt performance a bit, but that's NTLM for you.
-//
-// This transport also forces HTTP/1.1 and Keep-Alives in order
-// for NTLM to succeed.
-//
-// It is basically the same thing as
-// [nginx's paid ntlm directive](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#ntlm)
-// (but is free in Caddy!).
-type NTLMTransport struct {
-	*HTTPTransport
-
-	transports   map[string]*http.Transport
-	transportsMu *sync.RWMutex
-}
-
-// CaddyModule returns the Caddy module information.
-func (NTLMTransport) CaddyModule() caddy.ModuleInfo {
-	return caddy.ModuleInfo{
-		ID: "http.reverse_proxy.transport.http_ntlm",
-		New: func() caddy.Module {
-			m := new(NTLMTransport)
-			m.HTTPTransport = new(HTTPTransport)
-			return m
-		},
-	}
-}
-
-// Provision sets up the transport module.
-func (n *NTLMTransport) Provision(ctx caddy.Context) error {
-	n.transports = make(map[string]*http.Transport)
-	n.transportsMu = new(sync.RWMutex)
-
-	if n.HTTPTransport == nil {
-		n.HTTPTransport = new(HTTPTransport)
-	}
-
-	// NTLM requires HTTP/1.1
-	n.HTTPTransport.Versions = []string{"1.1"}
-
-	// NLTM requires keep-alive
-	if n.HTTPTransport.KeepAlive != nil {
-		enabled := true
-		n.HTTPTransport.KeepAlive.Enabled = &enabled
-	}
-
-	// set up the underlying transport, since we
-	// rely on it for the heavy lifting
-	err := n.HTTPTransport.Provision(ctx)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
-// RoundTrip implements http.RoundTripper. It basically wraps
-// the underlying HTTPTransport.Transport in a way that preserves
-// NTLM context by mapping transports/connections. Note that this
-// method does not call n.HTTPTransport.RoundTrip (our own method),
-// but the underlying n.HTTPTransport.Transport.RoundTrip (standard
-// library's method).
-func (n *NTLMTransport) RoundTrip(req *http.Request) (*http.Response, error) {
-	n.HTTPTransport.setScheme(req)
-
-	// when the upstream connection is closed, make sure
-	// we close the downstream connection with the client
-	// when this request is done; we only do this if
-	// using a bound transport
-	closeDownstreamIfClosedUpstream := func() {
-		n.transportsMu.Lock()
-		if _, ok := n.transports[req.RemoteAddr]; !ok {
-			req.Close = true
-		}
-		n.transportsMu.Unlock()
-	}
-
-	// first, see if this downstream connection is
-	// already bound to a particular transport
-	// (transports are abstractions over connections
-	// to our upstream, and NTLM auth requires
-	// preserving authentication state for separate
-	// connections over multiple roundtrips, sigh)
-	n.transportsMu.Lock()
-	transport, ok := n.transports[req.RemoteAddr]
-	if ok {
-		n.transportsMu.Unlock()
-		defer closeDownstreamIfClosedUpstream()
-		return transport.RoundTrip(req)
-	}
-
-	// otherwise, start by assuming we will use
-	// the default transport that carries all
-	// normal/non-NTLM-authenticated requests
-	transport = n.HTTPTransport.Transport
-
-	// but if this request begins the NTLM authentication
-	// process, we need to pin it to a specific transport
-	if requestHasAuth(req) {
-		var err error
-		transport, err = n.newTransport()
-		if err != nil {
-			return nil, fmt.Errorf("making new transport for %s: %v", req.RemoteAddr, err)
-		}
-		n.transports[req.RemoteAddr] = transport
-		defer closeDownstreamIfClosedUpstream()
-	}
-	n.transportsMu.Unlock()
-
-	// finally, do the roundtrip with the transport we selected
-	return transport.RoundTrip(req)
-}
-
-// newTransport makes an NTLM-compatible transport.
-func (n *NTLMTransport) newTransport() (*http.Transport, error) {
-	// start with a regular HTTP transport
-	transport, err := n.HTTPTransport.newTransport()
-	if err != nil {
-		return nil, err
-	}
-
-	// we need to wrap upstream connections so we can
-	// clean up in two ways when that connection is
-	// closed: 1) destroy the transport that housed
-	// this connection, and 2) use that as a signal
-	// to close the connection to the downstream.
-	wrappedDialContext := transport.DialContext
-
-	transport.DialContext = func(ctx context.Context, network, address string) (net.Conn, error) {
-		conn2, err := wrappedDialContext(ctx, network, address)
-		if err != nil {
-			return nil, err
-		}
-		req := ctx.Value(caddyhttp.OriginalRequestCtxKey).(http.Request)
-		conn := &unbinderConn{Conn: conn2, ntlm: n, clientAddr: req.RemoteAddr}
-		return conn, nil
-	}
-
-	return transport, nil
-}
-
-// Cleanup implements caddy.CleanerUpper and closes any idle connections.
-func (n *NTLMTransport) Cleanup() error {
-	if err := n.HTTPTransport.Cleanup(); err != nil {
-		return err
-	}
-
-	n.transportsMu.Lock()
-	for _, t := range n.transports {
-		t.CloseIdleConnections()
-	}
-	n.transports = make(map[string]*http.Transport)
-	n.transportsMu.Unlock()
-
-	return nil
-}
-
-// deleteTransportsForClient deletes (unmaps) transports that are
-// associated with clientAddr (a req.RemoteAddr value).
-func (n *NTLMTransport) deleteTransportsForClient(clientAddr string) {
-	n.transportsMu.Lock()
-	for key := range n.transports {
-		if key == clientAddr {
-			delete(n.transports, key)
-		}
-	}
-	n.transportsMu.Unlock()
-}
-
-// requestHasAuth returns true if req has an Authorization
-// header with values "NTLM" or "Negotiate".
-func requestHasAuth(req *http.Request) bool {
-	for _, val := range req.Header["Authorization"] {
-		if strings.HasPrefix(val, "NTLM") ||
-			strings.HasPrefix(val, "Negotiate") {
-			return true
-		}
-	}
-	return false
-}
-
-// unbinderConn is used to wrap upstream connections
-// so that we know when they are closed and can clean
-// up after that.
-type unbinderConn struct {
-	net.Conn
-	clientAddr string
-	ntlm       *NTLMTransport
-}
-
-func (uc *unbinderConn) Close() error {
-	uc.ntlm.deleteTransportsForClient(uc.clientAddr)
-	return uc.Conn.Close()
-}
-
-// Interface guards
-var (
-	_ caddy.Provisioner  = (*NTLMTransport)(nil)
-	_ http.RoundTripper  = (*NTLMTransport)(nil)
-	_ caddy.CleanerUpper = (*NTLMTransport)(nil)
-)
diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go
index 6d0d441..438533c 100644
--- a/modules/caddyhttp/reverseproxy/reverseproxy.go
+++ b/modules/caddyhttp/reverseproxy/reverseproxy.go
@@ -739,8 +739,19 @@ var hopHeaders = []string{
 
 // DialError is an error that specifically occurs
 // in a call to Dial or DialContext.
-type DialError struct {
-	error
+type DialError struct{ error }
+
+// TLSTransport is implemented by transports
+// that are capable of using TLS.
+type TLSTransport interface {
+	// TLSEnabled returns true if the transport
+	// has TLS enabled, false otherwise.
+	TLSEnabled() bool
+
+	// EnableTLS enables TLS within the transport
+	// if it is not already, using the provided
+	// value as a basis for the TLS config.
+	EnableTLS(base *TLSConfig) error
 }
 
 var bufPool = sync.Pool{