lib/http: use TLS on all --addr listeners when --cert and --key are set GHSA-mfvx-7rcj-9m5g

When --cert and --key were supplied TLS was only applied to the listener if
exactly one --addr was given. With two or more --addr flags every listener
without an explicit tls:// prefix silently served cleartext HTTP, so adding a
second --addr to an HTTPS server quietly disabled TLS on both.

Now when TLS is configured every listener serves TLS. An individual listener
can be prefixed with http:// to serve unencrypted HTTP on that address, and
tls:// still marks a listener as TLS explicitly. Using a tls:// address
without --cert and --key is now an error instead of silently serving
cleartext with an https:// URL.

Addresses GHSA-mfvx-7rcj-9m5g finding 3.

lib/http/middleware.go

@@ -79,6 +79,9 @@ func basicAuth(authenticator *LoggedBasicAuth) func(next http.Handler) http.Hand
 func MiddlewareAuthCertificateUser() Middleware {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			// r.TLS is nil on a plain HTTP listener (http:// prefix) where
+			// there can be no client certificate
+			if r.TLS != nil {
 				for _, cert := range r.TLS.PeerCertificates {
 					if cert.Subject.CommonName != "" {
 						r = r.WithContext(context.WithValue(r.Context(), ctxKeyUser, cert.Subject.CommonName))
@@ -86,6 +89,7 @@ func MiddlewareAuthCertificateUser() Middleware {
 						return
 					}
 				}
+			}
 			code := http.StatusUnauthorized
 			w.Header().Set("Content-Type", "text/plain")
 			http.Error(w, http.StatusText(code), code)

lib/http/middleware_test.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"net/http/httptest"
 	"strings"
 	"testing"
 
@@ -208,6 +209,15 @@ func TestMiddlewareAuth(t *testing.T) {
 	}
 }
 
+// A request arriving over a plain HTTP listener has no TLS state so it can't
+// carry a client certificate and must be rejected.
+func TestMiddlewareAuthCertificateUserNoTLS(t *testing.T) {
+	handler := MiddlewareAuthCertificateUser()(testEchoHandler([]byte("ok")))
+	rec := httptest.NewRecorder()
+	handler.ServeHTTP(rec, httptest.NewRequest("GET", "http://example.com/", nil))
+	require.Equal(t, http.StatusUnauthorized, rec.Code)
+}
+
 func TestMiddlewareAuthCertificateUser(t *testing.T) {
 	serverCertBytes := testReadTestdataFile(t, "local.crt")
 	serverKeyBytes := testReadTestdataFile(t, "local.key")

lib/http/server.go

@@ -78,6 +78,11 @@ https.  You will need to supply the ` + "`--{{ .Prefix }}cert` and `--{{ .Prefix
 If you wish to do client side certificate validation then you will need to
 supply ` + "`--{{ .Prefix }}client-ca`" + ` also.
 
+When TLS is configured every listener given with ` + "`--{{ .Prefix }}addr`" + ` serves TLS.
+An individual listener can be prefixed with ` + "`http://`" + ` to serve unencrypted
+HTTP on that address, or with ` + "`tls://`" + ` to state explicitly that it must serve
+TLS.  Using a ` + "`tls://`" + ` address without ` + "`--{{ .Prefix }}cert` and `--{{ .Prefix }}key`" + ` is an error.
+
 ` + "`--{{ .Prefix }}cert`" + ` must be set to the path of a file containing
 either a PEM encoded certificate, or a concatenation of that with the CA
 certificate. ` + "`--{{ .Prefix }}key`" + ` must be set to the path of a file
@@ -404,7 +409,12 @@ func NewServer(ctx context.Context, options ...Option) (*Server, error) {
 				return nil, err
 			}
 			instance = newInstance(ctx, s, listener, s.tlsConfig, addr)
-		} else if strings.HasPrefix(addr, "tls://") || (len(s.cfg.ListenAddr) == 1 && s.tlsConfig != nil) {
+		} else if strings.HasPrefix(addr, "tls://") || (!strings.HasPrefix(addr, "http://") && s.tlsConfig != nil) {
+			// If TLS is configured all listeners serve TLS unless
+			// explicitly marked http://.
+			if s.tlsConfig == nil {
+				return nil, fmt.Errorf("can't listen on %q: %w", addr, ErrTLSConfigRequired)
+			}
 			addr = strings.TrimPrefix(addr, "tls://")
 			listener, err := net.Listen("tcp", addr)
 			if err != nil {
@@ -486,6 +496,8 @@ var (
 	ErrTLSFileMismatch = errors.New("need both --cert and --key to use TLS")
 	// ErrTLSParseCA - hard coded errors, allowing for easier testing
 	ErrTLSParseCA = errors.New("unable to parse client certificate authority")
+	// ErrTLSConfigRequired - hard coded errors, allowing for easier testing
+	ErrTLSConfigRequired = errors.New("need both --cert and --key to use a tls:// address")
 )
 
 func (s *Server) initTLS() error {

lib/http/server_test.go

@@ -551,6 +551,91 @@ func TestNewServerTLS(t *testing.T) {
 	}
 }
 
+// TestNewServerTLSMultipleListeners checks that when TLS is configured it
+// applies to every listener, with the http:// and tls:// prefixes selecting
+// the protocol for an individual listener.
+func TestNewServerTLSMultipleListeners(t *testing.T) {
+	serverCertBytes := testReadTestdataFile(t, "local.crt")
+	serverKeyBytes := testReadTestdataFile(t, "local.key")
+
+	for _, ss := range []struct {
+		name    string
+		addrs   []string
+		schemes []string
+	}{
+		{
+			name:    "AllTLSByDefault",
+			addrs:   []string{"127.0.0.1:0", "127.0.0.1:0"},
+			schemes: []string{"https://", "https://"},
+		},
+		{
+			name:    "ExplicitHTTP",
+			addrs:   []string{"127.0.0.1:0", "http://127.0.0.1:0"},
+			schemes: []string{"https://", "http://"},
+		},
+		{
+			name:    "ExplicitTLS",
+			addrs:   []string{"tls://127.0.0.1:0", "http://127.0.0.1:0"},
+			schemes: []string{"https://", "http://"},
+		},
+	} {
+		t.Run(ss.name, func(t *testing.T) {
+			cfg := DefaultCfg()
+			cfg.ListenAddr = ss.addrs
+			cfg.TLSCertBody = serverCertBytes
+			cfg.TLSKeyBody = serverKeyBytes
+
+			s, err := NewServer(context.Background(), WithConfig(cfg))
+			require.NoError(t, err)
+			defer func() {
+				require.NoError(t, s.Shutdown())
+			}()
+
+			expected := []byte("secret-page")
+			s.Router().Mount("/", testEchoHandler(expected))
+			s.Serve()
+
+			urls := s.URLs()
+			require.Equal(t, len(ss.schemes), len(urls), "should have one URL per listener")
+			for i, url := range urls {
+				require.Truef(t, strings.HasPrefix(url, ss.schemes[i]), "url %q should have scheme %q", url, ss.schemes[i])
+
+				// Check the listener really speaks the protocol its URL claims
+				client := &http.Client{}
+				reqURL := url
+				if ss.schemes[i] == "https://" {
+					dest := strings.TrimSuffix(strings.TrimPrefix(url, "https://"), "/")
+					client.Transport = &http.Transport{
+						DialContext: func(_ context.Context, _, _ string) (net.Conn, error) {
+							return net.Dial("tcp", dest)
+						},
+						TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+					}
+					reqURL = "https://dev.rclone.org"
+				}
+				req, err := http.NewRequest("GET", reqURL, nil)
+				require.NoError(t, err)
+
+				resp, err := client.Do(req)
+				require.NoError(t, err)
+
+				require.Equal(t, http.StatusOK, resp.StatusCode, "should return ok")
+				testExpectRespBody(t, resp, expected)
+				require.NoError(t, resp.Body.Close())
+			}
+		})
+	}
+}
+
+// A tls:// address without a TLS config must be an error rather than silently
+// serving cleartext.
+func TestNewServerTLSListenerRequiresConfig(t *testing.T) {
+	cfg := DefaultCfg()
+	cfg.ListenAddr = []string{"tls://127.0.0.1:0"}
+	_, err := NewServer(context.Background(), WithConfig(cfg))
+	require.ErrorIs(t, err, ErrTLSConfigRequired)
+}
+
 func TestH2CServer(t *testing.T) {
 	ctx := context.Background()