From dec6f80d2bc9067347d80cba692da49c38cbdfde Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Colomb?= Date: Mon, 8 Nov 2021 13:32:04 +0100 Subject: [PATCH] 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 | 9 +++------ lib/api/api_auth_test.go | 13 +++++++------ lib/api/confighandler.go | 33 ++++++++++++++------------------- lib/config/config_test.go | 21 +++++++++++++++++++++ lib/config/guiconfiguration.go | 19 +++++++++++++++++++ 5 files changed, 64 insertions(+), 31 deletions(-) diff --git a/lib/api/api_auth.go b/lib/api/api_auth.go index d7d7d5a25..70add9a95 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 4735378a5..046b06a07 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 8f03acfdf..24e6b118c 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 e49aa9776..5da13207c 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 7b9830b30..147ed9fdc 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 {