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