lib/config: Move the bcrypt password hashing to GUIConfiguration (#8028)

What hash is used to store the password should ideally be an
implementation detail, so that every user of the GUIConfiguration
object automatically agrees on how to handle it.  That is currently
distribututed over the confighandler.go and api_auth.go files, plus
tests.

Add the SetHasedPassword() / CompareHashedPassword() API to keep the
hashing method encapsulated.  Add a separate test for it and adjust
other users and tests.  Remove all deprecated imports of the bcrypt
package.

lib/api/api_auth.go

@@ -19,7 +19,6 @@ import (
 	"github.com/syncthing/syncthing/lib/events"
 	"github.com/syncthing/syncthing/lib/rand"
 	"github.com/syncthing/syncthing/lib/sync"
-	"golang.org/x/crypto/bcrypt"
 )
 
 var (
@@ -117,14 +116,12 @@ func auth(username string, password string, guiCfg config.GUIConfiguration, ldap
 	if guiCfg.AuthMode == config.AuthModeLDAP {
 		return authLDAP(username, password, ldapCfg)
 	} else {
-		return authStatic(username, password, guiCfg.User, guiCfg.Password)
+		return authStatic(username, password, guiCfg)
 	}
 }
 
-func authStatic(username string, password string, configUser string, configPassword string) bool {
-	configPasswordBytes := []byte(configPassword)
-	passwordBytes := []byte(password)
-	return bcrypt.CompareHashAndPassword(configPasswordBytes, passwordBytes) == nil && username == configUser
+func authStatic(username string, password string, guiCfg config.GUIConfiguration) bool {
+	return guiCfg.CompareHashedPassword(password) == nil && username == guiCfg.User
 }
 
 func authLDAP(username string, password string, cfg config.LDAPConfiguration) bool {

lib/api/api_auth_test.go

@@ -9,19 +9,20 @@ package api
 import (
 	"testing"
 
-	"golang.org/x/crypto/bcrypt"
+	"github.com/syncthing/syncthing/lib/config"
 )
 
-var passwordHashBytes []byte
+var guiCfg config.GUIConfiguration
 
 func init() {
-	passwordHashBytes, _ = bcrypt.GenerateFromPassword([]byte("pass"), 0)
+	guiCfg.User = "user"
+	guiCfg.HashAndSetPassword("pass")
 }
 
 func TestStaticAuthOK(t *testing.T) {
 	t.Parallel()
 
-	ok := authStatic("user", "pass", "user", string(passwordHashBytes))
+	ok := authStatic("user", "pass", guiCfg)
 	if !ok {
 		t.Fatalf("should pass auth")
 	}
@@ -30,7 +31,7 @@ func TestStaticAuthOK(t *testing.T) {
 func TestSimpleAuthUsernameFail(t *testing.T) {
 	t.Parallel()
 
-	ok := authStatic("userWRONG", "pass", "user", string(passwordHashBytes))
+	ok := authStatic("userWRONG", "pass", guiCfg)
 	if ok {
 		t.Fatalf("should fail auth")
 	}
@@ -39,7 +40,7 @@ func TestSimpleAuthUsernameFail(t *testing.T) {
 func TestStaticAuthPasswordFail(t *testing.T) {
 	t.Parallel()
 
-	ok := authStatic("user", "passWRONG", "user", string(passwordHashBytes))
+	ok := authStatic("user", "passWRONG", guiCfg)
 	if ok {
 		t.Fatalf("should fail auth")
 	}

lib/api/confighandler.go

@@ -13,7 +13,6 @@ import (
 	"net/http"
 
 	"github.com/julienschmidt/httprouter"
-	"golang.org/x/crypto/bcrypt"
 
 	"github.com/syncthing/syncthing/lib/config"
 	"github.com/syncthing/syncthing/lib/protocol"
@@ -290,11 +289,13 @@ func (c *configMuxBuilder) adjustConfig(w http.ResponseWriter, r *http.Request)
 	var errMsg string
 	var status int
 	waiter, err := c.cfg.Modify(func(cfg *config.Configuration) {
-		if to.GUI.Password, err = checkGUIPassword(cfg.GUI.Password, to.GUI.Password); err != nil {
-			l.Warnln("bcrypting password:", err)
-			errMsg = err.Error()
-			status = http.StatusInternalServerError
-			return
+		if to.GUI.Password != cfg.GUI.Password {
+			if err := to.GUI.HashAndSetPassword(to.GUI.Password); err != nil {
+				l.Warnln("hashing password:", err)
+				errMsg = err.Error()
+				status = http.StatusInternalServerError
+				return
+			}
 		}
 		*cfg = to
 	})
@@ -370,11 +371,13 @@ func (c *configMuxBuilder) adjustGUI(w http.ResponseWriter, r *http.Request, gui
 	var errMsg string
 	var status int
 	waiter, err := c.cfg.Modify(func(cfg *config.Configuration) {
-		if gui.Password, err = checkGUIPassword(oldPassword, gui.Password); err != nil {
-			l.Warnln("bcrypting password:", err)
-			errMsg = err.Error()
-			status = http.StatusInternalServerError
-			return
+		if gui.Password != oldPassword {
+			if err := gui.HashAndSetPassword(gui.Password); err != nil {
+				l.Warnln("hashing password:", err)
+				errMsg = err.Error()
+				status = http.StatusInternalServerError
+				return
+			}
 		}
 		cfg.GUI = gui
 	})
@@ -418,14 +421,6 @@ func unmarshalToRawMessages(body io.ReadCloser) ([]json.RawMessage, error) {
 	return data, err
 }
 
-func checkGUIPassword(oldPassword, newPassword string) (string, error) {
-	if newPassword == oldPassword {
-		return newPassword, nil
-	}
-	hash, err := bcrypt.GenerateFromPassword([]byte(newPassword), 0)
-	return string(hash), err
-}
-
 func (c *configMuxBuilder) finish(w http.ResponseWriter, waiter config.Waiter) {
 	waiter.Wait()
 	if err := c.cfg.Save(); err != nil {

lib/config/config_test.go

@@ -739,6 +739,27 @@ func TestGUIConfigURL(t *testing.T) {
 	}
 }
 
+func TestGUIPasswordHash(t *testing.T) {
+	var c GUIConfiguration
+
+	testPass := "pass"
+	if err := c.HashAndSetPassword(testPass); err != nil {
+		t.Fatal(err)
+	}
+	if c.Password == testPass {
+		t.Error("Password hashing resulted in plaintext")
+	}
+
+	if err := c.CompareHashedPassword(testPass); err != nil {
+		t.Errorf("No match on same password: %v", err)
+	}
+
+	failPass := "different"
+	if err := c.CompareHashedPassword(failPass); err == nil {
+		t.Errorf("Match on different password: %v", err)
+	}
+}
+
 func TestDuplicateDevices(t *testing.T) {
 	// Duplicate devices should be removed
 

lib/config/guiconfiguration.go

@@ -12,6 +12,8 @@ import (
 	"strconv"
 	"strings"
 
+	"golang.org/x/crypto/bcrypt"
+
 	"github.com/syncthing/syncthing/lib/rand"
 )
 
@@ -113,6 +115,23 @@ func (c GUIConfiguration) URL() string {
 	return u.String()
 }
 
+// SetHashedPassword hashes the given plaintext password and stores the new hash.
+func (c *GUIConfiguration) HashAndSetPassword(password string) error {
+	hash, err := bcrypt.GenerateFromPassword([]byte(password), 0)
+	if err != nil {
+		return err
+	}
+	c.Password = string(hash)
+	return nil
+}
+
+// CompareHashedPassword returns nil when the given plaintext password matches the stored hash.
+func (c GUIConfiguration) CompareHashedPassword(password string) error {
+	configPasswordBytes := []byte(c.Password)
+	passwordBytes := []byte(password)
+	return bcrypt.CompareHashAndPassword(configPasswordBytes, passwordBytes)
+}
+
 // IsValidAPIKey returns true when the given API key is valid, including both
 // the value in config and any overrides
 func (c GUIConfiguration) IsValidAPIKey(apiKey string) bool {