diff options
author | André Colomb <src@andre.colomb.de> | 2021-11-08 13:32:04 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-11-08 13:32:04 +0100 |
commit | dec6f80d2bc9067347d80cba692da49c38cbdfde (patch) | |
tree | 2ba5c7ae99830ad27c3c6513f7e04124e13fbb1f | |
parent | ec8a748514e93f41e94478b59679afe67eb1d3a5 (diff) |
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.
-rw-r--r-- | lib/api/api_auth.go | 9 | ||||
-rw-r--r-- | lib/api/api_auth_test.go | 13 | ||||
-rw-r--r-- | lib/api/confighandler.go | 33 | ||||
-rw-r--r-- | lib/config/config_test.go | 21 | ||||
-rw-r--r-- | 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 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 { |