[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

@@ -143,6 +143,7 @@ var (
 				MgmtPort:                    mgmtPort,
 				MgmtMetricsPort:             mgmtMetricsPort,
 				DisableLegacyManagementPort: disableLegacyManagementPort,
+				LetsEncryptListenAddress:    mgmtLetsencryptListen,
 				DisableMetrics:              disableMetrics,
 				DisableGeoliteUpdate:        disableGeoliteUpdate,
 				UserDeleteFromIDPEnabled:    userDeleteFromIDPEnabled,

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")

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() {

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")
+}

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

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")

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")
+}