Skip to content

Commit 1657c6d

Browse files
drakkandrakkan
committed
serve: guard the state the servers publish while they initialize
Signed-off-by: Nicola Murino <nicola.murino@gmail.com>
1 parent e801831 commit 1657c6d

15 files changed

Lines changed: 262 additions & 184 deletions

‎internal/ftpd/ftpd.go‎

Lines changed: 45 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,10 @@ import (
2323
"net"
2424
"os"
2525
"path/filepath"
26+
"slices"
2627
"strings"
28+
"sync"
29+
"sync/atomic"
2730
"time"
2831

2932
ftpserver "github.com/fclairamb/ftpserverlib"
@@ -39,8 +42,9 @@ const (
3942
)
4043

4144
var (
42-
certMgr *common.CertManager
43-
serviceStatus ServiceStatus
45+
certMgr atomic.Pointer[common.CertManager]
46+
serviceStatus ServiceStatus
47+
serviceStatusMu sync.RWMutex
4448
)
4549

4650
// PassiveIPOverride defines an exception for the configured passive IP
@@ -222,7 +226,8 @@ func (b *Binding) HasProxy() bool {
222226

223227
// GetTLSDescription returns the TLS mode as string
224228
func (b *Binding) GetTLSDescription() string {
225-
if certMgr == nil {
229+
mgr := certMgr.Load()
230+
if mgr == nil {
226231
return util.I18nFTPTLSDisabled
227232
}
228233
switch b.TLSMode {
@@ -232,7 +237,7 @@ func (b *Binding) GetTLSDescription() string {
232237
return util.I18nFTPTLSImplicit
233238
}
234239

235-
if certMgr.HasCertificate(common.DefaultTLSKeyPaidID) || certMgr.HasCertificate(b.GetAddress()) {
240+
if mgr.HasCertificate(common.DefaultTLSKeyPaidID) || mgr.HasCertificate(b.GetAddress()) {
236241
return util.I18nFTPTLSMixed
237242
}
238243
return util.I18nFTPTLSDisabled
@@ -385,12 +390,9 @@ func (c *Configuration) Initialize(configDir string) error {
385390
if err := mgr.LoadCRLs(); err != nil {
386391
return err
387392
}
388-
certMgr = mgr
389-
}
390-
serviceStatus = ServiceStatus{
391-
Bindings: nil,
392-
PassivePortRange: c.PassivePortRange,
393+
certMgr.Store(mgr)
393394
}
395+
resetServiceStatus(c.PassivePortRange)
394396

395397
exitChannel := make(chan error, 1)
396398

@@ -415,25 +417,54 @@ func (c *Configuration) Initialize(configDir string) error {
415417
exitChannel <- ftpServer.ListenAndServe()
416418
}(server)
417419

418-
serviceStatus.Bindings = append(serviceStatus.Bindings, binding)
420+
addServiceStatusBinding(binding)
419421
}
420422

421-
serviceStatus.IsActive = true
423+
setServiceStatusActive()
422424

423425
return <-exitChannel
424426
}
425427

426428
// ReloadCertificateMgr reloads the certificate manager
427429
func ReloadCertificateMgr() error {
428-
if certMgr != nil {
429-
return certMgr.Reload()
430+
if mgr := certMgr.Load(); mgr != nil {
431+
return mgr.Reload()
430432
}
431433
return nil
432434
}
433435

436+
func resetServiceStatus(portRange PortRange) {
437+
serviceStatusMu.Lock()
438+
defer serviceStatusMu.Unlock()
439+
440+
serviceStatus = ServiceStatus{
441+
Bindings: nil,
442+
PassivePortRange: portRange,
443+
}
444+
}
445+
446+
func addServiceStatusBinding(binding Binding) {
447+
serviceStatusMu.Lock()
448+
defer serviceStatusMu.Unlock()
449+
450+
serviceStatus.Bindings = append(serviceStatus.Bindings, binding)
451+
}
452+
453+
func setServiceStatusActive() {
454+
serviceStatusMu.Lock()
455+
defer serviceStatusMu.Unlock()
456+
457+
serviceStatus.IsActive = true
458+
}
459+
434460
// GetStatus returns the server status
435461
func GetStatus() ServiceStatus {
436-
return serviceStatus
462+
serviceStatusMu.RLock()
463+
defer serviceStatusMu.RUnlock()
464+
465+
status := serviceStatus
466+
status.Bindings = slices.Clone(serviceStatus.Bindings)
467+
return status
437468
}
438469

439470
func parsePassiveIP(passiveIP string) (string, error) {

‎internal/ftpd/ftpd_test.go‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import (
5151
"github.com/drakkan/sftpgo/v2/internal/config"
5252
"github.com/drakkan/sftpgo/v2/internal/dataprovider"
5353
"github.com/drakkan/sftpgo/v2/internal/ftpd"
54+
"github.com/drakkan/sftpgo/v2/internal/httpd"
5455
"github.com/drakkan/sftpgo/v2/internal/httpdtest"
5556
"github.com/drakkan/sftpgo/v2/internal/kms"
5657
"github.com/drakkan/sftpgo/v2/internal/logger"
@@ -384,28 +385,28 @@ func TestMain(m *testing.M) {
384385
os.Exit(1)
385386
}
386387

387-
go func() {
388-
logger.Debug(logSender, "", "initializing FTP server with config %+v", ftpdConf)
389-
if err := ftpdConf.Initialize(configDir); err != nil {
388+
go func(cfg ftpd.Configuration) {
389+
logger.Debug(logSender, "", "initializing FTP server with config %+v", cfg)
390+
if err := cfg.Initialize(configDir); err != nil {
390391
logger.ErrorToConsole("could not start FTP server: %v", err)
391392
os.Exit(1)
392393
}
393-
}()
394+
}(ftpdConf)
394395

395-
go func() {
396-
logger.Debug(logSender, "", "initializing SFTP server with config %+v", sftpdConf)
397-
if err := sftpdConf.Initialize(configDir); err != nil {
396+
go func(cfg sftpd.Configuration) {
397+
logger.Debug(logSender, "", "initializing SFTP server with config %+v", cfg)
398+
if err := cfg.Initialize(configDir); err != nil {
398399
logger.ErrorToConsole("could not start SFTP server: %v", err)
399400
os.Exit(1)
400401
}
401-
}()
402+
}(sftpdConf)
402403

403-
go func() {
404-
if err := httpdConf.Initialize(configDir, 0); err != nil {
404+
go func(cfg httpd.Conf) {
405+
if err := cfg.Initialize(configDir, 0); err != nil {
405406
logger.ErrorToConsole("could not start HTTP server: %v", err)
406407
os.Exit(1)
407408
}
408-
}()
409+
}(httpdConf)
409410

410411
waitTCPListening(ftpdConf.Bindings[0].GetAddress())
411412
waitTCPListening(httpdConf.Bindings[0].GetAddress())
@@ -428,13 +429,13 @@ func TestMain(m *testing.M) {
428429
ftpdConf.CombineSupport = 1
429430
ftpdConf.HASHSupport = 1
430431

431-
go func() {
432-
logger.Debug(logSender, "", "initializing FTP server with config %+v", ftpdConf)
433-
if err := ftpdConf.Initialize(configDir); err != nil {
432+
go func(cfg ftpd.Configuration) {
433+
logger.Debug(logSender, "", "initializing FTP server with config %+v", cfg)
434+
if err := cfg.Initialize(configDir); err != nil {
434435
logger.ErrorToConsole("could not start FTP server: %v", err)
435436
os.Exit(1)
436437
}
437-
}()
438+
}(ftpdConf)
438439

439440
waitTCPListening(ftpdConf.Bindings[0].GetAddress())
440441

@@ -451,13 +452,13 @@ func TestMain(m *testing.M) {
451452
ftpdConf.CACertificates = []string{caCrtPath}
452453
ftpdConf.CARevocationLists = []string{caCRLPath}
453454

454-
go func() {
455-
logger.Debug(logSender, "", "initializing FTP server with config %+v", ftpdConf)
456-
if err := ftpdConf.Initialize(configDir); err != nil {
455+
go func(cfg ftpd.Configuration) {
456+
logger.Debug(logSender, "", "initializing FTP server with config %+v", cfg)
457+
if err := cfg.Initialize(configDir); err != nil {
457458
logger.ErrorToConsole("could not start FTP server: %v", err)
458459
os.Exit(1)
459460
}
460-
}()
461+
}(ftpdConf)
461462

462463
waitTCPListening(ftpdConf.Bindings[0].GetAddress())
463464

‎internal/ftpd/internal_test.go‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -422,8 +422,8 @@ func newMockOsFs(err, statErr error, atomicUpload bool, connectionID, rootDir st
422422
}
423423

424424
func TestInitialization(t *testing.T) {
425-
oldMgr := certMgr
426-
certMgr = nil
425+
oldMgr := certMgr.Load()
426+
certMgr.Store(nil)
427427

428428
binding := Binding{
429429
Port: 2121,
@@ -503,21 +503,22 @@ func TestInitialization(t *testing.T) {
503503
ID: binding.GetAddress(),
504504
},
505505
}
506-
certMgr, err = common.NewCertManager(keyPairs, configDir, "")
506+
mgr, err := common.NewCertManager(keyPairs, configDir, "")
507507
require.NoError(t, err)
508+
certMgr.Store(mgr)
508509

509510
assert.Equal(t, util.I18nFTPTLSMixed, binding.GetTLSDescription())
510511
server = NewServer(c, configDir, binding, 0)
511512
cfg, err := server.GetTLSConfig()
512513
require.NoError(t, err)
513514
assert.Equal(t, tls.RequireAndVerifyClientCert, cfg.ClientAuth)
514515

515-
certMgr = oldMgr
516+
certMgr.Store(oldMgr)
516517
}
517518

518519
func TestServerGetSettings(t *testing.T) {
519520
oldConfig := common.Config
520-
oldMgr := certMgr
521+
oldMgr := certMgr.Load()
521522

522523
binding := Binding{
523524
Port: 2121,
@@ -568,8 +569,9 @@ func TestServerGetSettings(t *testing.T) {
568569
ID: common.DefaultTLSKeyPaidID,
569570
},
570571
}
571-
certMgr, err = common.NewCertManager(keyPairs, configDir, "")
572+
mgr, err := common.NewCertManager(keyPairs, configDir, "")
572573
require.NoError(t, err)
574+
certMgr.Store(mgr)
573575
common.Config.ProxyAllowed = nil
574576
c.CertificateFile = certPath
575577
c.CertificateKeyFile = keyPath
@@ -588,7 +590,7 @@ func TestServerGetSettings(t *testing.T) {
588590
assert.True(t, ok)
589591

590592
common.Config = oldConfig
591-
certMgr = oldMgr
593+
certMgr.Store(oldMgr)
592594
}
593595

594596
func TestUserInvalidParams(t *testing.T) {
@@ -943,7 +945,7 @@ func TestTransferErrors(t *testing.T) {
943945
}
944946

945947
func TestVerifyTLSConnection(t *testing.T) {
946-
oldCertMgr := certMgr
948+
oldCertMgr := certMgr.Load()
947949

948950
caCrlPath := filepath.Join(os.TempDir(), "testcrl.crt")
949951
certPath := filepath.Join(os.TempDir(), "test.crt")
@@ -961,11 +963,12 @@ func TestVerifyTLSConnection(t *testing.T) {
961963
ID: common.DefaultTLSKeyPaidID,
962964
},
963965
}
964-
certMgr, err = common.NewCertManager(keyPairs, "", "ftp_test")
966+
mgr, err := common.NewCertManager(keyPairs, "", "ftp_test")
965967
assert.NoError(t, err)
968+
certMgr.Store(mgr)
966969

967-
certMgr.SetCARevocationLists([]string{caCrlPath})
968-
err = certMgr.LoadCRLs()
970+
certMgr.Load().SetCARevocationLists([]string{caCrlPath})
971+
err = certMgr.Load().LoadCRLs()
969972
assert.NoError(t, err)
970973

971974
crt, err := tls.X509KeyPair([]byte(client1Crt), []byte(client1Key))
@@ -1013,7 +1016,7 @@ func TestVerifyTLSConnection(t *testing.T) {
10131016
err = os.Remove(keyPath)
10141017
assert.NoError(t, err)
10151018

1016-
certMgr = oldCertMgr
1019+
certMgr.Store(oldCertMgr)
10171020
}
10181021

10191022
func TestCiphers(t *testing.T) {

‎internal/ftpd/server.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ func (s *Server) GetSettings() (*ftpserver.Settings, error) {
110110
return nil, fmt.Errorf("unsupported TLS mode: %d", s.binding.TLSMode)
111111
}
112112

113-
if s.binding.TLSMode > 0 && certMgr == nil {
113+
if s.binding.TLSMode > 0 && certMgr.Load() == nil {
114114
return nil, errors.New("to enable TLS you need to provide a certificate")
115115
}
116116

@@ -284,23 +284,23 @@ func (s *Server) VerifyConnection(cc ftpserver.ClientContext, user string, tlsCo
284284
}
285285

286286
func (s *Server) buildTLSConfig() {
287-
if certMgr != nil {
287+
if mgr := certMgr.Load(); mgr != nil {
288288
certID := common.DefaultTLSKeyPaidID
289289
if getConfigPath(s.binding.CertificateFile, "") != "" && getConfigPath(s.binding.CertificateKeyFile, "") != "" {
290290
certID = s.binding.GetAddress()
291291
}
292-
if !certMgr.HasCertificate(certID) {
292+
if !mgr.HasCertificate(certID) {
293293
return
294294
}
295295
s.tlsConfig = &tls.Config{
296-
GetCertificate: certMgr.GetCertificateFunc(certID),
296+
GetCertificate: mgr.GetCertificateFunc(certID),
297297
MinVersion: util.GetTLSVersion(s.binding.MinTLSVersion),
298298
CipherSuites: s.binding.ciphers,
299299
}
300300
logger.Debug(logSender, "", "configured TLS cipher suites for binding %q: %v, certID: %v",
301301
s.binding.GetAddress(), s.binding.ciphers, certID)
302302
if s.binding.isMutualTLSEnabled() {
303-
s.tlsConfig.ClientCAs = certMgr.GetRootCAs()
303+
s.tlsConfig.ClientCAs = mgr.GetRootCAs()
304304
s.tlsConfig.VerifyConnection = s.verifyTLSConnection
305305
switch s.binding.ClientAuthType {
306306
case 1:
@@ -329,7 +329,7 @@ func (s *Server) VerifyTLSConnectionState(_ ftpserver.ClientContext, cs tls.Conn
329329
}
330330

331331
func (s *Server) verifyTLSConnection(state tls.ConnectionState) error {
332-
if certMgr != nil {
332+
if mgr := certMgr.Load(); mgr != nil {
333333
var clientCrt *x509.Certificate
334334
var clientCrtName string
335335
if len(state.PeerCertificates) > 0 {
@@ -348,7 +348,7 @@ func (s *Server) verifyTLSConnection(state tls.ConnectionState) error {
348348
if len(verifiedChain) > 0 {
349349
caCrt = verifiedChain[len(verifiedChain)-1]
350350
}
351-
if certMgr.IsRevoked(clientCrt, caCrt) {
351+
if mgr.IsRevoked(clientCrt, caCrt) {
352352
logger.Debug(logSender, "", "tls handshake error, client certificate %q has beed revoked", clientCrtName)
353353
return common.ErrCrtRevoked
354354
}

0 commit comments

Comments
 (0)