From 043e58b83c378d096688e3e68b71a2337b1accfd Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Mon, 27 Jul 2026 18:03:58 +0100 Subject: [PATCH] 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 | 14 +++--- lib/http/middleware_test.go | 10 +++++ lib/http/server.go | 14 +++++- lib/http/server_test.go | 85 +++++++++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 6 deletions(-) diff --git a/lib/http/middleware.go b/lib/http/middleware.go index a9ccf4e2e..e72668f8d 100644 --- a/lib/http/middleware.go +++ b/lib/http/middleware.go @@ -79,11 +79,15 @@ 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) { - for _, cert := range r.TLS.PeerCertificates { - if cert.Subject.CommonName != "" { - r = r.WithContext(context.WithValue(r.Context(), ctxKeyUser, cert.Subject.CommonName)) - next.ServeHTTP(w, r) - return + // 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)) + next.ServeHTTP(w, r) + return + } } } code := http.StatusUnauthorized diff --git a/lib/http/middleware_test.go b/lib/http/middleware_test.go index d9ff11cf6..2623c0253 100644 --- a/lib/http/middleware_test.go +++ b/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") diff --git a/lib/http/server.go b/lib/http/server.go index f94cd8a5e..2df99ebbc 100644 --- a/lib/http/server.go +++ b/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 { diff --git a/lib/http/server_test.go b/lib/http/server_test.go index d675420d1..1bf67a452 100644 --- a/lib/http/server_test.go +++ b/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()