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.

5 files changed, 64 insertions, 31 deletions

diff --git a/lib/api/api_auth.go b/lib/api/api_auth.go
index d7d7d5a250..70add9a954 100644
--- a/lib/api/api_auth.go
+++ b/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 {
diff --git a/lib/api/api_auth_test.go b/lib/api/api_auth_test.go
index 4735378a5b..046b06a077 100644
--- a/lib/api/api_auth_test.go
+++ b/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")
 	}
diff --git a/lib/api/confighandler.go b/lib/api/confighandler.go
index 8f03acfdff..24e6b118c1 100644
--- a/lib/api/confighandler.go
+++ b/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 {
diff --git a/lib/config/config_test.go b/lib/config/config_test.go
index e49aa97760..5da13207c9 100644
--- a/lib/config/config_test.go
+++ b/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
 
diff --git a/lib/config/guiconfiguration.go b/lib/config/guiconfiguration.go
index 7b9830b30a..147ed9fdc3 100644
--- a/lib/config/guiconfiguration.go
+++ b/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 {