fix(sftpd): harden bcrypt migration and auth path

- SetPassword now returns an error instead of silently truncating passwords
  >72 bytes (the prior fallback could also panic for short passwords when
  bcrypt returned any non-ErrPasswordTooLong error).
- CheckPassword is now pure and reports (ok, legacy); migration moves to
  FileStore.migrateLegacyPassword under a write lock to fix a data race on
  concurrent logins and a double-check avoids redundant re-hashing.
- ValidatePassword only rewrites the user store when a migration actually
  happened, eliminating a full file rewrite on every successful login.
- Legacy plaintext comparison uses subtle.ConstantTimeCompare again,
  restoring the pre-PR constant-time property.
- CreateUser propagates SetPassword errors and releases the write lock
  before calling saveUsers to avoid recursive RWMutex locking.
- Add unit tests covering new-user hashing, legacy migration, no-rewrite
  on normal login, wrong/empty passwords, oversize password rejection,
  and concurrent migration under -race.

weed/sftpd/user/filestore.go

@@ -8,6 +8,7 @@ import (
 	"path"
 	"sync"
 
+	"github.com/seaweedfs/seaweedfs/weed/glog"
 	"golang.org/x/crypto/ssh"
 )
 
@@ -147,15 +148,45 @@ func (s *FileStore) GetUser(username string) (*User, error) {
 	return user, nil
 }
 
-// ValidatePassword checks if the password is valid for the user
+// ValidatePassword checks if the password is valid for the user. Legacy
+// plaintext entries are transparently upgraded to bcrypt on successful match.
 func (s *FileStore) ValidatePassword(username string, password []byte) bool {
 	user, err := s.GetUser(username)
 	if err != nil {
 		return false
 	}
 
-	// Compare plaintext password using constant time comparison for security
-	return subtle.ConstantTimeCompare([]byte(user.Password), password) == 1
+	ok, legacy := user.CheckPassword(string(password))
+	if !ok {
+		return false
+	}
+	if legacy {
+		s.migrateLegacyPassword(user, string(password))
+	}
+	return true
+}
+
+// migrateLegacyPassword re-hashes a legacy plaintext password under a write
+// lock and persists the user store. The FileStore mutex guards both the
+// re-hash (to avoid a data race with concurrent logins) and the double-check
+// that another goroutine hasn't already migrated. Failures are logged but
+// not surfaced — auth has already succeeded and the next login will retry.
+func (s *FileStore) migrateLegacyPassword(user *User, password string) {
+	s.mu.Lock()
+	if user.HashedPassword != "" {
+		s.mu.Unlock()
+		return
+	}
+	if err := user.SetPassword(password); err != nil {
+		s.mu.Unlock()
+		glog.V(0).Infof("sftpd: failed to upgrade legacy password for %s: %v", user.Username, err)
+		return
+	}
+	s.mu.Unlock()
+
+	if err := s.saveUsers(); err != nil {
+		glog.V(0).Infof("sftpd: failed to persist upgraded password for %s: %v", user.Username, err)
+	}
 }
 
 // ValidatePublicKey checks if the public key is valid for the user
@@ -239,25 +270,21 @@ func (s *FileStore) ListUsers() ([]string, error) {
 
 // CreateUser creates a new user with the given username and password
 func (s *FileStore) CreateUser(username, password string) (*User, error) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	// Check if user already exists
-	if _, exists := s.users[username]; exists {
-		return nil, fmt.Errorf("user already exists: %s", username)
-	}
-
-	// Create new user
 	user := NewUser(username)
-
-	// Store plaintext password
-	user.Password = password
-
-	// Add default permissions
+	if err := user.SetPassword(password); err != nil {
+		return nil, err
+	}
 	user.Permissions[user.HomeDir] = []string{"all"}
 
-	// Save the user
+	s.mu.Lock()
+	if _, exists := s.users[username]; exists {
+		s.mu.Unlock()
+		return nil, fmt.Errorf("user already exists: %s", username)
+	}
 	s.users[username] = user
+	s.mu.Unlock()
+
+	// saveUsers acquires RLock, so we must release the write lock above first.
 	if err := s.saveUsers(); err != nil {
 		return nil, err
 	}

weed/sftpd/user/filestore_test.go

@@ -0,0 +1,195 @@
+package user
+
+import (
+	"bytes"
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
+	"testing"
+
+	"golang.org/x/crypto/bcrypt"
+)
+
+func newTempStore(t *testing.T) *FileStore {
+	t.Helper()
+	dir := t.TempDir()
+	s, err := NewFileStore(filepath.Join(dir, "users.json"))
+	if err != nil {
+		t.Fatalf("NewFileStore: %v", err)
+	}
+	return s
+}
+
+func readUsers(t *testing.T, s *FileStore) []*User {
+	t.Helper()
+	data, err := os.ReadFile(s.filePath)
+	if err != nil {
+		t.Fatalf("read store file: %v", err)
+	}
+	var users []*User
+	if err := json.Unmarshal(data, &users); err != nil {
+		t.Fatalf("unmarshal store file: %v", err)
+	}
+	return users
+}
+
+func TestCreateUserHashesPassword(t *testing.T) {
+	s := newTempStore(t)
+	u, err := s.CreateUser("alice", "s3cret")
+	if err != nil {
+		t.Fatalf("CreateUser: %v", err)
+	}
+	if u.Password != "" {
+		t.Errorf("plaintext Password leaked: %q", u.Password)
+	}
+	if u.HashedPassword == "" {
+		t.Fatal("HashedPassword empty")
+	}
+	if err := bcrypt.CompareHashAndPassword([]byte(u.HashedPassword), []byte("s3cret")); err != nil {
+		t.Fatalf("hash does not match password: %v", err)
+	}
+	stored := readUsers(t, s)
+	if len(stored) != 1 || stored[0].Password != "" {
+		t.Errorf("plaintext persisted: %+v", stored)
+	}
+}
+
+func TestValidatePasswordMigratesLegacyPlaintext(t *testing.T) {
+	s := newTempStore(t)
+	legacy := NewUser("bob")
+	legacy.Password = "hunter2"
+	if err := s.SaveUser(legacy); err != nil {
+		t.Fatalf("SaveUser: %v", err)
+	}
+
+	if !s.ValidatePassword("bob", []byte("hunter2")) {
+		t.Fatal("legacy login rejected")
+	}
+	u, err := s.GetUser("bob")
+	if err != nil {
+		t.Fatalf("GetUser: %v", err)
+	}
+	if u.Password != "" {
+		t.Errorf("legacy Password not cleared: %q", u.Password)
+	}
+	if u.HashedPassword == "" {
+		t.Fatal("HashedPassword not populated after migration")
+	}
+
+	stored := readUsers(t, s)
+	if len(stored) != 1 || stored[0].Password != "" || stored[0].HashedPassword == "" {
+		t.Errorf("migration not persisted: %+v", stored)
+	}
+}
+
+// Successful logins that don't migrate must not rewrite the store file.
+// We detect a rewrite by replacing the file contents with a sentinel after
+// CreateUser and verifying the sentinel is untouched after a valid login.
+func TestValidatePasswordNoRewriteWithoutMigration(t *testing.T) {
+	s := newTempStore(t)
+	if _, err := s.CreateUser("carol", "pw"); err != nil {
+		t.Fatalf("CreateUser: %v", err)
+	}
+
+	sentinel := []byte("SENTINEL_NOT_REAL_JSON")
+	if err := os.WriteFile(s.filePath, sentinel, 0600); err != nil {
+		t.Fatalf("write sentinel: %v", err)
+	}
+
+	if !s.ValidatePassword("carol", []byte("pw")) {
+		t.Fatal("login rejected")
+	}
+	got, err := os.ReadFile(s.filePath)
+	if err != nil {
+		t.Fatalf("read: %v", err)
+	}
+	if !bytes.Equal(got, sentinel) {
+		t.Errorf("store file was rewritten on non-migrating login: got %q", got)
+	}
+}
+
+func TestValidatePasswordWrongPassword(t *testing.T) {
+	s := newTempStore(t)
+	if _, err := s.CreateUser("dave", "correct"); err != nil {
+		t.Fatalf("CreateUser: %v", err)
+	}
+	if s.ValidatePassword("dave", []byte("wrong")) {
+		t.Error("wrong password accepted for bcrypt user")
+	}
+
+	legacy := NewUser("eve")
+	legacy.Password = "plain"
+	if err := s.SaveUser(legacy); err != nil {
+		t.Fatal(err)
+	}
+	if s.ValidatePassword("eve", []byte("wrong")) {
+		t.Error("wrong password accepted for legacy user")
+	}
+	if s.ValidatePassword("eve", []byte("")) {
+		t.Error("empty password accepted for legacy user")
+	}
+}
+
+func TestValidatePasswordUnknownUser(t *testing.T) {
+	s := newTempStore(t)
+	if s.ValidatePassword("nobody", []byte("whatever")) {
+		t.Error("unknown user accepted")
+	}
+}
+
+func TestCheckPasswordEmptyCredentials(t *testing.T) {
+	u := NewUser("frank")
+	if ok, _ := u.CheckPassword(""); ok {
+		t.Error("empty password accepted on user with no credentials")
+	}
+	if ok, _ := u.CheckPassword("x"); ok {
+		t.Error("password accepted on user with no credentials")
+	}
+}
+
+func TestSetPasswordTooLong(t *testing.T) {
+	u := NewUser("grace")
+	if err := u.SetPassword(strings.Repeat("a", 73)); err == nil {
+		t.Error("expected error for >72 byte password")
+	}
+	if u.HashedPassword != "" {
+		t.Errorf("HashedPassword set despite error: %q", u.HashedPassword)
+	}
+}
+
+// Concurrent ValidatePassword calls on a legacy user must not race on the
+// User fields and must converge to a single persisted bcrypt hash.
+func TestValidatePasswordConcurrentMigration(t *testing.T) {
+	s := newTempStore(t)
+	legacy := NewUser("heidi")
+	legacy.Password = "p"
+	if err := s.SaveUser(legacy); err != nil {
+		t.Fatal(err)
+	}
+
+	const N = 16
+	var wg sync.WaitGroup
+	wg.Add(N)
+	for i := 0; i < N; i++ {
+		go func() {
+			defer wg.Done()
+			if !s.ValidatePassword("heidi", []byte("p")) {
+				t.Error("login rejected")
+			}
+		}()
+	}
+	wg.Wait()
+
+	u, err := s.GetUser("heidi")
+	if err != nil {
+		t.Fatalf("GetUser: %v", err)
+	}
+	if u.Password != "" {
+		t.Errorf("plaintext not cleared after concurrent migration: %q", u.Password)
+	}
+	if err := bcrypt.CompareHashAndPassword([]byte(u.HashedPassword), []byte("p")); err != nil {
+		t.Errorf("final hash invalid: %v", err)
+	}
+}

weed/sftpd/user/user.go

@@ -2,19 +2,24 @@
 package user
 
 import (
+	"crypto/subtle"
+	"fmt"
 	"math/rand/v2"
 	"path/filepath"
+
+	"golang.org/x/crypto/bcrypt"
 )
 
 // User represents an SFTP user with authentication and permission details
 type User struct {
-	Username    string              // Username for authentication
-	Password    string              // Plaintext password
-	PublicKeys  []string            // Authorized public keys
-	HomeDir     string              // User's home directory
-	Permissions map[string][]string // path -> permissions (read, write, list, etc.)
-	Uid         uint32              // User ID for file ownership
-	Gid         uint32              // Group ID for file ownership
+	Username       string              `json:"Username"`
+	HashedPassword string              `json:"HashedPassword"`     // bcrypt hash
+	Password       string              `json:"Password,omitempty"` // deprecated: plaintext, migrated to HashedPassword on first successful login
+	PublicKeys     []string            `json:"PublicKeys,omitempty"`
+	HomeDir        string              `json:"HomeDir"`
+	Permissions    map[string][]string `json:"Permissions,omitempty"`
+	Uid            uint32              `json:"Uid"`
+	Gid            uint32              `json:"Gid"`
 }
 
 // NewUser creates a new user with default settings
@@ -33,9 +38,31 @@ func NewUser(username string) *User {
 	}
 }
 
-// SetPassword sets a plaintext password for the user
-func (u *User) SetPassword(password string) {
-	u.Password = password
+// SetPassword hashes and stores the password using bcrypt. It clears any
+// legacy plaintext value. Returns an error if the password cannot be hashed
+// (e.g. exceeds bcrypt's 72-byte limit).
+func (u *User) SetPassword(password string) error {
+	hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost)
+	if err != nil {
+		return fmt.Errorf("hash password: %w", err)
+	}
+	u.HashedPassword = string(hash)
+	u.Password = ""
+	return nil
+}
+
+// CheckPassword verifies a password against the stored credentials without
+// mutating the receiver. The legacy return is true when the match succeeded
+// via the deprecated plaintext field; the caller is expected to re-hash and
+// persist the password under an appropriate lock.
+func (u *User) CheckPassword(password string) (ok bool, legacy bool) {
+	if u.HashedPassword != "" {
+		return bcrypt.CompareHashAndPassword([]byte(u.HashedPassword), []byte(password)) == nil, false
+	}
+	if u.Password != "" && subtle.ConstantTimeCompare([]byte(u.Password), []byte(password)) == 1 {
+		return true, true
+	}
+	return false, false
 }
 
 // AddPublicKey adds a public key to the user