diff --git a/server/channels/app/authentication.go b/server/channels/app/authentication.go index e5c6cbe8b7a..edda80f5aac 100644 --- a/server/channels/app/authentication.go +++ b/server/channels/app/authentication.go @@ -12,6 +12,7 @@ import ( "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/public/shared/request" + "github.com/mattermost/mattermost/server/v8/channels/app/password/hashers" "github.com/mattermost/mattermost/server/v8/channels/app/users" "github.com/mattermost/mattermost/server/v8/channels/utils" "github.com/mattermost/mattermost/server/v8/platform/shared/mfa" @@ -61,6 +62,61 @@ func (a *App) IsPasswordValid(rctx request.CTX, password string) *model.AppError return nil } +func (a *App) checkUserPassword(user *model.User, password string, invalidateCache bool) *model.AppError { + if user.Password == "" || password == "" { + return model.NewAppError("checkUserPassword", "api.user.check_user_password.invalid.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized) + } + + // Get the hasher and parsed PHC + hasher, phc, err := hashers.GetHasherFromPHCString(user.Password) + if err != nil { + return model.NewAppError("checkUserPassword", "api.user.check_user_password.invalid_hash.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized) + } + + // Compare the password using the hasher that generated it + err = hasher.CompareHashAndPassword(phc, password) + if err != nil && errors.Is(err, hashers.ErrMismatchedHashAndPassword) { + // Increment the number of failed password attempts in case of + // mismatched hash and password + if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); passErr != nil { + return model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) + } + + if invalidateCache { + a.InvalidateCacheForUser(user.Id) + } + + return model.NewAppError("checkUserPassword", "api.user.check_user_password.invalid.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized).Wrap(err) + } else if err != nil { + return model.NewAppError("checkUserPassword", "app.valid_password_generic.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + + // Migrate the password if needed + if !hashers.IsLatestHasher(hasher) { + return a.migratePassword(user, password) + } + + return nil +} + +// migratePassword updates the database with the user's password hashed with the +// latest hashing method. It assumes that the password has been already validated. +func (a *App) migratePassword(user *model.User, password string) *model.AppError { + // Compute the new hash with the latest hashing method + newHash, err := hashers.Hash(password) + if err != nil { + return model.NewAppError("migratePassword", "app.user.check_user_password.failed_migration", nil, "", http.StatusInternalServerError).Wrap(err) + } + + // Update the password + if err := a.Srv().Store().User().UpdatePassword(user.Id, newHash); err != nil { + return model.NewAppError("migratePassword", "app.user.check_user_password.failed_update", nil, "", http.StatusInternalServerError).Wrap(err) + } + a.InvalidateCacheForUser(user.Id) + + return nil +} + func (a *App) CheckPasswordAndAllCriteria(rctx request.CTX, userID string, password string, mfaToken string) *model.AppError { // MM-37585 // Use locks to avoid concurrently checking AND updating the failed login attempts. @@ -81,18 +137,8 @@ func (a *App) CheckPasswordAndAllCriteria(rctx request.CTX, userID string, passw return err } - if err := users.CheckUserPassword(user, password); err != nil { - if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); passErr != nil { - return model.NewAppError("CheckPasswordAndAllCriteria", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) - } - - var invErr *users.ErrInvalidPassword - switch { - case errors.As(err, &invErr): - return model.NewAppError("checkUserPassword", "api.user.check_user_password.invalid.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized).Wrap(err) - default: - return model.NewAppError("checkUserPassword", "app.valid_password_generic.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } + if err := a.checkUserPassword(user, password, false); err != nil { + return err } if err := a.CheckUserMfa(rctx, user, mfaToken); err != nil { @@ -124,20 +170,8 @@ func (a *App) DoubleCheckPassword(rctx request.CTX, user *model.User, password s return err } - if err := users.CheckUserPassword(user, password); err != nil { - if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, user.FailedAttempts+1); passErr != nil { - return model.NewAppError("DoubleCheckPassword", "app.user.update_failed_pwd_attempts.app_error", nil, "", http.StatusInternalServerError).Wrap(passErr) - } - - a.InvalidateCacheForUser(user.Id) - - var invErr *users.ErrInvalidPassword - switch { - case errors.As(err, &invErr): - return model.NewAppError("DoubleCheckPassword", "api.user.check_user_password.invalid.app_error", nil, "user_id="+user.Id, http.StatusUnauthorized).Wrap(err) - default: - return model.NewAppError("DoubleCheckPassword", "app.valid_password_generic.app_error", nil, "", http.StatusInternalServerError).Wrap(err) - } + if err := a.checkUserPassword(user, password, true); err != nil { + return err } if passErr := a.Srv().Store().User().UpdateFailedPasswordAttempts(user.Id, 0); passErr != nil { diff --git a/server/channels/app/authentication_test.go b/server/channels/app/authentication_test.go index a0fb6fe5dad..06be2638b39 100644 --- a/server/channels/app/authentication_test.go +++ b/server/channels/app/authentication_test.go @@ -15,8 +15,10 @@ import ( "github.com/dgryski/dgoogauth" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/v8/channels/app/password/hashers" "github.com/mattermost/mattermost/server/v8/einterfaces/mocks" ) @@ -365,3 +367,179 @@ func TestCheckLdapUserPasswordConcurrency(t *testing.T) { } }) } + +func TestCheckUserPassword(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + pwd := "testPassword123$" + pwdBcryptBytes, err := bcrypt.GenerateFromPassword([]byte(pwd), 10) + require.NoError(t, err) + pwdBcrypt := string(pwdBcryptBytes) + pwdPBKDF2, err := hashers.Hash(pwd) + require.NoError(t, err) + + createUserWithHash := func(hash string) *model.User { + t.Helper() + + user := th.CreateUser() + + // Update the hash directly in the store (otherwise the app hashes it) + err := th.Server.Store().User().UpdatePassword(user.Id, hash) + require.NoError(t, err) + th.App.InvalidateCacheForUser(user.Id) + + updatedUser, appErr := th.App.GetUser(user.Id) + require.Nil(t, appErr) + require.Equal(t, hash, updatedUser.Password) + + return updatedUser + } + + t.Run("valid password with current hashing", func(t *testing.T) { + user := createUserWithHash(pwdPBKDF2) + err := th.App.checkUserPassword(user, pwd, false) + require.Nil(t, err) + }) + + t.Run("valid password with current hashing and cache invalidation", func(t *testing.T) { + user := createUserWithHash(pwdPBKDF2) + err := th.App.checkUserPassword(user, pwd, true) + require.Nil(t, err) + }) + + t.Run("invalid password", func(t *testing.T) { + user := createUserWithHash(pwdPBKDF2) + + err := th.App.checkUserPassword(user, "wrongpassword", false) + require.NotNil(t, err) + require.Equal(t, "api.user.check_user_password.invalid.app_error", err.Id) + + updatedUser, err := th.App.GetUser(user.Id) + require.Nil(t, err) + require.Equal(t, user.FailedAttempts+1, updatedUser.FailedAttempts) + }) + + t.Run("password migration from outdated hash", func(t *testing.T) { + user := createUserWithHash(pwdBcrypt) + require.Contains(t, user.Password, "$2a$10") + require.NotContains(t, user.Password, "pbkdf2") + + err := th.App.checkUserPassword(user, pwd, false) + require.Nil(t, err) + + updatedUser, err := th.App.GetUser(user.Id) + require.Nil(t, err) + require.NotEqual(t, pwdBcrypt, updatedUser.Password) + require.Contains(t, updatedUser.Password, "$pbkdf2") + + // Re-check with updated password + err = th.App.checkUserPassword(user, pwd, false) + require.Nil(t, err) + }) + + t.Run("password migration fails with invalid password", func(t *testing.T) { + user := createUserWithHash(pwdBcrypt) + + err := th.App.checkUserPassword(user, "wrongpassword", false) + require.NotNil(t, err) + require.Equal(t, "api.user.check_user_password.invalid.app_error", err.Id) + + updatedUser, err := th.App.GetUser(user.Id) + require.Nil(t, err) + require.Equal(t, user.FailedAttempts+1, updatedUser.FailedAttempts) + }) + + t.Run("empty password", func(t *testing.T) { + user := createUserWithHash(pwdPBKDF2) + + user, err := th.App.GetUser(user.Id) + require.Nil(t, err) + + err = th.App.checkUserPassword(user, "", false) + require.NotNil(t, err) + require.Equal(t, "api.user.check_user_password.invalid.app_error", err.Id) + }) + + t.Run("user with empty password hash", func(t *testing.T) { + user := createUserWithHash("") + + user, err := th.App.GetUser(user.Id) + require.Nil(t, err) + + err = th.App.checkUserPassword(user, pwd, false) + require.NotNil(t, err) + require.Equal(t, "api.user.check_user_password.invalid.app_error", err.Id) + }) + + t.Run("successful migration from PBKDF2 with old parameter to new parameter", func(t *testing.T) { + // Create a PBKDF2 hasher with work factor = 10000 instead of the default 60000 + oldParamPBKDF2, err := hashers.NewPBKDF2(10000, 32) + require.NoError(t, err) + + pwdOldParamPBKDF2, err := oldParamPBKDF2.Hash(pwd) + require.NoError(t, err) + + user := createUserWithHash(pwdOldParamPBKDF2) + require.Contains(t, user.Password, "$pbkdf2") + // The user hash contains the old parameter + require.Contains(t, user.Password, "w=10000") + + appErr := th.App.checkUserPassword(user, pwd, false) + require.Nil(t, appErr) + + updatedUser, appErr := th.App.GetUser(user.Id) + require.Nil(t, appErr) + require.NotEqual(t, pwdBcrypt, updatedUser.Password) + require.Contains(t, updatedUser.Password, "$pbkdf2") + // The new user hash contains the new parameter + require.Contains(t, updatedUser.Password, "w=60000") + + // Re-check with updated password + appErr = th.App.checkUserPassword(user, pwd, false) + require.Nil(t, appErr) + }) +} + +func TestMigratePassword(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + pwd := "testPassword123$" + pwdBcryptBytes, err := bcrypt.GenerateFromPassword([]byte(pwd), 10) + require.NoError(t, err) + pwdBcrypt := string(pwdBcryptBytes) + + createUserWithHash := func(hash string) *model.User { + t.Helper() + + user := th.CreateUser() + + // Update the hash directly in the store (otherwise the app hashes it) + err := th.Server.Store().User().UpdatePassword(user.Id, hash) + require.NoError(t, err) + th.App.InvalidateCacheForUser(user.Id) + + updatedUser, appErr := th.App.GetUser(user.Id) + require.Nil(t, appErr) + require.Equal(t, hash, updatedUser.Password) + + return updatedUser + } + + t.Run("successful migration from BCrypt to PBKDF2", func(t *testing.T) { + user := createUserWithHash(pwdBcrypt) + + err := th.App.migratePassword(user, pwd) + require.Nil(t, err) + + updatedUser, err := th.App.GetUser(user.Id) + require.Nil(t, err) + require.NotEqual(t, pwdBcrypt, updatedUser.Password) + require.Contains(t, updatedUser.Password, "$pbkdf2") + + // Re-check with updated password + err = th.App.checkUserPassword(user, pwd, false) + require.Nil(t, err) + }) +} diff --git a/server/channels/app/password/hashers/bcrypt.go b/server/channels/app/password/hashers/bcrypt.go new file mode 100644 index 00000000000..9fe6f8cd9dd --- /dev/null +++ b/server/channels/app/password/hashers/bcrypt.go @@ -0,0 +1,87 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package hashers + +import ( + "errors" + + "github.com/mattermost/mattermost/server/v8/channels/app/password/phcparser" + "golang.org/x/crypto/bcrypt" +) + +// BCrypt implements the [PasswordHasher] interface using the +// golang.org/x/crypto/bcrypt as the hashing method. +// +// This is the first hashing method used to hash passwords in the codebase, and so +// it predates the implementation of the PHC string format for passwords. This means +// that this hasher is *not* PHC-compliant, although its output kind of look like +// PHC: +// +// $2a$10$z0OlN1MpiLVlLTyE1xtEjOJ6/xV95RAwwIUaYKQBAqoeyvPgLEnUa +// +// The format is $xy$n$salthash, where: +// - $ is the literal '$' (1 byte) +// - x is the major version (1 byte) +// - y is the minor version (0 or 1 byte) +// - $ is the literal '$' (1 byte) +// - n is the cost (2 bytes) +// - $ is the literal '$' (1 byte) +// - salt is the encoded salt (22 bytes) +// - hash is the encoded hash (31 bytes) +// +// In total, 60 bytes (59 if there is no minor version) +// +// But this is not PHC-compliant: xy is not the function id, n is not +// a parameter name=value, nor the version, and there is no '$' +// separating the salt and the hash. +type BCrypt struct{} + +const ( + // BCryptCost is the value of the cost parameter used throughout the history + // of the codebase. + BCryptCost = 10 +) + +// NewBCrypt returns a new BCrypt instance +func NewBCrypt() BCrypt { + return BCrypt{} +} + +// Hash is a wrapper over golang.org/x/crypto/bcrypt.GenerateFromPassword, with +// two main differences: +// - If the password is too long, it returns [ErrPasswordTooLong] instead of +// bcrypt.ErrPasswordTooLong, in order to comply with the rest of the hashers +// in this package. +// - It returns a string instead of a byte slice, so that [BCrypt] implements +// the [PasswordHasher] interface. +func (b BCrypt) Hash(password string) (string, error) { + hash, err := bcrypt.GenerateFromPassword([]byte(password), BCryptCost) + if err != nil { + if errors.Is(err, bcrypt.ErrPasswordTooLong) { + return "", ErrPasswordTooLong + } + return "", err + } + + return string(hash), nil +} + +// CompareHashAndPassword is a wrapper over +// golang.org/x/crypto/bcrypt.CompareHashAndPassword, using the PHC's Hash field +// as the input for its first argument: this is why [BCrypt] is an edge case for +// a [PasswordHasher]: it only uses the [PHC.Hash] field, and ignores anything +// else in there. +func (b BCrypt) CompareHashAndPassword(hash phcparser.PHC, password string) error { + err := bcrypt.CompareHashAndPassword([]byte(hash.Hash), []byte(password)) + if errors.Is(err, bcrypt.ErrMismatchedHashAndPassword) { + return ErrMismatchedHashAndPassword + } + + return err +} + +// IsPHCValid returns always false: [BCrypt] is not PHC compliant +func (b BCrypt) IsPHCValid(hash phcparser.PHC) bool { + return false +} diff --git a/server/channels/app/password/hashers/bcrypt_test.go b/server/channels/app/password/hashers/bcrypt_test.go new file mode 100644 index 00000000000..5fdd17eb005 --- /dev/null +++ b/server/channels/app/password/hashers/bcrypt_test.go @@ -0,0 +1,88 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package hashers + +import ( + "strings" + "testing" + + "github.com/mattermost/mattermost/server/v8/channels/app/password/phcparser" + "github.com/stretchr/testify/require" +) + +func TestBCryptHash(t *testing.T) { + testCases := []struct { + testName string + pwd string + expectedErr error + }{ + // BCrypt.Hash is a very thing wrapper over crypto/bcrypt, so we only test + // the differences with that method + { + testName: "valid password", + pwd: "^a v3ery c0mp_ex Passw∙rd$", + expectedErr: nil, + }, + { + testName: "very long password", + pwd: strings.Repeat("verylong", 72), + expectedErr: ErrPasswordTooLong, + }, + } + + for _, tc := range testCases { + hasher := NewBCrypt() + + _, err := hasher.Hash(tc.pwd) + if tc.expectedErr != nil { + require.ErrorIs(t, err, tc.expectedErr) + } else { + require.NoError(t, err) + } + } +} + +func TestBCryptCompareHashAndPassword(t *testing.T) { + testCases := []struct { + testName string + storedPwd string + inputPwd string + expectedErr error + }{ + { + "empty password", + "", + "", + nil, + }, + { + "same password", + "one password", + "one password", + nil, + }, + { + "different password", + "one password", + "another password", + ErrMismatchedHashAndPassword, + }, + } + + hasher := NewBCrypt() + + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + hash, err := hasher.Hash(tc.storedPwd) + require.NoError(t, err) + + err = hasher.CompareHashAndPassword(phcparser.PHC{Hash: hash}, tc.inputPwd) + if tc.expectedErr != nil { + require.ErrorIs(t, err, tc.expectedErr) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/server/channels/app/password/hashers/hashers.go b/server/channels/app/password/hashers/hashers.go new file mode 100644 index 00000000000..1f07ce6507f --- /dev/null +++ b/server/channels/app/password/hashers/hashers.go @@ -0,0 +1,151 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +// Package hashers provides several implementations of password hashing functions. +// +// This package allows for seamless migrations of password hashing methods. To +// migrate to a new hasher in the future, the steps needed are: +// 1. Add a new type that implements the [PasswordHasher] interface. Let's call +// it `NewHasher`. +// 2. Update the [latestHasher] variable so that it points to a `NewHasher` +// instance: +// +// ``` diff +// var ( +// +// // latestHasher is the hasher currently in use. +// // Any password hashed with a different hasher must be migrated to this one. +// +// - latestHasher PasswordHasher = DefaultPBKDF2() +// + latestHasher PasswordHasher = DefaultNewHasher() +// ``` +// 3. Modify [GetHasherFromPHCString] to add a new case in the switch to +// identify the new function ID. +// +// If what is needed is to upgrade to a new set of parameters for the same +// hashing method (let's say keep using PBKDF2 but increase the work factor +// from 60,000 to 120,000), then no modification to [GetHasherFromPHCString] +// is needed. Simply update the [latestHasher] varible with the new parameter, +// and [IsPHCValid] will detect the difference in the parameter. +// +// Note that the migration happens in [App.migratePassword], which is triggered +// whenever the user enters their password and an old hashing method is +// identified when parsing their stored hashed password. +// This means that the older password hashers can *never* be removed, unless all +// users whose passwords are not migrated are either forced to re-login, or +// forced to generate a new password. +// +// Another important note is that once a server upgrades to a version with an +// updated hashing method, downgrading to any previous version will break the +// login flow for users whose passwords were migrated to the newer method. +package hashers + +import ( + "fmt" + "strings" + + "github.com/mattermost/mattermost/server/v8/channels/app/password/phcparser" +) + +// PasswordHasher is a password hasher compliant with the PHC string format: +// https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md +// +// Implementations of this interface need to make sure that all of the methods +// are thread-safe. +type PasswordHasher interface { + // Hash computes a hash of the provided password, returning a PHC string + // containing all the information that was needed to compute it: the function + // used to generate it, its version, parameters and salt, if needed. + // + // If an implementation of Hash needs a salt to generate the hash, it will + // create one, so that callers don't need to provide it. + Hash(password string) (string, error) + + // CompareHashAndPassword compares the parsed PHC and a provided password, + // returning an error if and only if the hashes do not match. + // + // Implementations need to make sure that the comparisons are done in constant + // time; e.g., using crypto/internal/fips140/subtle.ConstantTimeCompare. + CompareHashAndPassword(hash phcparser.PHC, password string) error + + // IsPHCValid validates whether a parsed PHC string conforms to the parameters + // of the hasher. + IsPHCValid(hash phcparser.PHC) bool +} + +const ( + // Maximum password length for all password hashers + PasswordMaxLengthBytes = 72 +) + +var ( + // latestHasher is the hasher currently in use. + // Any password hashed with a different hasher must be migrated to this one. + latestHasher PasswordHasher = DefaultPBKDF2() + + // ErrPasswordTooLong is the error returned when the provided password is + // longer than [PasswordMaxLengthBytes]. + ErrPasswordTooLong = fmt.Errorf("password too long; maximum length in bytes: %d", PasswordMaxLengthBytes) + + // ErrMismatchedHashAndPassword is the error returned when the provided + // password does not match the stored hash + ErrMismatchedHashAndPassword = fmt.Errorf("hash and password do not match") +) + +// getOriginalHasher returns a [BCrypt] hasher, the first hasher used in the +// codebase. +func getOriginalHasher(phcString string) (PasswordHasher, phcparser.PHC) { + // [BCrypt] is somewhat of an edge case, since it is not PHC-compliant, and + // needs the whole PHC string in its Hash field + return NewBCrypt(), phcparser.PHC{Hash: phcString} +} + +// GetHasherFromPHC returns the password hasher that was used to generate the +// provided PHC string. +// If the PHC string is not valid, or the function ID is unknown, this function +// defaults to the first hasher ever used, which did not properly implement PHC: +// the [BCrypt] hasher. +func GetHasherFromPHCString(phcString string) (PasswordHasher, phcparser.PHC, error) { + phc, err := phcparser.New(strings.NewReader(phcString)).Parse() + if err != nil { + // If the PHC string is invalid, return the original hasher, bcrypt + bcrypt, bcryptPhc := getOriginalHasher(phcString) + return bcrypt, bcryptPhc, nil + } + + // First check whether PHC conforms to the latest hasher + if latestHasher.IsPHCValid(phc) { + return latestHasher, phc, nil + } + + // If not, check the function ID and create a new one depending on it + switch phc.Id { + case PBKDF2FunctionId: + pbkdf2, err := NewPBKDF2FromPHC(phc) + if err != nil { + return PBKDF2{}, phcparser.PHC{}, fmt.Errorf("the provided PHC string is PBKDF2, but is not valid: %w", err) + } + return pbkdf2, phc, nil + // If the function ID is unknown, return the original hasher + default: + bcrypt, phc := getOriginalHasher(phcString) + return bcrypt, phc, nil + } +} + +// Hash hashes the provided password with the latest hashing method. +func Hash(password string) (string, error) { + return latestHasher.Hash(password) +} + +// CompareHashAndPassword compares the parsed [phcparser.PHC] and the provided +// password using the latest hashing method. +func CompareHashAndPassword(phc phcparser.PHC, password string) error { + return latestHasher.CompareHashAndPassword(phc, password) +} + +// IsLatestHasher verifies that the provided hasher is the latest one. This +// function is useful for identifying stored hashes that require a migration. +func IsLatestHasher(hasher PasswordHasher) bool { + return latestHasher == hasher +} diff --git a/server/channels/app/password/hashers/hashers_test.go b/server/channels/app/password/hashers/hashers_test.go new file mode 100644 index 00000000000..f2955c2669c --- /dev/null +++ b/server/channels/app/password/hashers/hashers_test.go @@ -0,0 +1,138 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package hashers + +import ( + "testing" + + "github.com/mattermost/mattermost/server/v8/channels/app/password/phcparser" + "github.com/stretchr/testify/require" +) + +func TestGetHasherFromPHCString(t *testing.T) { + testCases := []struct { + testName string + input string + expectedHasher PasswordHasher + expectedPHC phcparser.PHC + expectedErr bool + }{ + { + testName: "latest hasher (PBKDF2)", + input: "$pbkdf2$f=SHA256,w=600000,l=32$5Zq8TvET7nMrXof49Rp4Sw$d0Mx8467kv+3ylbGrkyu4jTd8O8SP51k4s1RuWb9S/o", + expectedHasher: latestHasher, + expectedPHC: phcparser.PHC{ + Id: "pbkdf2", + Version: "", + Params: map[string]string{ + "f": "SHA256", + "w": "600000", + "l": "32", + }, + Salt: "5Zq8TvET7nMrXof49Rp4Sw", + Hash: "d0Mx8467kv+3ylbGrkyu4jTd8O8SP51k4s1RuWb9S/o", + }, + expectedErr: false, + }, + { + testName: "valid, non-default PBKDF2", + input: "$pbkdf2$f=SHA256,w=10000,l=10$5Zq8TvET7nMrXof49Rp4Sw$d0Mx8467kv+3ylbGrkyu4jTd8O8SP51k4s1RuWb9S/o", + expectedHasher: PBKDF2{ + workFactor: 10000, + keyLength: 10, + phcHeader: "$pbkdf2$f=SHA256,w=10000,l=10$", + }, + expectedPHC: phcparser.PHC{ + Id: "pbkdf2", + Version: "", + Params: map[string]string{ + "f": "SHA256", + "w": "10000", + "l": "10", + }, + Salt: "5Zq8TvET7nMrXof49Rp4Sw", + Hash: "d0Mx8467kv+3ylbGrkyu4jTd8O8SP51k4s1RuWb9S/o", + }, + expectedErr: false, + }, + { + testName: "valid bcrypt", + input: "$2a$10$z0OlN1MpiLVlLTyE1xtEjOJ6/xV95RAwwIUaYKQBAqoeyvPgLEnUa", + expectedHasher: NewBCrypt(), + expectedPHC: phcparser.PHC{ + Hash: "$2a$10$z0OlN1MpiLVlLTyE1xtEjOJ6/xV95RAwwIUaYKQBAqoeyvPgLEnUa", + }, + expectedErr: false, + }, + { + testName: "invalid phc - default to bcrypt", + input: "invalid", + expectedHasher: NewBCrypt(), + expectedPHC: phcparser.PHC{ + Hash: "invalid", + }, + expectedErr: false, + }, + { + testName: "valid PBKDF2 with invalid parameters", + input: "$pbkdf2$f=SHA256,w=-50,l=0$5Zq8TvET7nMrXof49Rp4Sw$d0Mx8467kv+3ylbGrkyu4jTd8O8SP51k4s1RuWb9S/o", + expectedHasher: PBKDF2{ + workFactor: 10000, + keyLength: 10, + phcHeader: "$pbkdf2$f=SHA256,w=10000,l=10$", + }, + expectedPHC: phcparser.PHC{}, + expectedErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + actualHasher, actualPHC, err := GetHasherFromPHCString(tc.input) + if tc.expectedErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.expectedHasher, actualHasher) + require.Equal(t, tc.expectedPHC, actualPHC) + }) + } +} + +func TestIsLatestHasher(t *testing.T) { + pbkdf2WithOtherParams, err := NewPBKDF2(10000, 16) + require.NoError(t, err) + + testCases := []struct { + testName string + inputHasher PasswordHasher + expectedOutput bool + }{ + { + "latestHasher is the latest hasher", + latestHasher, + true, + }, + { + "DefaultPBKDF2 is the latest hasher", + DefaultPBKDF2(), + true, + }, + { + "PBKDF2 with other parameters is not the latest hasher", + pbkdf2WithOtherParams, + false, + }, + { + "bcrypt is not the latest hasher", + NewBCrypt(), + false, + }, + } + for _, tc := range testCases { + actualOutput := IsLatestHasher(tc.inputHasher) + require.Equal(t, tc.expectedOutput, actualOutput) + } +} diff --git a/server/channels/app/password/hashers/pbkdf2.go b/server/channels/app/password/hashers/pbkdf2.go new file mode 100644 index 00000000000..b20637e9464 --- /dev/null +++ b/server/channels/app/password/hashers/pbkdf2.go @@ -0,0 +1,232 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package hashers + +import ( + "crypto/pbkdf2" + "crypto/rand" + "crypto/sha256" + "crypto/subtle" + "encoding/base64" + "fmt" + "io" + "strconv" + "strings" + + "github.com/mattermost/mattermost/server/v8/channels/app/password/phcparser" +) + +const ( + // PBKDF2FunctionId is the name of the PBKDF2 hasher. + PBKDF2FunctionId string = "pbkdf2" +) + +const ( + // Default parameter values + defaultPRFName = "SHA256" + defaultWorkFactor = 600000 + defaultKeyLength = 32 + + // Length of the salt, in bytes + saltLenBytes = 16 +) + +var ( + defaultPRF = sha256.New +) + +// PBKDF2 implements the [PasswordHasher] interface using [crypto/pbkdf2] as the +// hashing method. +// +// It is parametrized by: +// - The work factor: the number of iterations performed during hashing. The +// larger this number, the longer and more costly the hashing process. OWASP +// has some recommendations on what number to use here: +// https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pbkdf2 +// - The key length: the desired length, in bytes, of the resulting hash. +// +// The internal hashing function is always set to SHA256. +// +// Its PHC string is of the form: +// +// $pbkdf2$f=,w=,l=$$ +// +// Where: +// - is a string specifying the internal hashing function (defaults to SHA256). +// - is an integer specifying the work factor (defaults to 600000). +// - is an integer specifying the key length (defaults to 32). +// - is the base64-encoded salt. +// - is the base64-encoded hash. +type PBKDF2 struct { + workFactor int + keyLength int + + phcHeader string +} + +// DefaultPBKDF2 returns a [PBKDF2] already initialized with the following +// parameters: +// - Internal hashing function: SHA256 +// - Work factor: 600,000 +// - Key length: 32 bytes +func DefaultPBKDF2() PBKDF2 { + hasher, err := NewPBKDF2(defaultWorkFactor, defaultKeyLength) + if err != nil { + panic("DefaultPBKDF2 implementation is incorrect") + } + return hasher +} + +// NewPBKDF2 returns a [PBKDF2] initialized with the provided parameters +func NewPBKDF2(workFactor int, keyLength int) (PBKDF2, error) { + if workFactor <= 0 { + return PBKDF2{}, fmt.Errorf("work factor must be strictly positive") + } + + if keyLength <= 0 { + return PBKDF2{}, fmt.Errorf("key length must be strictly positive") + } + // Precompute and store the PHC header, since it is common to every hashed + // password; it will be something like: + // $pbkdf2$f=SHA256,w=600000,l=32$ + phcHeader := new(strings.Builder) + + // First, the function ID + phcHeader.WriteRune('$') + phcHeader.WriteString(PBKDF2FunctionId) + + // Then, the parameters + phcHeader.WriteString("$f=") + phcHeader.WriteString(defaultPRFName) + phcHeader.WriteString(",w=") + phcHeader.WriteString(strconv.Itoa(workFactor)) + phcHeader.WriteString(",l=") + phcHeader.WriteString(strconv.Itoa(keyLength)) + + // Finish with the '$' that will mark the start of the salt + phcHeader.WriteRune('$') + + return PBKDF2{ + workFactor: workFactor, + keyLength: keyLength, + phcHeader: phcHeader.String(), + }, nil +} + +// NewPBKDF2FromPHC returns a [PBKDF2] that conforms to the provided parsed PHC, +// using the same parameters (if valid) present there. +func NewPBKDF2FromPHC(phc phcparser.PHC) (PBKDF2, error) { + workFactor, err := strconv.Atoi(phc.Params["w"]) + if err != nil { + return PBKDF2{}, fmt.Errorf("invalid work factor parameter 'w=%s'", phc.Params["w"]) + } + + keyLength, err := strconv.Atoi(phc.Params["l"]) + if err != nil { + return PBKDF2{}, fmt.Errorf("invalid key length parameter 'l=%s'", phc.Params["l"]) + } + + return NewPBKDF2(workFactor, keyLength) +} + +// hashWithSalt calls crypto/pbkdf2.Key with the provided salt and the stored +// parameters. +func (p PBKDF2) hashWithSalt(password string, salt []byte) (string, error) { + hash, err := pbkdf2.Key(defaultPRF, password, salt, p.workFactor, p.keyLength) + if err != nil { + return "", fmt.Errorf("failed hashing the password: %w", err) + } + + encodedHash := base64.RawStdEncoding.EncodeToString(hash) + return encodedHash, nil +} + +// Hash hashes the provided password using the PBKDF2 algorithm with the stored +// parameters, returning a PHC-compliant string. +// +// The salt is generated randomly and stored in the returned PHC string. If the +// provided password is longer than [PasswordMaxLengthBytes], [ErrPasswordTooLong] +// is returned. +func (p PBKDF2) Hash(password string) (string, error) { + // Enforce a maximum length, even if PBKDF2 can theoretically accept *any* length + if len(password) > PasswordMaxLengthBytes { + return "", ErrPasswordTooLong + } + + // Create random salt + salt := make([]byte, saltLenBytes) + if _, err := io.ReadFull(rand.Reader, salt); err != nil { + return "", fmt.Errorf("unable to generate salt for user: %w", err) + } + + // Compute hash + hash, err := p.hashWithSalt(password, salt) + if err != nil { + return "", fmt.Errorf("failed to hash the password: %w", err) + } + + // Initialize string builder and base64 encoder + phcString := new(strings.Builder) + b64Encoder := base64.RawStdEncoding + + // Now, start writing: first, the stored header: function ID and parameters + phcString.WriteString(p.phcHeader) + + // Next, the encoded salt (the header already contains the initial $, so we + // can skip it) + // If we were to use a real encoder using an io.Writer, we would need to + // call Close after the salt, otherwise the last block doesn't get written; + // but we don't want to close it yet, because we want to write the hash later; + // so I think it's not worth using an encoder, and it's better to call + // EncodeToString directly, here and when writing the hash + phcString.WriteString(b64Encoder.EncodeToString(salt)) + + // Finally, the encoded hash + phcString.WriteRune('$') + phcString.WriteString(hash) + + return phcString.String(), nil +} + +// CompareHashAndPassword compares the provided [phcparser.PHC] with the plain-text +// password. +// +// The provided [phcparser.PHC] is validated to double-check it was generated with +// this hasher and parameters. +func (p PBKDF2) CompareHashAndPassword(hash phcparser.PHC, password string) error { + // Validate parameters + if !p.IsPHCValid(hash) { + return fmt.Errorf("the stored password does not comply with the PBKDF2 parser's PHC serialization") + } + + salt, err := base64.RawStdEncoding.DecodeString(hash.Salt) + if err != nil { + return fmt.Errorf("failed decoding hash's salt: %w", err) + } + + // Hash the new password with the stored hash's salt + newHash, err := p.hashWithSalt(password, salt) + if err != nil { + return fmt.Errorf("failed to hash the password: %w", err) + } + + // Compare both hashes + if subtle.ConstantTimeCompare([]byte(hash.Hash), []byte(newHash)) != 1 { + return ErrMismatchedHashAndPassword + } + + return nil +} + +// IsPHCValid validates that the provided [phcparser.PHC] is valid, meaning: +// - The function used to generate it was [PBKDF2FunctionId]. +// - The parameters used to generate it were the same as the ones used to +// create this hasher. +func (p PBKDF2) IsPHCValid(phc phcparser.PHC) bool { + return phc.Id == PBKDF2FunctionId && + len(phc.Params) == 3 && + phc.Params["f"] == defaultPRFName && + phc.Params["w"] == strconv.Itoa(p.workFactor) && + phc.Params["l"] == strconv.Itoa(p.keyLength) +} diff --git a/server/channels/app/password/hashers/pbkdf2_test.go b/server/channels/app/password/hashers/pbkdf2_test.go new file mode 100644 index 00000000000..50717631c24 --- /dev/null +++ b/server/channels/app/password/hashers/pbkdf2_test.go @@ -0,0 +1,94 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package hashers + +import ( + "crypto/pbkdf2" + "crypto/sha256" + "encoding/base64" + "strings" + "testing" + + "github.com/mattermost/mattermost/server/v8/channels/app/password/phcparser" + "github.com/stretchr/testify/require" +) + +func TestPBKDF2Hash(t *testing.T) { + password := "^a v3ery c0mp_ex Passw∙rd$" + workFactor := 600000 + keyLength := 32 + + hasher, err := NewPBKDF2(workFactor, keyLength) + require.NoError(t, err) + + str, err := hasher.Hash(password) + require.NoError(t, err) + + phc, err := phcparser.New(strings.NewReader(str)).Parse() + require.NoError(t, err) + require.Equal(t, "pbkdf2", phc.Id) + require.Equal(t, "", phc.Version) + require.Equal(t, map[string]string{ + "f": "SHA256", + "w": "600000", + "l": "32", + }, phc.Params) + + salt, err := base64.RawStdEncoding.DecodeString(phc.Salt) + require.NoError(t, err) + + hash, err := pbkdf2.Key(sha256.New, password, salt, workFactor, keyLength) + require.NoError(t, err) + + expectedHash := base64.RawStdEncoding.EncodeToString(hash) + require.Equal(t, expectedHash, phc.Hash) +} + +func TestPBKDF2CompareHashAndPassword(t *testing.T) { + testCases := []struct { + testName string + storedPwd string + inputPwd string + expectedErr error + }{ + { + + "empty password", + "", + "", + nil, + }, + { + "same password", + "one password", + "one password", + nil, + }, + { + "different password", + "one password", + "another password", + ErrMismatchedHashAndPassword, + }, + } + + hasher := DefaultPBKDF2() + + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + storedPHCStr, err := hasher.Hash(tc.storedPwd) + require.NoError(t, err) + + storedPHC, err := phcparser.New(strings.NewReader(storedPHCStr)).Parse() + require.NoError(t, err) + + err = hasher.CompareHashAndPassword(storedPHC, tc.inputPwd) + if tc.expectedErr != nil { + require.ErrorIs(t, err, tc.expectedErr) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/server/channels/app/password/phcparser/parser.go b/server/channels/app/password/phcparser/parser.go new file mode 100644 index 00000000000..263a37d5a5b --- /dev/null +++ b/server/channels/app/password/phcparser/parser.go @@ -0,0 +1,434 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +// Package parser provides a type to parse strings conformant to the PHC string format: +// https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md +package phcparser + +import ( + "bufio" + "bytes" + "fmt" + "io" +) + +// PHC represents a PHC string, with all its parts already parsed: +type PHC struct { + // Id is the identifier of the hashing function. + Id string + + // Version is an optional string containing the specific version of the + // hashing function used. + Version string + + // Params is a map of parameters, containing a set of all parameter names + // with their corresponding values. + Params map[string]string + + // Salt is the base64-encoded salt used when hashing the original password. + Salt string + + // Hash is the base64-encoded hash generated when hashing the original + // password with the function specified by all other parameters. + Hash string +} + +// Parser is a wrapper of a limited bufio.Reader that will parse its input into +// a [PHC]. +type Parser struct { + reader *bufio.Reader +} + +// MaxRunes is the maximum number of runes allowed in a PHC string. If the +// string is longer, the remaining runes are ignored. +const MaxRunes = 256 + +// New builds a new [Parser], limiting the input to [MaxRunes] runes. +func New(r io.Reader) *Parser { + return &Parser{reader: bufio.NewReader(io.LimitReader(r, MaxRunes))} +} + +// Token represents a minimal unit of meaning in the parsed string. +type Token uint + +const ( + // ILLEGAL is a token representing an illegal token + ILLEGAL Token = 1 << iota + + // Separator tokens + // EOF is a token representing the end of the input + EOF + // DOLLARSIGN is a token representing a '$' + DOLLARSIGN + // COMMA is a token representing a ',' + COMMA + // EQUALSIGN is a token representing a '=' + EQUALSIGN + + // Literals + // FUNCTIONID is a token representing a non-empty set of any of the following symbols: + // [a-z0-9-] + FUNCTIONID + // PARAMNAME is a token representing a non-empty set of any of the following symbols: + // [a-z0-9-] + PARAMNAME + // PARAMVALUE is a token representing a non-empty set of any of the following symbols: + // [a-zA-Z0-9/+.-] + PARAMVALUE + // B64ENCODED is a token representing a non-empty set of any of the following symbols: + // [A-Za-z0-9+/] + B64ENCODED +) + +const ( + // IDENT is a generic identifier that represents any of its possibilities: + // either a FUNCTIONID, a PARAMNAME, a PARAMVALUE or a B64ENCODED + IDENT Token = FUNCTIONID | PARAMNAME | PARAMVALUE | B64ENCODED +) + +// eof is a constant literal representing EOF +const eof = rune(0) + +// [a-z] +func isLowercaseLetter(ch rune) bool { + return (ch >= 'a' && ch <= 'z') +} + +// [A-Za-z] +func isLetter(ch rune) bool { + return (ch >= 'A' && ch <= 'Z') || (ch >= 'a' && ch <= 'z') +} + +// [0-9] +func isDigit(ch rune) bool { + return (ch >= '0' && ch <= '9') +} + +// [A-Za-z0-9+/] +func isB64(ch rune) bool { + return isLetter(ch) || isDigit(ch) || ch == '+' || ch == '/' +} + +// [/+.-] +func isSymbol(ch rune) bool { + return ch == '/' || ch == '+' || ch == '.' || ch == '-' +} + +// [a-z0-9-] +func isLowercaseLetterOrDigitOrMinus(ch rune) bool { + return isLowercaseLetter(ch) || isDigit(ch) || ch == '-' +} + +// [a-zA-Z0-9/+.-] +func isLetterOrDigitOrSymbol(ch rune) bool { + return isLetter(ch) || isDigit(ch) || isSymbol(ch) +} + +// no identifiers allowed +func none(ch rune) bool { + return false +} + +// read reads a single rune, returning [eof] in case of any error. +func (p *Parser) read() rune { + ch, _, err := p.reader.ReadRune() + if err != nil { + return eof + } + return ch +} + +// unread unreads a single rune +func (p *Parser) unread() { _ = p.reader.UnreadRune() } + +// scan scans either an identifier whose runes are allowed by the provided +// function, or a single separator token: EOF $ , = +func (p *Parser) scan(isIdentAllowedRune func(rune) bool) (tok Token, lit string) { + ch := p.read() + + if isIdentAllowedRune(ch) { + p.unread() + return p.scanIdent(isIdentAllowedRune) + } + + switch ch { + case eof: + return EOF, "" + case '$': + return DOLLARSIGN, string(ch) + case ',': + return COMMA, string(ch) + case '=': + return EQUALSIGN, string(ch) + } + + return ILLEGAL, string(ch) +} + +// scanIdent scans a series of contiguous runes allowed by the provided function +// that form a single identifier. +func (p *Parser) scanIdent(isIdentAllowedRune func(rune) bool) (tok Token, lit string) { + var buf bytes.Buffer + buf.WriteRune(p.read()) + + for { + ch := p.read() + if ch == eof { + break + } + + if !isIdentAllowedRune(ch) { + p.unread() + break + } + + _, _ = buf.WriteRune(ch) + } + + // On success, return the generic IDENT, the check for each specific + // identifier is already done with isIdentAllowedRune + return IDENT, buf.String() +} + +// scanSeparator scans one of the separator tokens: +// - EOF +// - $ +// - , +// - = +func (p *Parser) scanSeparator() (tok Token, lit string) { + return p.scan(none) +} + +// parseToken returns the literal string of an expected token, or an error. +// expected can be an ORed expression of different tokens, like +// +// EOF | DOLLARSIGN | FUNCTIONID +// +// In this case, any of those tokens are allowd, and its literal will be returned. +func (p *Parser) parseToken(expected Token) (string, error) { + var allowedRuneFunc func(rune) bool + switch expected { + case FUNCTIONID, PARAMNAME: + allowedRuneFunc = isLowercaseLetterOrDigitOrMinus + case PARAMVALUE: + allowedRuneFunc = isLetterOrDigitOrSymbol + case B64ENCODED: + allowedRuneFunc = isB64 + default: + allowedRuneFunc = none + } + + token, literal := p.scan(allowedRuneFunc) + if token&expected == 0 { + return "", fmt.Errorf("found %q, expected '$'", literal) + } + + return literal, nil +} + +// parseFunctionId parses a function ID +func (p *Parser) parseFunctionId() (string, error) { + literal, err := p.parseToken(DOLLARSIGN) + if err != nil { + return literal, fmt.Errorf("found %q, expected '$'", literal) + } + + literal, err = p.parseToken(FUNCTIONID) + if err != nil { + return literal, fmt.Errorf("found %q, expected a function identifier", literal) + } + return literal, nil +} + +// parseHash parses a base64-encoded hash +func (p *Parser) parseHash() (string, error) { + // We parse the hash + hash, err := p.parseToken(B64ENCODED) + if err != nil { + return "", fmt.Errorf("found %q, expected the hash", hash) + } + + // and make sure that the string finishes right after it + literal, err := p.parseToken(EOF) + if err != nil { + return "", fmt.Errorf("found %q, expected EOF", literal) + } + + return hash, nil +} + +// parseParamsRHS parses an equal sign followed by a parameter value, returning +// only the parameter value. +func (p *Parser) parseParamRHS() (string, error) { + if literal, err := p.parseToken(EQUALSIGN); err != nil { + return literal, err + } + + return p.parseToken(PARAMVALUE) +} + +// Parse parses the [Parser]'s reader into a [PHC]. +// +// This function will return an error along with an empty [PHC] when the provided +// input is not PHC-compliant. +func (p *Parser) Parse() (PHC, error) { + // Initialize the returned PHC and its inner parameters map + out := PHC{} + out.Params = make(map[string]string) + + // Start parsing: first, we expect '$functionId' + id, err := p.parseFunctionId() + if err != nil { + return PHC{}, fmt.Errorf("failed to parse function ID: %w", err) + } + out.Id = id + + // Now we expect either EOF, or to continue parsing with a '$' + switch token, literal := p.scanSeparator(); token { + case EOF: + // Just a function identifier is valid, according to the spec + return out, nil + case DOLLARSIGN: + // We continue parsing + break + default: + return PHC{}, fmt.Errorf("found %q, expected '$' or EOF", literal) + } + + // There was a '$', so we expect now another identifier, which can either be: + // - The version key, "v", + // - A parameter name + // - The salt + // B64ENCODED is a superset of PARAMNAME (which is also a superset of "v"), + // sso we allow the former because we don't know yet what we're parsing. + versionKeyOrParamNameOrSalt, err := p.parseToken(B64ENCODED) + if err != nil { + return PHC{}, fmt.Errorf("found %q, expected the version key, 'v', a parameter name or the salt: %w", versionKeyOrParamNameOrSalt, err) + } + + // If it's the version key, then we know now that we are parsing + // '$v=versionStr', and we expect now '=versionStr' + if versionKeyOrParamNameOrSalt == "v" { + versionStr, err := p.parseParamRHS() + if err != nil { + return PHC{}, fmt.Errorf("failed parsing version string: %w", err) + } + out.Version = versionStr + + // Now we expect either EOF, or to continue parsing with a '$' + switch token, literal := p.scanSeparator(); token { + case EOF: + // Just a function identifier + version is valid, according to the spec + return out, nil + case DOLLARSIGN: + // We continue parsing + break + default: + return PHC{}, fmt.Errorf("found %q, expected '$' or EOF", literal) + } + + // Read the next ident into the variable we had before, so we can continue + // the logic regardless of whether this block was executed or not. + versionKeyOrParamNameOrSalt, err = p.parseToken(B64ENCODED) + if err != nil { + return PHC{}, fmt.Errorf("found %q, expected a parameter name or the version key, 'v'", versionKeyOrParamNameOrSalt) + } + } + + // Now, we either didn't have a version key, or we have already parsed it, + // so we are left with either a parameter name or the salt. + paramNameOrSalt := versionKeyOrParamNameOrSalt + + // We know which one by scaning the next token: + switch token, literal := p.scanSeparator(); token { + // If the following token is '=', then it was a parameter name, and we + // expect now '=value' + case EQUALSIGN: + paramName := paramNameOrSalt + // Additional validation for the parameter name not to have the invalid + // value "v" + if paramName == "v" { + return PHC{}, fmt.Errorf("found 'v' as a parameter name, which is only allowed as the version key") + } + // Now we parse '=value' + paramValue, err := p.parseToken(PARAMVALUE) + if err != nil { + return PHC{}, fmt.Errorf("found %q, expected a value for parameter %q", paramValue, paramName) + } + + // And we store the parameter + out.Params[paramName] = paramValue + + // If the following token is '$' or EOF, then it was the salt, so we store it, + // and optionally parse the hash + case DOLLARSIGN, EOF: + salt := paramNameOrSalt + out.Salt = salt + + // If the token was '$', then now we expect a hash + if token == DOLLARSIGN { + hash, err := p.parseHash() + if err != nil { + return PHC{}, err + } + out.Hash = hash + } + + return out, nil + // Otherwise, we have an error + default: + return PHC{}, fmt.Errorf("found %q, expected either '$', or '=' or EOF", literal) + } + + // If we are here, it means that we just parsed a parameter value, so now we + // have three possibilities (in a loop): + // - If we see EOF, then we're done! + // - If we see a comma, then we expect another name=value pair, and we + // restart the loop + // - If we see '$', then we need to parse 'salt[$hash]', and we finish + for { + switch token, literal := p.scanSeparator(); token { + // We're done! + case EOF: + return out, nil + // Parse a name=value pair, and continue the loop + case COMMA: + paramName, err := p.parseToken(PARAMNAME) + if err != nil { + return PHC{}, err + } + + paramValue, err := p.parseParamRHS() + if err != nil { + return PHC{}, fmt.Errorf("failed parsing value from parameter %q: %w", paramName, err) + } + out.Params[paramName] = paramValue + // Parse a salt and an optional hash, and finish + case DOLLARSIGN: + salt, err := p.parseToken(B64ENCODED) + if err != nil { + return PHC{}, err + } + out.Salt = salt + + switch token, newLiteral := p.scanSeparator(); token { + // If what we parsed was a $, then now we expect a $hash + case DOLLARSIGN: + hash, err := p.parseHash() + if err != nil { + return PHC{}, err + } + out.Hash = hash + return out, nil + // If what we parsed was an EOF, then we return successfully + case EOF: + return out, nil + // Otherwise, we have an error + default: + return PHC{}, fmt.Errorf("found %q, expected either '$', or EOF", newLiteral) + } + default: + return PHC{}, fmt.Errorf("found %q, expected either ',', '$' or EOF", literal) + } + } +} diff --git a/server/channels/app/password/phcparser/parser_test.go b/server/channels/app/password/phcparser/parser_test.go new file mode 100644 index 00000000000..ee465739be7 --- /dev/null +++ b/server/channels/app/password/phcparser/parser_test.go @@ -0,0 +1,584 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package phcparser + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestParse(t *testing.T) { + testCases := []struct { + input string + output PHC + expectErr bool + }{ + ///////////////////////////////////////////////////////////////// + // Valid strings + { + "$argon2i$m=120,t=4294967295,p=2", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "4294967295", + "p": "2", + }, + Salt: "", + Hash: "", + }, + false, + }, + { + "$argon2i$m=2040,t=5000,p=255", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "2040", + "t": "5000", + "p": "255", + }, + Salt: "", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0", + }, + Salt: "", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0ZQ", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0ZQ", + }, + Salt: "", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0ZQA", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0ZQA", + }, + Salt: "", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,data=sRlHhRmKUGzdOmXn01XmXygd5Kc", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "data": "sRlHhRmKUGzdOmXn01XmXygd5Kc", + }, + Salt: "", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0,data=sRlHhRmKUGzdOmXn01XmXygd5Kc", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0", + "data": "sRlHhRmKUGzdOmXn01XmXygd5Kc", + }, + Salt: "", + Hash: "", + }, + false, + }, + + { + "$argon2i$m=120,t=5000,p=2$4fXXG0spB92WPB1NitT8/OH0VKI", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + }, + Salt: "4fXXG0spB92WPB1NitT8/OH0VKI", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2$BwUgJHHQaynE+a4nZrYRzOllGSjjxuxNXxyNRUtI6Dlw/zlbt6PzOL8Onfqs6TcG", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + }, + Salt: "BwUgJHHQaynE+a4nZrYRzOllGSjjxuxNXxyNRUtI6Dlw/zlbt6PzOL8Onfqs6TcG", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0$4fXXG0spB92WPB1NitT8/OH0VKI", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0", + }, + Salt: "4fXXG0spB92WPB1NitT8/OH0VKI", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,data=sRlHhRmKUGzdOmXn01XmXygd5Kc$4fXXG0spB92WPB1NitT8/OH0VKI", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "data": "sRlHhRmKUGzdOmXn01XmXygd5Kc", + }, + Salt: "4fXXG0spB92WPB1NitT8/OH0VKI", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0,data=sRlHhRmKUGzdOmXn01XmXygd5Kc$4fXXG0spB92WPB1NitT8/OH0VKI", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0", + "data": "sRlHhRmKUGzdOmXn01XmXygd5Kc", + }, + Salt: "4fXXG0spB92WPB1NitT8/OH0VKI", + Hash: "", + }, + false, + }, + + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0$4fXXG0spB92WPB1NitT8/OH0VKI$iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0", + }, + Salt: "4fXXG0spB92WPB1NitT8/OH0VKI", + Hash: "iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,data=sRlHhRmKUGzdOmXn01XmXygd5Kc$4fXXG0spB92WPB1NitT8/OH0VKI$iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "data": "sRlHhRmKUGzdOmXn01XmXygd5Kc", + }, + Salt: "4fXXG0spB92WPB1NitT8/OH0VKI", + Hash: "iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0,data=sRlHhRmKUGzdOmXn01XmXygd5Kc$4fXXG0spB92WPB1NitT8/OH0VKI$iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0", + "data": "sRlHhRmKUGzdOmXn01XmXygd5Kc", + }, + Salt: "4fXXG0spB92WPB1NitT8/OH0VKI", + Hash: "iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0,data=sRlHhRmKUGzdOmXn01XmXygd5Kc$iHSDPHzUhPzK7rCcJgOFfg$EkCWX6pSTqWruiR0", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0", + "data": "sRlHhRmKUGzdOmXn01XmXygd5Kc", + }, + Salt: "iHSDPHzUhPzK7rCcJgOFfg", + Hash: "EkCWX6pSTqWruiR0", + }, + false, + }, + { + "$argon2i", + PHC{ + Id: "argon2i", + Params: map[string]string{}, + }, + false, + }, + { + "$argon2i$m=120", + PHC{ + Id: "argon2i", + Params: map[string]string{ + "m": "120", + }, + }, + false, + }, + { + "$argon2i$v=120", + PHC{ + Id: "argon2i", + Version: "120", + Params: map[string]string{}, + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2", + PHC{ + Id: "argon2i", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + }, + }, + false, + }, + { + "$argon2i$v=5$m=120,t=5000,p=2", + PHC{ + Id: "argon2i", + Version: "5", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + }, + }, + false, + }, + { + "$argon2i$/LtFjH5rVL8", + PHC{ + Id: "argon2i", + Params: map[string]string{}, + Salt: "/LtFjH5rVL8", + Hash: "", + }, + false, + }, + { + "$argon2i$/LtFjH5rVL8$iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + PHC{ + Id: "argon2i", + Params: map[string]string{}, + Salt: "/LtFjH5rVL8", + Hash: "iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + }, + false, + }, + { + "$argon2i$v=v2$/LtFjH5rVL8", + PHC{ + Id: "argon2i", + Version: "v2", + Params: map[string]string{}, + Salt: "/LtFjH5rVL8", + Hash: "", + }, + false, + }, + { + "$argon2i$v=v2$/LtFjH5rVL8$iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + PHC{ + Id: "argon2i", + Version: "v2", + Params: map[string]string{}, + Salt: "/LtFjH5rVL8", + Hash: "iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2$/LtFjH5rVL8", + PHC{ + Id: "argon2i", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + }, + Salt: "/LtFjH5rVL8", + Hash: "", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2$4fXXG0spB92WPB1NitT8/OH0VKI$iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + PHC{ + Id: "argon2i", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + }, + Salt: "4fXXG0spB92WPB1NitT8/OH0VKI", + Hash: "iPBVuORECm5biUsjq33hn9/7BKqy9aPWKhFfK2haEsM", + }, + false, + }, + { + "$argon2i$m=120,t=5000,p=2,keyid=Hj5+dsK0,data=sRlHhRmKUGzdOmXn01XmXygd5Kc$iHSDPHzUhPzK7rCcJgOFfg$J4moa2MM0/6uf3HbY2Tf5Fux8JIBTwIhmhxGRbsY14qhTltQt+Vw3b7tcJNEbk8ium8AQfZeD4tabCnNqfkD1g", + PHC{ + Id: "argon2i", + Version: "", + Params: map[string]string{ + "m": "120", + "t": "5000", + "p": "2", + "keyid": "Hj5+dsK0", + "data": "sRlHhRmKUGzdOmXn01XmXygd5Kc", + }, + Salt: "iHSDPHzUhPzK7rCcJgOFfg", + Hash: "J4moa2MM0/6uf3HbY2Tf5Fux8JIBTwIhmhxGRbsY14qhTltQt+Vw3b7tcJNEbk8ium8AQfZeD4tabCnNqfkD1g", + }, + false, + }, + { + "$pbkdf2", + PHC{ + Id: "pbkdf2", + Version: "", + Params: map[string]string{}, + Salt: "", + Hash: "", + }, + false, + }, + { + "$pbkdf2$cGFsZXN0aW5lIHdpbGwgYmUgZnJlZQ", + PHC{ + Id: "pbkdf2", + Version: "", + Params: map[string]string{}, + Salt: "cGFsZXN0aW5lIHdpbGwgYmUgZnJlZQ", + Hash: "", + }, + false, + }, + { + "$pbkdf2$cGFsZXN0aW5lIHdpbGwgYmUgZnJlZQ$EFpj2Mnn+EbXTxZD5kv5t5Y69wzPJnDEZI3BtqlRCH0", + PHC{ + Id: "pbkdf2", + Version: "", + Params: map[string]string{}, + Salt: "cGFsZXN0aW5lIHdpbGwgYmUgZnJlZQ", + Hash: "EFpj2Mnn+EbXTxZD5kv5t5Y69wzPJnDEZI3BtqlRCH0", + }, + false, + }, + { + "$pbkdf2$f=SHA256,w=600000,l=32$cGFsZXN0aW5lIHdpbGwgYmUgZnJlZQ$EFpj2Mnn+EbXTxZD5kv5t5Y69wzPJnDEZI3BtqlRCH0", + PHC{ + Id: "pbkdf2", + Version: "", + Params: map[string]string{ + "w": "600000", + "f": "SHA256", + "l": "32", + }, + Salt: "cGFsZXN0aW5lIHdpbGwgYmUgZnJlZQ", + Hash: "EFpj2Mnn+EbXTxZD5kv5t5Y69wzPJnDEZI3BtqlRCH0", + }, + false, + }, + { + "$pbkdf2$v=5$w=600000,f=SHA256,l=32$iHSDPHzUhPzK7rCcJgOFfg$J4moa2MM0/6uf3HbY2Tf5Fux8JIBTwIhmhxGRbsY14qhTltQt+Vw3b7tcJNEbk8ium8AQfZeD4tabCnNqfkD1g", + PHC{ + Id: "pbkdf2", + Version: "5", + Params: map[string]string{ + "w": "600000", + "f": "SHA256", + "l": "32", + }, + Salt: "iHSDPHzUhPzK7rCcJgOFfg", + Hash: "J4moa2MM0/6uf3HbY2Tf5Fux8JIBTwIhmhxGRbsY14qhTltQt+Vw3b7tcJNEbk8ium8AQfZeD4tabCnNqfkD1g", + }, + false, + }, + + ///////////////////////////////////////////////////////////////// + // Invalid strings + { + "", + PHC{}, + true, + }, + { + "$", + PHC{}, + true, + }, + { + "$pbkdf2$m=", + PHC{}, + true, + }, + { + "$pbkdf2$m=120,", + PHC{}, + true, + }, + { + "$pbkdf2$m=120,t=", + PHC{}, + true, + }, + { + "$pbkdf2$", + PHC{}, + true, + }, + { + "$pbkdf2$$$$", + PHC{}, + true, + }, + { + "$pbkdf2$v=5$v=600000,f=SHA256,l=32$iHSDPHzUhPzK7rCcJgOFfg$J4moa2MM0/6uf3HbY2Tf5Fux8JIBTwIhmhxGRbsY14qhTltQt+Vw3b7tcJNEbk8ium8AQfZeD4tabCnNqfkD1g", + PHC{}, + true, + }, + // We limit the input to MaxRunes, so any runes after the limit are ignored + { + "$pbkdf2$cGFsZXN0aW5lIHdpbGwgYmUgZnJlZQ" + strings.Repeat("a", MaxRunes), + PHC{ + Id: "pbkdf2", + Version: "", + Params: map[string]string{}, + Salt: "cGFsZXN0aW5lIHdpbGwgYmUgZnJlZQ" + strings.Repeat("a", MaxRunes-len("$pbkdf2$cGFsZXN0aW5lIHdpbGwgYmUgZnJlZQ")), + Hash: "", + }, + false, + }, + // Edge case + { + // This is a bcrypt hashed password: it looks like PHC, but it is not: + // The format is $xy$n$salthash, where: + // - $ is the literal '$' (1 byte) + // - x is the major version (1 byte) + // - y is the minor version (0 or 1 byte) + // - $ is the literal '$' (1 byte) + // - n is the cost (2 bytes) + // - $ is the literal '$' (1 byte) + // - salt is the encoded salt (22 bytes) + // - hash is the encoded hash (31 bytes) + // In total, 60 bytes (59 if there is no minor version) + // + // But this is not PHC-compliant: xy is not the function id, n is not + // a parameter name=value, nor the version, and there is no '$' + // separating the salt and the hash. + // + // However, it is *technically* PHC-compliant: + // - xy is parsed as a function ID + // - n is parsed as the salt + // - salthash is parsed as the hash + // + // This is somewhat of an edge-case, and should probably be rejected + // as non-parseable because "2a" is *not* a known function ID, but + // we don't check for specific strings as of today + "$2a$10$z0OlN1MpiLVlLTyE1xtEjOJ6/xV95RAwwIUaYKQBAqoeyvPgLEnUa", + PHC{ + Id: "2a", + Version: "", + Params: map[string]string{}, + Salt: "10", + Hash: "z0OlN1MpiLVlLTyE1xtEjOJ6/xV95RAwwIUaYKQBAqoeyvPgLEnUa", + }, + false, + }, + } + + for _, tc := range testCases { + p := New(strings.NewReader(tc.input)) + phc, err := p.Parse() + if tc.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, phc, tc.output) + } + } +} diff --git a/server/channels/app/user.go b/server/channels/app/user.go index 329b2d5811e..1208e917093 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -26,6 +26,7 @@ import ( "github.com/mattermost/mattermost/server/public/shared/request" "github.com/mattermost/mattermost/server/v8/channels/app/email" "github.com/mattermost/mattermost/server/v8/channels/app/imaging" + "github.com/mattermost/mattermost/server/v8/channels/app/password/hashers" "github.com/mattermost/mattermost/server/v8/channels/app/users" "github.com/mattermost/mattermost/server/v8/channels/store" "github.com/mattermost/mattermost/server/v8/einterfaces" @@ -1500,7 +1501,7 @@ func (a *App) UpdatePassword(rctx request.CTX, user *model.User, newPassword str return model.NewAppError("UpdatePassword", "api.user.update_password.failed.app_error", nil, "", http.StatusInternalServerError) } - hashedPassword, err := model.HashPassword(newPassword) + hashedPassword, err := hashers.Hash(newPassword) if err != nil { // can't be password length (checked in IsPasswordValid) return model.NewAppError("UpdatePassword", "api.user.update_password.password_hash.app_error", nil, "user_id="+user.Id, http.StatusInternalServerError).Wrap(err) diff --git a/server/channels/app/users/errors.go b/server/channels/app/users/errors.go index f5ddbc88392..acc7803bfb6 100644 --- a/server/channels/app/users/errors.go +++ b/server/channels/app/users/errors.go @@ -18,6 +18,8 @@ var ( UserInitialsError = errors.New("could not get user initials") ImageEncodingError = errors.New("could not encode image") GlyphError = errors.New("could not get glyph") + + InvalidPasswordError = NewErrInvalidPassword("") ) // ErrInvalidPassword indicates an error against the password settings diff --git a/server/channels/app/users/password.go b/server/channels/app/users/password.go index 4282c7e6503..5615db26f74 100644 --- a/server/channels/app/users/password.go +++ b/server/channels/app/users/password.go @@ -4,30 +4,11 @@ package users import ( - "errors" "strings" - "golang.org/x/crypto/bcrypt" - "github.com/mattermost/mattermost/server/public/model" ) -func CheckUserPassword(user *model.User, password string) error { - if err := ComparePassword(user.Password, password); err != nil { - return NewErrInvalidPassword("") - } - - return nil -} - -func ComparePassword(hash string, password string) error { - if password == "" || hash == "" { - return errors.New("empty password or hash") - } - - return bcrypt.CompareHashAndPassword([]byte(hash), []byte(password)) -} - func (us *UserService) isPasswordValid(password string) error { return IsPasswordValidWithSettings(password, &us.config().PasswordSettings) } diff --git a/server/channels/store/storetest/user_store.go b/server/channels/store/storetest/user_store.go index a893ae869a8..fb8a658e305 100644 --- a/server/channels/store/storetest/user_store.go +++ b/server/channels/store/storetest/user_store.go @@ -12,13 +12,12 @@ import ( "testing" "time" - "golang.org/x/crypto/bcrypt" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/shared/request" + "github.com/mattermost/mattermost/server/v8/channels/app/password/hashers" "github.com/mattermost/mattermost/server/v8/channels/store" ) @@ -2324,10 +2323,10 @@ func testUserStoreUpdatePassword(t *testing.T, rctx request.CTX, ss store.Store) _, nErr := ss.Team().SaveMember(rctx, &model.TeamMember{TeamId: teamID, UserId: u1.Id}, -1) require.NoError(t, nErr) - _, err = model.HashPassword(strings.Repeat("1234567890", 8)) - require.ErrorIs(t, err, bcrypt.ErrPasswordTooLong) + _, err = hashers.Hash(strings.Repeat("1234567890", 8)) + require.ErrorIs(t, err, hashers.ErrPasswordTooLong) - hashedPassword, err := model.HashPassword("newpwd") + hashedPassword, err := hashers.Hash("newpwd") require.NoError(t, err) err = ss.User().UpdatePassword(u1.Id, hashedPassword) diff --git a/server/i18n/en.json b/server/i18n/en.json index e6044ce116a..729c15ffc6b 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -4006,6 +4006,10 @@ "id": "api.user.check_user_password.invalid.app_error", "translation": "Login failed because of invalid password." }, + { + "id": "api.user.check_user_password.invalid_hash.app_error", + "translation": "The format of the hashed password is invalid and could not be parsed." + }, { "id": "api.user.complete_switch_with_oauth.blank_email.app_error", "translation": "Blank email." @@ -7464,6 +7468,14 @@ "id": "app.user.analytics_get_inactive_users_count.app_error", "translation": "We could not count the inactive users." }, + { + "id": "app.user.check_user_password.failed_migration", + "translation": "Failed to migrate the user's password to the newest hashing method." + }, + { + "id": "app.user.check_user_password.failed_update", + "translation": "Failed to update the user's password." + }, { "id": "app.user.clear_all_custom_role_assignments.select.app_error", "translation": "Failed to retrieve the users." diff --git a/server/public/model/user.go b/server/public/model/user.go index 715b1254aa5..3e2ec00e2fb 100644 --- a/server/public/model/user.go +++ b/server/public/model/user.go @@ -17,11 +17,11 @@ import ( "github.com/pkg/errors" - "golang.org/x/crypto/bcrypt" "golang.org/x/text/language" "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/public/shared/timezones" + "github.com/mattermost/mattermost/server/v8/channels/app/password/hashers" ) const ( @@ -509,8 +509,8 @@ func (u *User) PreSave() *AppError { } if u.Password != "" { - hashed, err := HashPassword(u.Password) - if errors.Is(err, bcrypt.ErrPasswordTooLong) { + hashed, err := hashers.Hash(u.Password) + if errors.Is(err, hashers.ErrPasswordTooLong) { return NewAppError("User.PreSave", "model.user.pre_save.password_too_long.app_error", nil, "user_id="+u.Id, http.StatusBadRequest).Wrap(err) } else if err != nil { @@ -993,16 +993,6 @@ func (u *UserPatch) SetField(fieldName string, fieldValue string) { } } -// HashPassword generates a hash using the bcrypt.GenerateFromPassword -func HashPassword(password string) (string, error) { - hash, err := bcrypt.GenerateFromPassword([]byte(password), 10) - if err != nil { - return "", err - } - - return string(hash), nil -} - var validUsernameChars = regexp.MustCompile(`^[a-z0-9\.\-_]+$`) var validUsernameCharsForRemote = regexp.MustCompile(`^[a-z0-9\.\-_:]*$`) diff --git a/server/public/model/user_test.go b/server/public/model/user_test.go index ac4296ca753..8e18b335df8 100644 --- a/server/public/model/user_test.go +++ b/server/public/model/user_test.go @@ -11,10 +11,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "golang.org/x/crypto/bcrypt" "github.com/mattermost/mattermost/server/public/shared/mlog" "github.com/mattermost/mattermost/server/public/shared/timezones" + "github.com/mattermost/mattermost/server/v8/channels/app/password/hashers" ) func TestUserAuditable(t *testing.T) { @@ -220,7 +220,7 @@ func TestUserPreSave(t *testing.T) { func TestUserPreSavePwdTooLong(t *testing.T) { user := User{Password: strings.Repeat("1234567890", 8)} err := user.PreSave() - assert.ErrorIs(t, err, bcrypt.ErrPasswordTooLong) + assert.ErrorIs(t, err, hashers.ErrPasswordTooLong) } func TestUserPreUpdate(t *testing.T) {