Skip to content

Commit fb45585

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

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,
@@ -575,8 +576,9 @@ func TestServerGetSettings(t *testing.T) {
575576
ID: common.DefaultTLSKeyPaidID,
576577
},
577578
}
578-
certMgr, err = common.NewCertManager(keyPairs, configDir, "")
579+
mgr, err := common.NewCertManager(keyPairs, configDir, "")
579580
require.NoError(t, err)
581+
certMgr.Store(mgr)
580582
common.Config.ProxyAllowed = nil
581583
c.CertificateFile = certPath
582584
c.CertificateKeyFile = keyPath
@@ -595,7 +597,7 @@ func TestServerGetSettings(t *testing.T) {
595597
assert.True(t, ok)
596598

597599
common.Config = oldConfig
598-
certMgr = oldMgr
600+
certMgr.Store(oldMgr)
599601
}
600602

601603
func TestUserInvalidParams(t *testing.T) {
@@ -960,7 +962,7 @@ func TestTransferErrors(t *testing.T) {
960962
}
961963

962964
func TestVerifyTLSConnection(t *testing.T) {
963-
oldCertMgr := certMgr
965+
oldCertMgr := certMgr.Load()
964966

965967
caCrlPath := filepath.Join(os.TempDir(), "testcrl.crt")
966968
certPath := filepath.Join(os.TempDir(), "test.crt")
@@ -978,11 +980,12 @@ func TestVerifyTLSConnection(t *testing.T) {
978980
ID: common.DefaultTLSKeyPaidID,
979981
},
980982
}
981-
certMgr, err = common.NewCertManager(keyPairs, "", "ftp_test")
983+
mgr, err := common.NewCertManager(keyPairs, "", "ftp_test")
982984
assert.NoError(t, err)
985+
certMgr.Store(mgr)
983986

984-
certMgr.SetCARevocationLists([]string{caCrlPath})
985-
err = certMgr.LoadCRLs()
987+
certMgr.Load().SetCARevocationLists([]string{caCrlPath})
988+
err = certMgr.Load().LoadCRLs()
986989
assert.NoError(t, err)
987990

988991
crt, err := tls.X509KeyPair([]byte(client1Crt), []byte(client1Key))
@@ -1030,7 +1033,7 @@ func TestVerifyTLSConnection(t *testing.T) {
10301033
err = os.Remove(keyPath)
10311034
assert.NoError(t, err)
10321035

1033-
certMgr = oldCertMgr
1036+
certMgr.Store(oldCertMgr)
10341037
}
10351038

10361039
func TestCiphers(t *testing.T) {

‎internal/ftpd/server.go‎

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

122-
if s.binding.TLSMode > 0 && certMgr == nil {
122+
if s.binding.TLSMode > 0 && certMgr.Load() == nil {
123123
return nil, errors.New("to enable TLS you need to provide a certificate")
124124
}
125125

@@ -306,23 +306,23 @@ func (s *Server) VerifyConnection(cc ftpserver.ClientContext, user string, tlsCo
306306
}
307307

308308
func (s *Server) buildTLSConfig() {
309-
if certMgr != nil {
309+
if mgr := certMgr.Load(); mgr != nil {
310310
certID := common.DefaultTLSKeyPaidID
311311
if getConfigPath(s.binding.CertificateFile, "") != "" && getConfigPath(s.binding.CertificateKeyFile, "") != "" {
312312
certID = s.binding.GetAddress()
313313
}
314-
if !certMgr.HasCertificate(certID) {
314+
if !mgr.HasCertificate(certID) {
315315
return
316316
}
317317
s.tlsConfig = &tls.Config{
318-
GetCertificate: certMgr.GetCertificateFunc(certID),
318+
GetCertificate: mgr.GetCertificateFunc(certID),
319319
MinVersion: util.GetTLSVersion(s.binding.MinTLSVersion),
320320
CipherSuites: s.binding.ciphers,
321321
}
322322
logger.Debug(logSender, "", "configured TLS cipher suites for binding %q: %v, certID: %v",
323323
s.binding.GetAddress(), s.binding.ciphers, certID)
324324
if s.binding.isMutualTLSEnabled() {
325-
s.tlsConfig.ClientCAs = certMgr.GetRootCAs()
325+
s.tlsConfig.ClientCAs = mgr.GetRootCAs()
326326
s.tlsConfig.VerifyConnection = s.verifyTLSConnection
327327
switch s.binding.ClientAuthType {
328328
case 1:
@@ -351,7 +351,7 @@ func (s *Server) VerifyTLSConnectionState(_ ftpserver.ClientContext, cs tls.Conn
351351
}
352352

353353
func (s *Server) verifyTLSConnection(state tls.ConnectionState) error {
354-
if certMgr != nil {
354+
if mgr := certMgr.Load(); mgr != nil {
355355
var clientCrt *x509.Certificate
356356
var clientCrtName string
357357
if len(state.PeerCertificates) > 0 {
@@ -370,7 +370,7 @@ func (s *Server) verifyTLSConnection(state tls.ConnectionState) error {
370370
if len(verifiedChain) > 0 {
371371
caCrt = verifiedChain[len(verifiedChain)-1]
372372
}
373-
if certMgr.IsRevoked(clientCrt, caCrt) {
373+
if mgr.IsRevoked(clientCrt, caCrt) {
374374
logger.Debug(logSender, "", "tls handshake error, client certificate %q has beed revoked", clientCrtName)
375375
return common.ErrCrtRevoked
376376
}

0 commit comments

Comments
 (0)