kms: add a lock, secrets could be modified concurrently for cached users

also reduce the size of the JSON payload omitting empty secrets

dataprovider/bolt.go

@@ -576,7 +576,7 @@ func (p *BoltProvider) getUsers(limit int, offset int, order string) ([]User, er
 				}
 				user, err := joinUserAndFolders(v, folderBucket)
 				if err == nil {
-					user.HideConfidentialData()
+					user.PrepareForRendering()
 					users = append(users, user)
 				}
 				if len(users) >= limit {
@@ -591,7 +591,7 @@ func (p *BoltProvider) getUsers(limit int, offset int, order string) ([]User, er
 				}
 				user, err := joinUserAndFolders(v, folderBucket)
 				if err == nil {
-					user.HideConfidentialData()
+					user.PrepareForRendering()
 					users = append(users, user)
 				}
 				if len(users) >= limit {
@@ -649,7 +649,7 @@ func (p *BoltProvider) getFolders(limit, offset int, order string) ([]vfs.BaseVi
 				if err != nil {
 					return err
 				}
-				folder.HideConfidentialData()
+				folder.PrepareForRendering()
 				folders = append(folders, folder)
 				if len(folders) >= limit {
 					break
@@ -666,7 +666,7 @@ func (p *BoltProvider) getFolders(limit, offset int, order string) ([]vfs.BaseVi
 				if err != nil {
 					return err
 				}
-				folder.HideConfidentialData()
+				folder.PrepareForRendering()
 				folders = append(folders, folder)
 				if len(folders) >= limit {
 					break

dataprovider/dataprovider.go

@@ -2224,7 +2224,7 @@ func ExecutePostLoginHook(user *User, loginMethod, ip, protocol string, err erro
 			status = "1"
 		}
 
-		user.HideConfidentialData()
+		user.PrepareForRendering()
 		userAsJSON, err := json.Marshal(user)
 		if err != nil {
 			providerLog(logger.LevelWarn, "error serializing user in post login hook: %v", err)
@@ -2422,7 +2422,7 @@ func executeAction(operation string, user *User) {
 			}
 			user = &u
 		}
-		user.HideConfidentialData()
+		user.PrepareForRendering()
 		userAsJSON, err := json.Marshal(user)
 		if err != nil {
 			providerLog(logger.LevelWarn, "unable to serialize user as JSON for operation %#v: %v", operation, err)

dataprovider/memory.go

@@ -330,7 +330,7 @@ func (p *MemoryProvider) getUsers(limit int, offset int, order string) ([]User,
 			}
 			u := p.dbHandle.users[username]
 			user := u.getACopy()
-			user.HideConfidentialData()
+			user.PrepareForRendering()
 			users = append(users, user)
 			if len(users) >= limit {
 				break
@@ -345,7 +345,7 @@ func (p *MemoryProvider) getUsers(limit int, offset int, order string) ([]User,
 			username := p.dbHandle.usernames[i]
 			u := p.dbHandle.users[username]
 			user := u.getACopy()
-			user.HideConfidentialData()
+			user.PrepareForRendering()
 			users = append(users, user)
 			if len(users) >= limit {
 				break
@@ -635,7 +635,7 @@ func (p *MemoryProvider) getFolders(limit, offset int, order string) ([]vfs.Base
 			}
 			f := p.dbHandle.vfolders[name]
 			folder := f.GetACopy()
-			folder.HideConfidentialData()
+			folder.PrepareForRendering()
 			folders = append(folders, folder)
 			if len(folders) >= limit {
 				break
@@ -650,7 +650,7 @@ func (p *MemoryProvider) getFolders(limit, offset int, order string) ([]vfs.Base
 			name := p.dbHandle.vfoldersNames[i]
 			f := p.dbHandle.vfolders[name]
 			folder := f.GetACopy()
-			folder.HideConfidentialData()
+			folder.PrepareForRendering()
 			folders = append(folders, folder)
 			if len(folders) >= limit {
 				break

dataprovider/sqlcommon.go

@@ -484,7 +484,7 @@ func sqlCommonGetUsers(limit int, offset int, order string, dbHandle sqlQuerier)
 			if err != nil {
 				return users, err
 			}
-			u.HideConfidentialData()
+			u.PrepareForRendering()
 			users = append(users, u)
 		}
 	}
@@ -832,7 +832,7 @@ func sqlCommonGetFolders(limit, offset int, order string, dbHandle sqlQuerier) (
 				folder.FsConfig = fs
 			}
 		}
-		folder.HideConfidentialData()
+		folder.PrepareForRendering()
 		folders = append(folders, folder)
 	}
 

dataprovider/user.go

@@ -278,8 +278,8 @@ func (u *User) CheckFsRoot(connectionID string) error {
 	return nil
 }
 
-// HideConfidentialData hides user confidential data
-func (u *User) HideConfidentialData() {
+// hideConfidentialData hides user confidential data
+func (u *User) hideConfidentialData() {
 	u.Password = ""
 	switch u.FsConfig.Provider {
 	case vfs.S3FilesystemProvider:
@@ -294,9 +294,17 @@ func (u *User) HideConfidentialData() {
 		u.FsConfig.SFTPConfig.Password.Hide()
 		u.FsConfig.SFTPConfig.PrivateKey.Hide()
 	}
+}
+
+// PrepareForRendering prepares a user for rendering.
+// It hides confidential data and set to nil the empty secrets
+// so they are not serialized
+func (u *User) PrepareForRendering() {
+	u.hideConfidentialData()
+	u.FsConfig.SetNilSecretsIfEmpty()
 	for idx := range u.VirtualFolders {
 		folder := &u.VirtualFolders[idx]
-		folder.HideConfidentialData()
+		folder.PrepareForRendering()
 	}
 }
 

httpd/api_folder.go

@@ -88,7 +88,7 @@ func renderFolder(w http.ResponseWriter, r *http.Request, name string, status in
 		sendAPIResponse(w, r, err, "", getRespStatus(err))
 		return
 	}
-	folder.HideConfidentialData()
+	folder.PrepareForRendering()
 	if status != http.StatusOK {
 		ctx := context.WithValue(r.Context(), render.StatusCtxKey, status)
 		render.JSON(w, r.WithContext(ctx), folder)

httpd/api_user.go

@@ -40,7 +40,7 @@ func renderUser(w http.ResponseWriter, r *http.Request, username string, status
 		sendAPIResponse(w, r, err, "", getRespStatus(err))
 		return
 	}
-	user.HideConfidentialData()
+	user.PrepareForRendering()
 	if status != http.StatusOK {
 		ctx := context.WithValue(r.Context(), render.StatusCtxKey, status)
 		render.JSON(w, r.WithContext(ctx), user)

httpd/httpd_test.go

@@ -1400,7 +1400,7 @@ func TestUserS3Config(t *testing.T) {
 	user.FsConfig.S3Config.UploadConcurrency = 4
 	user, body, err = httpdtest.UpdateUser(user, http.StatusOK, "")
 	assert.NoError(t, err, string(body))
-	assert.True(t, user.FsConfig.S3Config.AccessSecret.IsEmpty())
+	assert.Nil(t, user.FsConfig.S3Config.AccessSecret)
 	_, err = httpdtest.RemoveUser(user, http.StatusOK)
 	assert.NoError(t, err)
 	user.Password = defaultPassword
@@ -1408,7 +1408,7 @@ func TestUserS3Config(t *testing.T) {
 	// shared credential test for add instead of update
 	user, _, err = httpdtest.AddUser(user, http.StatusCreated)
 	assert.NoError(t, err)
-	assert.True(t, user.FsConfig.S3Config.AccessSecret.IsEmpty())
+	assert.Nil(t, user.FsConfig.S3Config.AccessSecret)
 	_, err = httpdtest.RemoveUser(user, http.StatusOK)
 	assert.NoError(t, err)
 }
@@ -1551,7 +1551,7 @@ func TestUserAzureBlobConfig(t *testing.T) {
 	user.FsConfig.AzBlobConfig.UploadConcurrency = 4
 	user, _, err = httpdtest.UpdateUser(user, http.StatusOK, "")
 	assert.NoError(t, err)
-	assert.True(t, user.FsConfig.AzBlobConfig.AccountKey.IsEmpty())
+	assert.Nil(t, user.FsConfig.AzBlobConfig.AccountKey)
 	_, err = httpdtest.RemoveUser(user, http.StatusOK)
 	assert.NoError(t, err)
 	user.Password = defaultPassword
@@ -1559,7 +1559,7 @@ func TestUserAzureBlobConfig(t *testing.T) {
 	// sas test for add instead of update
 	user, _, err = httpdtest.AddUser(user, http.StatusCreated)
 	assert.NoError(t, err)
-	assert.True(t, user.FsConfig.AzBlobConfig.AccountKey.IsEmpty())
+	assert.Nil(t, user.FsConfig.AzBlobConfig.AccountKey)
 	_, err = httpdtest.RemoveUser(user, http.StatusOK)
 	assert.NoError(t, err)
 }
@@ -1681,7 +1681,7 @@ func TestUserSFTPFs(t *testing.T) {
 	user, _, err = httpdtest.AddUser(user, http.StatusCreated)
 	assert.NoError(t, err)
 	initialPkeyPayload = user.FsConfig.SFTPConfig.PrivateKey.GetPayload()
-	assert.Empty(t, user.FsConfig.SFTPConfig.Password.GetStatus())
+	assert.Nil(t, user.FsConfig.SFTPConfig.Password)
 	assert.Equal(t, kms.SecretStatusSecretBox, user.FsConfig.SFTPConfig.PrivateKey.GetStatus())
 	assert.NotEmpty(t, initialPkeyPayload)
 	assert.Empty(t, user.FsConfig.SFTPConfig.PrivateKey.GetAdditionalData())
@@ -5811,7 +5811,7 @@ func TestWebUserS3Mock(t *testing.T) {
 	var userGet dataprovider.User
 	err = render.DecodeJSON(rr.Body, &userGet)
 	assert.NoError(t, err)
-	assert.True(t, userGet.FsConfig.S3Config.AccessSecret.IsEmpty())
+	assert.Nil(t, userGet.FsConfig.S3Config.AccessSecret)
 
 	req, _ = http.NewRequest(http.MethodDelete, path.Join(userPath, user.Username), nil)
 	setBearerForReq(req, apiToken)

kms/kms.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"os"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/drakkan/sftpgo/utils"
@@ -144,11 +145,15 @@ func (c *Configuration) getSecretProvider(base baseSecret) SecretProvider {
 
 // Secret defines the struct used to store confidential data
 type Secret struct {
+	sync.RWMutex
 	provider SecretProvider
 }
 
 // MarshalJSON return the JSON encoding of the Secret object
 func (s *Secret) MarshalJSON() ([]byte, error) {
+	s.RLock()
+	defer s.RUnlock()
+
 	return json.Marshal(&baseSecret{
 		Status:         s.provider.GetStatus(),
 		Payload:        s.provider.GetPayload(),
@@ -161,6 +166,9 @@ func (s *Secret) MarshalJSON() ([]byte, error) {
 // UnmarshalJSON parses the JSON-encoded data and stores the result
 // in the Secret object
 func (s *Secret) UnmarshalJSON(data []byte) error {
+	s.Lock()
+	defer s.Unlock()
+
 	baseSecret := baseSecret{}
 	err := json.Unmarshal(data, &baseSecret)
 	if err != nil {
@@ -191,6 +199,9 @@ func (s *Secret) UnmarshalJSON(data []byte) error {
 
 // Clone returns a copy of the secret object
 func (s *Secret) Clone() *Secret {
+	s.RLock()
+	defer s.RUnlock()
+
 	baseSecret := baseSecret{
 		Status:         s.provider.GetStatus(),
 		Payload:        s.provider.GetPayload(),
@@ -227,11 +238,17 @@ func (s *Secret) Clone() *Secret {
 // This isn't a pointer receiver because we don't want to pass
 // a pointer to html template
 func (s *Secret) IsEncrypted() bool {
+	s.RLock()
+	defer s.RUnlock()
+
 	return s.provider.IsEncrypted()
 }
 
 // IsPlain returns true if the secret is in plain text
 func (s *Secret) IsPlain() bool {
+	s.RLock()
+	defer s.RUnlock()
+
 	return s.provider.GetStatus() == SecretStatusPlain
 }
 
@@ -239,56 +256,89 @@ func (s *Secret) IsPlain() bool {
 // This is an utility method, we update the secret for an existing user
 // if it is empty or plain
 func (s *Secret) IsNotPlainAndNotEmpty() bool {
+	s.RLock()
+	defer s.RUnlock()
+
 	return !s.IsPlain() && !s.IsEmpty()
 }
 
 // IsRedacted returns true if the secret is redacted
 func (s *Secret) IsRedacted() bool {
+	s.RLock()
+	defer s.RUnlock()
+
 	return s.provider.GetStatus() == SecretStatusRedacted
 }
 
 // GetPayload returns the secret payload
 func (s *Secret) GetPayload() string {
+	s.RLock()
+	defer s.RUnlock()
+
 	return s.provider.GetPayload()
 }
 
 // GetAdditionalData returns the secret additional data
 func (s *Secret) GetAdditionalData() string {
+	s.RLock()
+	defer s.RUnlock()
+
 	return s.provider.GetAdditionalData()
 }
 
 // GetStatus returns the secret status
 func (s *Secret) GetStatus() SecretStatus {
+	s.RLock()
+	defer s.RUnlock()
+
 	return s.provider.GetStatus()
 }
 
 // GetKey returns the secret key
 func (s *Secret) GetKey() string {
+	s.RLock()
+	defer s.RUnlock()
+
 	return s.provider.GetKey()
 }
 
 // GetMode returns the secret mode
 func (s *Secret) GetMode() int {
+	s.RLock()
+	defer s.RUnlock()
+
 	return s.provider.GetMode()
 }
 
 // SetAdditionalData sets the given additional data
 func (s *Secret) SetAdditionalData(value string) {
+	s.Lock()
+	defer s.Unlock()
+
 	s.provider.SetAdditionalData(value)
 }
 
 // SetStatus sets the status for this secret
 func (s *Secret) SetStatus(value SecretStatus) {
+	s.Lock()
+	defer s.Unlock()
+
 	s.provider.SetStatus(value)
 }
 
 // SetKey sets the key for this secret
 func (s *Secret) SetKey(value string) {
+	s.Lock()
+	defer s.Unlock()
+
 	s.provider.SetKey(value)
 }
 
 // IsEmpty returns true if all fields are empty
 func (s *Secret) IsEmpty() bool {
+	s.RLock()
+	defer s.RUnlock()
+
 	if s.provider.GetStatus() != "" {
 		return false
 	}
@@ -306,6 +356,9 @@ func (s *Secret) IsEmpty() bool {
 
 // IsValid returns true if the secret is not empty and valid
 func (s *Secret) IsValid() bool {
+	s.RLock()
+	defer s.RUnlock()
+
 	if !s.IsValidInput() {
 		return false
 	}
@@ -325,6 +378,9 @@ func (s *Secret) IsValid() bool {
 
 // IsValidInput returns true if the secret is a valid user input
 func (s *Secret) IsValidInput() bool {
+	s.RLock()
+	defer s.RUnlock()
+
 	if !utils.IsStringInSlice(s.provider.GetStatus(), validSecretStatuses) {
 		return false
 	}
@@ -336,16 +392,25 @@ func (s *Secret) IsValidInput() bool {
 
 // Hide hides info to decrypt data
 func (s *Secret) Hide() {
+	s.Lock()
+	defer s.Unlock()
+
 	s.provider.SetKey("")
 	s.provider.SetAdditionalData("")
 }
 
 // Encrypt encrypts a plain text Secret object
 func (s *Secret) Encrypt() error {
+	s.Lock()
+	defer s.Unlock()
+
 	return s.provider.Encrypt()
 }
 
 // Decrypt decrypts a Secret object
 func (s *Secret) Decrypt() error {
+	s.Lock()
+	defer s.Unlock()
+
 	return s.provider.Decrypt()
 }

vfs/filesystem.go

@@ -48,6 +48,30 @@ func (f *Filesystem) SetEmptySecretsIfNil() {
 	}
 }
 
+// SetNilSecretsIfEmpty set the secrets to nil if empty.
+// This is useful before rendering as JSON so the empty fields
+// will not be serialized.
+func (f *Filesystem) SetNilSecretsIfEmpty() {
+	if f.S3Config.AccessSecret != nil && f.S3Config.AccessSecret.IsEmpty() {
+		f.S3Config.AccessSecret = nil
+	}
+	if f.GCSConfig.Credentials != nil && f.GCSConfig.Credentials.IsEmpty() {
+		f.GCSConfig.Credentials = nil
+	}
+	if f.AzBlobConfig.AccountKey != nil && f.AzBlobConfig.AccountKey.IsEmpty() {
+		f.AzBlobConfig.AccountKey = nil
+	}
+	if f.CryptConfig.Passphrase != nil && f.CryptConfig.Passphrase.IsEmpty() {
+		f.CryptConfig.Passphrase = nil
+	}
+	if f.SFTPConfig.Password != nil && f.SFTPConfig.Password.IsEmpty() {
+		f.SFTPConfig.Password = nil
+	}
+	if f.SFTPConfig.PrivateKey != nil && f.SFTPConfig.PrivateKey.IsEmpty() {
+		f.SFTPConfig.PrivateKey = nil
+	}
+}
+
 // GetACopy returns a copy
 func (f *Filesystem) GetACopy() Filesystem {
 	f.SetEmptySecretsIfNil()

vfs/folder.go

@@ -79,8 +79,8 @@ func (v *BaseVirtualFolder) IsLocalOrLocalCrypted() bool {
 	return v.FsConfig.Provider == LocalFilesystemProvider || v.FsConfig.Provider == CryptedFilesystemProvider
 }
 
-// HideConfidentialData hides folder confidential data
-func (v *BaseVirtualFolder) HideConfidentialData() {
+// hideConfidentialData hides folder confidential data
+func (v *BaseVirtualFolder) hideConfidentialData() {
 	switch v.FsConfig.Provider {
 	case S3FilesystemProvider:
 		v.FsConfig.S3Config.AccessSecret.Hide()
@@ -96,6 +96,14 @@ func (v *BaseVirtualFolder) HideConfidentialData() {
 	}
 }
 
+// PrepareForRendering prepares a folder for rendering.
+// It hides confidential data and set to nil the empty secrets
+// so they are not serialized
+func (v *BaseVirtualFolder) PrepareForRendering() {
+	v.hideConfidentialData()
+	v.FsConfig.SetEmptySecretsIfNil()
+}
+
 // HasRedactedSecret returns true if the folder has a redacted secret
 func (v *BaseVirtualFolder) HasRedactedSecret() bool {
 	switch v.FsConfig.Provider {

webdavd/internal_test.go

@@ -1331,7 +1331,6 @@ func TestUserCacheIsolation(t *testing.T) {
 	if assert.True(t, ok) {
 		cachedUser := result.(*dataprovider.CachedUser).User
 		assert.Equal(t, vfs.LocalFilesystemProvider, cachedUser.FsConfig.Provider)
-		// FIXME: should we really allow to modify the cached users concurrently?????
 		assert.False(t, cachedUser.FsConfig.S3Config.AccessSecret.IsEncrypted())
 	}