From a5834fdaab96834bde79303b247113d23ebb1380 Mon Sep 17 00:00:00 2001 From: Nicolas Frati Date: Fri, 9 Oct 2026 13:37:25 +0200 Subject: [PATCH] [management,signal] Make the Let's Encrypt challenge listener address configurable (#7706) * [management,signal] Make the Let's Encrypt challenge listener address configurable With Let's Encrypt enabled and --port set to something other than 443, signal and management also opened a separate challenge listener that was hard-coded to :443. Non-root deployments, such as the UBI images, could not start that listener. Add --letsencrypt-listen-address to both. It defaults to :443, so current behavior is unchanged. An empty value disables the separate listener for setups that forward public port 443 to --port, where the main TLS listener already answers TLS-ALPN-01 challenges. Signal now fails on startup when the challenge listener cannot bind, and exits non-zero when a server stops unexpectedly instead of exiting 0. A failure reported before the run loop waited was previously dropped. Management no longer opens a new :443 listener on shutdown just to close it. * [management,signal] Keep the challenge listener change additive Remove the Signal fail-fast changes from this PR. They change the behavior that existing installations see after an upgrade, so they move to a separate PR. If the challenge listener cannot bind, Signal now logs the error and continues. The main TLS listener still answers TLS-ALPN-01 challenges. Management keeps its previous behavior and stops with an error. The check for an empty address moves to the caller, so the function does not return a nil listener with a nil error. Also add assertion messages, guard a nil listener in a test cleanup, and add the flag to the Signal README. --- management/cmd/management.go | 1 + management/cmd/root.go | 2 + management/internals/server/server.go | 40 +++++++++++--- .../server/server_letsencrypt_test.go | 55 +++++++++++++++++++ signal/README.md | 1 + signal/cmd/run.go | 47 ++++++++++++---- signal/cmd/run_test.go | 52 ++++++++++++++++++ 7 files changed, 181 insertions(+), 17 deletions(-) create mode 100644 management/internals/server/server_letsencrypt_test.go create mode 100644 signal/cmd/run_test.go diff --git a/management/cmd/management.go b/management/cmd/management.go index fc6bd0a46..434ad8402 100644 --- a/management/cmd/management.go +++ b/management/cmd/management.go @@ -143,6 +143,7 @@ var ( MgmtPort: mgmtPort, MgmtMetricsPort: mgmtMetricsPort, DisableLegacyManagementPort: disableLegacyManagementPort, + LetsEncryptListenAddress: mgmtLetsencryptListen, DisableMetrics: disableMetrics, DisableGeoliteUpdate: disableGeoliteUpdate, UserDeleteFromIDPEnabled: userDeleteFromIDPEnabled, diff --git a/management/cmd/root.go b/management/cmd/root.go index ae03a09e8..466ee133f 100644 --- a/management/cmd/root.go +++ b/management/cmd/root.go @@ -29,6 +29,7 @@ var ( mgmtMetricsPort int disableLegacyManagementPort bool mgmtLetsencryptDomain string + mgmtLetsencryptListen string mgmtSingleAccModeDomain string certFile string certKey string @@ -70,6 +71,7 @@ func init() { mgmtCmd.Flags().StringVar(&mgmtDataDir, "datadir", defaultMgmtDataDir, "server data directory location") mgmtCmd.Flags().StringVar(&nbconfig.MgmtConfigPath, "config", defaultMgmtConfig, "Netbird config file location. Config params specified via command line (e.g. datadir) have a precedence over configuration from this file") mgmtCmd.Flags().StringVar(&mgmtLetsencryptDomain, "letsencrypt-domain", "", "a domain to issue Let's Encrypt certificate for. Enables TLS using Let's Encrypt. Will fetch and renew certificate, and run the server with TLS") + mgmtCmd.Flags().StringVar(&mgmtLetsencryptListen, "letsencrypt-listen-address", ":443", "address of the separate Let's Encrypt challenge listener, used when --port is not 443. Set it empty when public port 443 is forwarded to --port, which answers the challenges itself") mgmtCmd.Flags().StringVar(&mgmtSingleAccModeDomain, "single-account-mode-domain", defaultSingleAccModeDomain, "Enables single account mode. This means that all the users will be under the same account grouped by the specified domain. If the installation has more than one account, the property is ineffective. Enabled by default with the default domain "+defaultSingleAccModeDomain) mgmtCmd.Flags().BoolVar(&disableSingleAccMode, "disable-single-account-mode", false, "If set to true, disables single account mode. The --single-account-mode-domain property will be ignored and every new user will have a separate NetBird account.") mgmtCmd.Flags().StringVar(&certFile, "cert-file", "", "Location of your SSL certificate. Can be used when you have an existing certificate and don't want a new certificate be generated automatically. If letsencrypt-domain is specified this property has no effect") diff --git a/management/internals/server/server.go b/management/internals/server/server.go index 6d51745a7..bbf0f389d 100644 --- a/management/internals/server/server.go +++ b/management/internals/server/server.go @@ -68,6 +68,7 @@ type BaseServer struct { mgmtMetricsPort int mgmtPort int disableLegacyManagementPort bool + letsEncryptListenAddress string autoResolveDomains bool proxyAuthClose func() @@ -82,6 +83,8 @@ type BaseServer struct { tlsConfig *tls.Config certManager *autocert.Manager update *version.Update + // certListener serves Let's Encrypt challenges when mgmtPort is not 443. + certListener net.Listener errCh chan error wg sync.WaitGroup @@ -103,6 +106,9 @@ type Config struct { UserDeleteFromIDPEnabled bool AutoResolveDomains bool TLSConfig *tls.Config + // LetsEncryptListenAddress is the separate Let's Encrypt challenge listener + // used when MgmtPort is not 443. Empty disables it. + LetsEncryptListenAddress string } // NewServer initializes and configures a new Server instance @@ -117,6 +123,7 @@ func NewServer(cfg *Config) *BaseServer { userDeleteFromIDPEnabled: cfg.UserDeleteFromIDPEnabled, mgmtPort: cfg.MgmtPort, disableLegacyManagementPort: cfg.DisableLegacyManagementPort, + letsEncryptListenAddress: cfg.LetsEncryptListenAddress, mgmtMetricsPort: cfg.MgmtMetricsPort, autoResolveDomains: cfg.AutoResolveDomains, tlsConfig: cfg.TLSConfig, @@ -210,19 +217,18 @@ func (s *BaseServer) start(ctx context.Context) error { rootHandler := s.handlerFunc(srvCtx, s.GRPCServer(), s.APIHandler(), s.IDPHandler(), s.Metrics().GetMeter()) switch { case s.certManager != nil: - // a call to certManager.Listener() always creates a new listener so we do it once - cml := s.certManager.Listener() if s.mgmtPort == 443 { // CertManager, HTTP and gRPC API all on the same port rootHandler = s.certManager.HTTPHandler(rootHandler) - s.listener = cml + s.listener = s.certManager.Listener() } else { s.listener, err = tls.Listen("tcp", fmt.Sprintf(":%d", s.mgmtPort), s.certManager.TLSConfig()) if err != nil { return fmt.Errorf("failed creating TLS listener on port %d: %v", s.mgmtPort, err) } - log.WithContext(ctx).Infof("running HTTP server (LetsEncrypt challenge handler): %s", cml.Addr().String()) - s.serveHTTP(ctx, cml, s.certManager.HTTPHandler(nil)) + if err := s.serveLetsEncryptChallenges(ctx); err != nil { + return err + } } case s.tlsConfig != nil: s.listener, err = tls.Listen("tcp", fmt.Sprintf(":%d", s.mgmtPort), s.tlsConfig) @@ -309,8 +315,8 @@ func (s *BaseServer) Stop() error { if s.listener != nil { _ = s.listener.Close() } - if s.certManager != nil { - _ = s.certManager.Listener().Close() + if s.certListener != nil { + _ = s.certListener.Close() } s.GRPCServer().Stop() if s.proxyAuthClose != nil { @@ -416,6 +422,26 @@ func (s *BaseServer) serveGRPC(ctx context.Context, grpcServer *grpc.Server, por return listener, nil } +// serveLetsEncryptChallenges starts the separate Let's Encrypt challenge listener +// unless it is disabled. The main TLS listener uses the cert manager's TLS +// config, so it still answers TLS-ALPN-01 challenges when public port 443 is +// forwarded to it. +func (s *BaseServer) serveLetsEncryptChallenges(ctx context.Context) error { + if s.letsEncryptListenAddress == "" { + log.WithContext(ctx).Infof("LetsEncrypt challenge server disabled, challenges are answered on port %d", s.mgmtPort) + return nil + } + + cml, err := tls.Listen("tcp", s.letsEncryptListenAddress, s.certManager.TLSConfig()) + if err != nil { + return fmt.Errorf("create LetsEncrypt challenge listener on %s: %w", s.letsEncryptListenAddress, err) + } + s.certListener = cml + log.WithContext(ctx).Infof("running HTTP server (LetsEncrypt challenge handler): %s", cml.Addr().String()) + s.serveHTTP(ctx, cml, s.certManager.HTTPHandler(nil)) + return nil +} + func (s *BaseServer) serveHTTP(ctx context.Context, httpListener net.Listener, handler http.Handler) { s.wg.Add(1) go func() { diff --git a/management/internals/server/server_letsencrypt_test.go b/management/internals/server/server_letsencrypt_test.go new file mode 100644 index 000000000..0827d5e5d --- /dev/null +++ b/management/internals/server/server_letsencrypt_test.go @@ -0,0 +1,55 @@ +package server + +import ( + "context" + "net" + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/crypto/acme/autocert" + + nbconfig "github.com/netbirdio/netbird/management/internals/server/config" +) + +func newLetsEncryptTestServer(address string) *BaseServer { + srv := NewServer(&Config{NbConfig: &nbconfig.Config{}, MgmtPort: 8443, LetsEncryptListenAddress: address}) + srv.certManager = &autocert.Manager{} + return srv +} + +func TestServeLetsEncryptChallenges_Disabled(t *testing.T) { + srv := newLetsEncryptTestServer("") + + require.NoError(t, srv.serveLetsEncryptChallenges(context.Background())) + require.Nil(t, srv.certListener, "no challenge listener should be created when the address is empty") +} + +func TestServeLetsEncryptChallenges_CustomAddress(t *testing.T) { + srv := newLetsEncryptTestServer("127.0.0.1:0") + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(func() { + cancel() + if srv.certListener != nil { + _ = srv.certListener.Close() + } + srv.wg.Wait() + }) + + require.NoError(t, srv.serveLetsEncryptChallenges(ctx)) + require.NotNil(t, srv.certListener, "challenge listener should be created on the configured address") + + conn, err := net.DialTimeout("tcp", srv.certListener.Addr().String(), time.Second) + require.NoError(t, err) + require.NoError(t, conn.Close()) +} + +func TestServeLetsEncryptChallenges_BindFailure(t *testing.T) { + occupied, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = occupied.Close() }) + srv := newLetsEncryptTestServer(occupied.Addr().String()) + + require.Error(t, srv.serveLetsEncryptChallenges(context.Background())) + require.Nil(t, srv.certListener, "no challenge listener should be stored when the bind fails") +} diff --git a/signal/README.md b/signal/README.md index 0033eaf90..dd57252ad 100644 --- a/signal/README.md +++ b/signal/README.md @@ -16,6 +16,7 @@ Usage: Flags: -h, --help help for run --letsencrypt-domain string a domain to issue Let's Encrypt certificate for. Enables TLS using Let's Encrypt. Will fetch and renew certificate, and run the server with TLS + --letsencrypt-listen-address string address of the separate Let's Encrypt challenge listener, used when --port is not 443. Set it empty when public port 443 is forwarded to --port, which answers the challenges itself (default ":443") --port int Server port to listen on (e.g. 10000) (default 10000) --ssl-dir string server ssl directory location. *Required only for Let's Encrypt certificates. (default "/var/lib/netbird/") --cert-file string Location of your SSL certificate. Can be used when you have an existing certificate and don't want a new certificate be generated automatically. If letsencrypt-domain is specified this property has no effect diff --git a/signal/cmd/run.go b/signal/cmd/run.go index 42b7d2505..ffb596909 100644 --- a/signal/cmd/run.go +++ b/signal/cmd/run.go @@ -36,7 +36,12 @@ import ( "google.golang.org/grpc/keepalive" ) -const legacyGRPCPort = 10000 +const ( + legacyGRPCPort = 10000 + // defaultLetsencryptListenAddress is where Let's Encrypt connects for + // TLS-ALPN-01 challenges unless public port 443 is forwarded elsewhere. + defaultLetsencryptListenAddress = ":443" +) var ( signalPort int @@ -44,6 +49,7 @@ var ( signalLetsencryptDomain string signalLetsencryptEmail string signalLetsencryptDataDir string + signalLetsencryptListen string signalCertFile string signalCertKey string @@ -124,8 +130,18 @@ var ( grpcRootHandler := grpcHandlerFunc(grpcServer, metricsServer.Meter) - if certManager != nil { - startServerWithCertManager(certManager, grpcRootHandler) + var certListener net.Listener + switch { + case certManager == nil: + case signalPort != 443 && signalLetsencryptListen == "": + // The main TLS listener uses the cert manager's TLS config, so it still + // answers TLS-ALPN-01 challenges when public port 443 is forwarded to it. + log.Infof("LetsEncrypt challenge server disabled, challenges are answered on port %d", signalPort) + default: + certListener, err = startServerWithCertManager(certManager, grpcRootHandler) + if err != nil { + log.Errorf("LetsEncrypt challenge server not started: %v", err) + } } var compatListener net.Listener @@ -169,6 +185,10 @@ var ( SetupCloseHandler() <-stopCh + if certListener != nil { + _ = certListener.Close() + log.Infof("stopped LetsEncrypt challenge server") + } if grpcListener != nil { _ = grpcListener.Close() log.Infof("stopped gRPC server") @@ -245,18 +265,24 @@ func getTLSConfigurations() ([]grpc.ServerOption, *autocert.Manager, *tls.Config return []grpc.ServerOption{grpc.Creds(transportCredentials)}, certManager, tlsConfig, err } -func startServerWithCertManager(certManager *autocert.Manager, grpcRootHandler http.Handler) { - // a call to certManager.Listener() always creates a new listener so we do it once - httpListener := certManager.Listener() +func startServerWithCertManager(certManager *autocert.Manager, grpcRootHandler http.Handler) (net.Listener, error) { if signalPort == 443 { + // a call to certManager.Listener() always creates a new listener so we do it once + httpListener := certManager.Listener() // running gRPC and HTTP cert manager on the same port serveHTTP(httpListener, certManager.HTTPHandler(grpcRootHandler)) log.Infof("running HTTP server (LetsEncrypt challenge handler) and gRPC server on the same port: %s", httpListener.Addr().String()) - } else { - // Start the HTTP cert manager server separately - serveHTTP(httpListener, certManager.HTTPHandler(nil)) - log.Infof("running HTTP server (LetsEncrypt challenge handler): %s", httpListener.Addr().String()) + return httpListener, nil } + + httpListener, err := tls.Listen("tcp", signalLetsencryptListen, certManager.TLSConfig()) + if err != nil { + return nil, fmt.Errorf("create LetsEncrypt challenge listener on %s: %w", signalLetsencryptListen, err) + } + // Start the HTTP cert manager server separately + serveHTTP(httpListener, certManager.HTTPHandler(nil)) + log.Infof("running HTTP server (LetsEncrypt challenge handler): %s", httpListener.Addr().String()) + return httpListener, nil } func grpcHandlerFunc(grpcServer *grpc.Server, meter metric.Meter) http.Handler { @@ -334,6 +360,7 @@ func init() { runCmd.PersistentFlags().StringVar(&signalLetsencryptDataDir, "letsencrypt-data-dir", "", "a directory to store Let's Encrypt data. Required if Let's Encrypt is enabled.") runCmd.PersistentFlags().StringVar(&signalLetsencryptDataDir, "ssl-dir", "", "server ssl directory location. *Required only for Let's Encrypt certificates. Deprecated: use --letsencrypt-data-dir") runCmd.PersistentFlags().StringVar(&signalLetsencryptDomain, "letsencrypt-domain", "", "a domain to issue Let's Encrypt certificate for. Enables TLS using Let's Encrypt. Will fetch and renew certificate, and run the server with TLS") + runCmd.PersistentFlags().StringVar(&signalLetsencryptListen, "letsencrypt-listen-address", defaultLetsencryptListenAddress, "address of the separate Let's Encrypt challenge listener, used when --port is not 443. Set it empty when public port 443 is forwarded to --port, which answers the challenges itself") runCmd.PersistentFlags().StringVar(&signalLetsencryptEmail, "letsencrypt-email", "", "email address to use for Let's Encrypt certificate registration") runCmd.PersistentFlags().StringVar(&signalCertFile, "cert-file", "", "Location of your SSL certificate. Can be used when you have an existing certificate and don't want a new certificate be generated automatically. If letsencrypt-domain is specified this property has no effect") runCmd.PersistentFlags().StringVar(&signalCertKey, "cert-key", "", "Location of your SSL certificate private key. Can be used when you have an existing certificate and don't want a new certificate be generated automatically. If letsencrypt-domain is specified this property has no effect") diff --git a/signal/cmd/run_test.go b/signal/cmd/run_test.go new file mode 100644 index 000000000..e0a07e923 --- /dev/null +++ b/signal/cmd/run_test.go @@ -0,0 +1,52 @@ +package cmd + +import ( + "net" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/crypto/acme" + "golang.org/x/crypto/acme/autocert" +) + +func setLetsencryptListen(t *testing.T, port int, address string) { + t.Helper() + oldPort, oldAddress := signalPort, signalLetsencryptListen + signalPort, signalLetsencryptListen = port, address + t.Cleanup(func() { + signalPort, signalLetsencryptListen = oldPort, oldAddress + }) +} + +func TestStartServerWithCertManager_CustomAddress(t *testing.T) { + setLetsencryptListen(t, 10000, "127.0.0.1:0") + + listener, err := startServerWithCertManager(&autocert.Manager{}, http.NotFoundHandler()) + require.NoError(t, err) + require.NotNil(t, listener, "challenge listener should be created on the configured address") + t.Cleanup(func() { _ = listener.Close() }) + + conn, err := net.DialTimeout("tcp", listener.Addr().String(), time.Second) + require.NoError(t, err) + require.NoError(t, conn.Close()) +} + +func TestStartServerWithCertManager_BindFailure(t *testing.T) { + occupied, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = occupied.Close() }) + setLetsencryptListen(t, 10000, occupied.Addr().String()) + + listener, err := startServerWithCertManager(&autocert.Manager{}, http.NotFoundHandler()) + require.Error(t, err) + require.Nil(t, listener, "no listener should be returned when the bind fails") +} + +func TestCertManagerTLSConfigAnswersTLSALPN01(t *testing.T) { + // Disabling the separate listener relies on the main listener answering + // TLS-ALPN-01 challenges through the cert manager's TLS config. + cfg := (&autocert.Manager{}).TLSConfig() + require.Contains(t, cfg.NextProtos, acme.ALPNProto, "cert manager TLS config should offer the ACME TLS-ALPN protocol") +}