From 332a68541c33497694582f39d101ddba3118853d Mon Sep 17 00:00:00 2001 From: James Elliott Date: Fri, 8 May 2020 13:38:22 +1000 Subject: [PATCH] [MISC] Refactor Authentication (#987) * only do salt validation in validate salt * fix tests * remove panic(err.Error()) * use file mode const * do hash cleanup on file read instead of check password * design ConfigAlgoToCryptoAlgo and implement it * split HashPassword func into functional chunks that could theoretically be reused --- internal/authentication/const.go | 2 + internal/authentication/file_user_provider.go | 35 +++---- .../authentication/file_user_provider_test.go | 19 ++-- internal/authentication/password_hash.go | 98 ++++++++++++------- 4 files changed, 92 insertions(+), 62 deletions(-) diff --git a/internal/authentication/const.go b/internal/authentication/const.go index b929736a..0be0af1a 100644 --- a/internal/authentication/const.go +++ b/internal/authentication/const.go @@ -58,3 +58,5 @@ const argon2id = "argon2id" const sha512 = "sha512" const testPassword = "my;secure*password" + +const fileAuthenticationMode = 0600 diff --git a/internal/authentication/file_user_provider.go b/internal/authentication/file_user_provider.go index 90b2083d..18e38ad6 100644 --- a/internal/authentication/file_user_provider.go +++ b/internal/authentication/file_user_provider.go @@ -1,7 +1,6 @@ package authentication import ( - "errors" "fmt" "io/ioutil" "strings" @@ -42,13 +41,13 @@ func NewFileUserProvider(configuration *schema.FileAuthenticationBackendConfigur database, err := readDatabase(configuration.Path) if err != nil { // Panic since the file does not exist when Authelia is starting. - panic(err.Error()) + panic(err) } // Early check whether hashed passwords are correct for all users err = checkPasswordHashes(database) if err != nil { - panic(err.Error()) + panic(err) } var cryptAlgo CryptAlgo = HashingAlgorithmArgon2id @@ -74,10 +73,14 @@ func NewFileUserProvider(configuration *schema.FileAuthenticationBackendConfigur func checkPasswordHashes(database *DatabaseModel) error { for u, v := range database.Users { + v.HashedPassword = strings.ReplaceAll(v.HashedPassword, "{CRYPT}", "") _, err := ParseHash(v.HashedPassword) + if err != nil { return fmt.Errorf("Unable to parse hash of user %s: %s", u, err) } + + database.Users[u] = v } return nil @@ -111,9 +114,7 @@ func readDatabase(path string) (*DatabaseModel, error) { // CheckUserPassword checks if provided password matches for the given user. func (p *FileUserProvider) CheckUserPassword(username string, password string) (bool, error) { if details, ok := p.database.Users[username]; ok { - hashedPassword := strings.ReplaceAll(details.HashedPassword, "{CRYPT}", "") - - ok, err := CheckPassword(password, hashedPassword) + ok, err := CheckPassword(password, details.HashedPassword) if err != nil { return false, err } @@ -122,10 +123,9 @@ func (p *FileUserProvider) CheckUserPassword(username string, password string) ( } // TODO: Remove this. This is only here to temporarily fix the username enumeration security flaw in #949. - hashedPassword := strings.ReplaceAll(p.fakeHash, "{CRYPT}", "") - _, err := CheckPassword(password, hashedPassword) + _, _ = CheckPassword(password, p.fakeHash) - return false, err + return false, ErrUserNotFound } // GetDetails retrieve the groups a user belongs to. @@ -145,24 +145,19 @@ func (p *FileUserProvider) GetDetails(username string) (*UserDetails, error) { func (p *FileUserProvider) UpdatePassword(username string, newPassword string) error { details, ok := p.database.Users[username] if !ok { - return fmt.Errorf("User '%s' does not exist in database", username) + return ErrUserNotFound } - var algorithm CryptAlgo - - switch p.configuration.Password.Algorithm { - case argon2id: - algorithm = HashingAlgorithmArgon2id - case sha512: - algorithm = HashingAlgorithmSHA512 - default: - return errors.New("Invalid algorithm in configuration. It should be `argon2id` or `sha512`") + algorithm, err := ConfigAlgoToCryptoAlgo(p.configuration.Password.Algorithm) + if err != nil { + return err } hash, err := HashPassword( newPassword, "", algorithm, p.configuration.Password.Iterations, p.configuration.Password.Memory*1024, p.configuration.Password.Parallelism, p.configuration.Password.KeyLength, p.configuration.Password.SaltLength) + if err != nil { return err } @@ -178,7 +173,7 @@ func (p *FileUserProvider) UpdatePassword(username string, newPassword string) e return err } - err = ioutil.WriteFile(p.configuration.Path, b, 0644) //nolint:gosec // Fixed in future PR. + err = ioutil.WriteFile(p.configuration.Path, b, fileAuthenticationMode) p.lock.Unlock() return err diff --git a/internal/authentication/file_user_provider_test.go b/internal/authentication/file_user_provider_test.go index 7e7dda16..2c9dbe1f 100644 --- a/internal/authentication/file_user_provider_test.go +++ b/internal/authentication/file_user_provider_test.go @@ -86,8 +86,9 @@ func TestShouldCheckUserPasswordOfUserThatDoesNotExist(t *testing.T) { config.Path = path provider := NewFileUserProvider(&config) ok, err := provider.CheckUserPassword("fake", "password") - assert.NoError(t, err) + assert.Error(t, err) assert.Equal(t, false, ok) + assert.EqualError(t, err, "user not found") }) } @@ -126,7 +127,7 @@ func TestShouldUpdatePasswordHashingAlgorithmToArgon2id(t *testing.T) { config := DefaultFileAuthenticationBackendConfiguration config.Path = path provider := NewFileUserProvider(&config) - assert.True(t, strings.HasPrefix(provider.database.Users["harry"].HashedPassword, "{CRYPT}$6$")) + assert.True(t, strings.HasPrefix(provider.database.Users["harry"].HashedPassword, "$6$")) err := provider.UpdatePassword("harry", "newpassword") assert.NoError(t, err) @@ -147,7 +148,7 @@ func TestShouldUpdatePasswordHashingAlgorithmToSHA512(t *testing.T) { config.Password.Iterations = 50000 provider := NewFileUserProvider(&config) - assert.True(t, strings.HasPrefix(provider.database.Users["john"].HashedPassword, "{CRYPT}$argon2id$")) + assert.True(t, strings.HasPrefix(provider.database.Users["john"].HashedPassword, "$argon2id$")) err := provider.UpdatePassword("john", "newpassword") assert.NoError(t, err) @@ -164,7 +165,7 @@ func TestShouldRaiseWhenLoadingMalformedDatabaseForFirstTime(t *testing.T) { WithDatabase(MalformedUserDatabaseContent, func(path string) { config := DefaultFileAuthenticationBackendConfiguration config.Path = path - assert.PanicsWithValue(t, "Unable to parse database: yaml: line 4: mapping values are not allowed in this context", func() { + assert.PanicsWithError(t, "Unable to parse database: yaml: line 4: mapping values are not allowed in this context", func() { NewFileUserProvider(&config) }) }) @@ -174,7 +175,7 @@ func TestShouldRaiseWhenLoadingDatabaseWithBadSchemaForFirstTime(t *testing.T) { WithDatabase(BadSchemaUserDatabaseContent, func(path string) { config := DefaultFileAuthenticationBackendConfiguration config.Path = path - assert.PanicsWithValue(t, "Invalid schema of database: Users: non zero value required", func() { + assert.PanicsWithError(t, "Invalid schema of database: Users: non zero value required", func() { NewFileUserProvider(&config) }) }) @@ -184,7 +185,7 @@ func TestShouldRaiseWhenLoadingDatabaseWithBadSHA512HashesForTheFirstTime(t *tes WithDatabase(BadSHA512HashContent, func(path string) { config := DefaultFileAuthenticationBackendConfiguration config.Path = path - assert.PanicsWithValue(t, "Unable to parse hash of user john: Hash key is not the last parameter, the hash is likely malformed ($6$rounds00000$jgiCMRyGXzoqpxS3$w2pJeZnnH8bwW3zzvoMWtTRfQYsHbWbD/hquuQ5vUeIyl9gdwBIt6RWk2S6afBA0DPakbeWgD/4SZPiS0hYtU/)", func() { + assert.PanicsWithError(t, "Unable to parse hash of user john: Hash key is not the last parameter, the hash is likely malformed ($6$rounds00000$jgiCMRyGXzoqpxS3$w2pJeZnnH8bwW3zzvoMWtTRfQYsHbWbD/hquuQ5vUeIyl9gdwBIt6RWk2S6afBA0DPakbeWgD/4SZPiS0hYtU/)", func() { NewFileUserProvider(&config) }) }) @@ -194,7 +195,7 @@ func TestShouldRaiseWhenLoadingDatabaseWithBadArgon2idHashSettingsForTheFirstTim WithDatabase(BadArgon2idHashSettingsContent, func(path string) { config := DefaultFileAuthenticationBackendConfiguration config.Path = path - assert.PanicsWithValue(t, "Unable to parse hash of user john: Hash key is not the last parameter, the hash is likely malformed ($argon2id$v=19$m65536,t3,p2$BpLnfgDsc2WD8F2q$o/vzA4myCqZZ36bUGsDY//8mKUYNZZaR0t4MFFSs+iM)", func() { + assert.PanicsWithError(t, "Unable to parse hash of user john: Hash key is not the last parameter, the hash is likely malformed ($argon2id$v=19$m65536,t3,p2$BpLnfgDsc2WD8F2q$o/vzA4myCqZZ36bUGsDY//8mKUYNZZaR0t4MFFSs+iM)", func() { NewFileUserProvider(&config) }) }) @@ -204,7 +205,7 @@ func TestShouldRaiseWhenLoadingDatabaseWithBadArgon2idHashKeyForTheFirstTime(t * WithDatabase(BadArgon2idHashKeyContent, func(path string) { config := DefaultFileAuthenticationBackendConfiguration config.Path = path - assert.PanicsWithValue(t, "Unable to parse hash of user john: Hash key contains invalid base64 characters", func() { + assert.PanicsWithError(t, "Unable to parse hash of user john: Hash key contains invalid base64 characters", func() { NewFileUserProvider(&config) }) }) @@ -214,7 +215,7 @@ func TestShouldRaiseWhenLoadingDatabaseWithBadArgon2idHashSaltForTheFirstTime(t WithDatabase(BadArgon2idHashSaltContent, func(path string) { config := DefaultFileAuthenticationBackendConfiguration config.Path = path - assert.PanicsWithValue(t, "Unable to parse hash of user john: Salt contains invalid base64 characters", func() { + assert.PanicsWithError(t, "Unable to parse hash of user john: Salt contains invalid base64 characters", func() { NewFileUserProvider(&config) }) }) diff --git a/internal/authentication/password_hash.go b/internal/authentication/password_hash.go index a966b657..b9a70b05 100644 --- a/internal/authentication/password_hash.go +++ b/internal/authentication/password_hash.go @@ -23,6 +23,18 @@ type PasswordHash struct { Parallelism int } +// ConfigAlgoToCryptoAlgo returns a CryptAlgo and nil error if valid, otherwise it returns argon2id and an error. +func ConfigAlgoToCryptoAlgo(fromConfig string) (CryptAlgo, error) { + switch fromConfig { + case argon2id: + return HashingAlgorithmArgon2id, nil + case sha512: + return HashingAlgorithmSHA512, nil + default: + return HashingAlgorithmArgon2id, errors.New("Invalid algorithm in configuration. It should be `argon2id` or `sha512`") + } +} + // ParseHash extracts all characteristics of a hash given its string representation. func ParseHash(hash string) (passwordHash *PasswordHash, err error) { parts := strings.Split(hash, "$") @@ -91,7 +103,6 @@ func ParseHash(hash string) (passwordHash *PasswordHash, err error) { } // HashPassword generate a salt and hash the password with the salt and a constant number of rounds. -//nolint:gocyclo // TODO: Consider refactoring/simplifying, time permitting. func HashPassword(password, salt string, algorithm CryptAlgo, iterations, memory, parallelism, keyLength, saltLength int) (hash string, err error) { var settings string @@ -99,41 +110,16 @@ func HashPassword(password, salt string, algorithm CryptAlgo, iterations, memory return "", fmt.Errorf("Hashing algorithm input of '%s' is invalid, only values of %s and %s are supported", algorithm, HashingAlgorithmArgon2id, HashingAlgorithmSHA512) } - if salt == "" { - if saltLength < 2 { - return "", fmt.Errorf("Salt length input of %d is invalid, it must be 2 or higher", saltLength) - } else if saltLength > 16 { - return "", fmt.Errorf("Salt length input of %d is invalid, it must be 16 or lower", saltLength) + if algorithm == HashingAlgorithmArgon2id { + err := validateArgon2idSettings(memory, parallelism, iterations, keyLength) + if err != nil { + return "", err } - } else if len(salt) > 16 { - return "", fmt.Errorf("Salt input of %s is invalid (%d characters), it must be 16 or fewer characters", salt, len(salt)) - } else if len(salt) < 2 { - return "", fmt.Errorf("Salt input of %s is invalid (%d characters), it must be 2 or more characters", salt, len(salt)) - } else if _, err = crypt.Base64Encoding.DecodeString(salt); err != nil { - return "", fmt.Errorf("Salt input of %s is invalid, only characters [a-zA-Z0-9+/] are valid for input", salt) } - if algorithm == HashingAlgorithmArgon2id { - // Caution: Increasing any of the values in the below block has a high chance in old passwords that cannot be verified. - if memory < 8 { - return "", fmt.Errorf("Memory (argon2id) input of %d is invalid, it must be 8 or higher", memory) - } - - if parallelism < 1 { - return "", fmt.Errorf("Parallelism (argon2id) input of %d is invalid, it must be 1 or higher", parallelism) - } - - if memory < parallelism*8 { - return "", fmt.Errorf("Memory (argon2id) input of %d is invalid with a parallelism input of %d, it must be %d (parallelism * 8) or higher", memory, parallelism, parallelism*8) - } - - if keyLength < 16 { - return "", fmt.Errorf("Key length (argon2id) input of %d is invalid, it must be 16 or higher", keyLength) - } - - if iterations < 1 { - return "", fmt.Errorf("Iterations (argon2id) input of %d is invalid, it must be 1 or more", iterations) - } + err = validateSalt(salt, saltLength) + if err != nil { + return "", err } if salt == "" { @@ -175,3 +161,49 @@ func getCryptSettings(salt string, algorithm CryptAlgo, iterations, memory, para return settings } + +// validateSalt checks the salt input and settings are valid and returns it and a nil error if they are, otherwise returns an error. +func validateSalt(salt string, saltLength int) error { + if salt == "" { + if saltLength < 2 { + return fmt.Errorf("Salt length input of %d is invalid, it must be 2 or higher", saltLength) + } else if saltLength > 16 { + return fmt.Errorf("Salt length input of %d is invalid, it must be 16 or lower", saltLength) + } + } else if len(salt) > 16 { + return fmt.Errorf("Salt input of %s is invalid (%d characters), it must be 16 or fewer characters", salt, len(salt)) + } else if len(salt) < 2 { + return fmt.Errorf("Salt input of %s is invalid (%d characters), it must be 2 or more characters", salt, len(salt)) + } else if _, err := crypt.Base64Encoding.DecodeString(salt); err != nil { + return fmt.Errorf("Salt input of %s is invalid, only characters [a-zA-Z0-9+/] are valid for input", salt) + } + + return nil +} + +// validateArgon2idSettings checks the argon2id settings are valid. +func validateArgon2idSettings(memory, parallelism, iterations, keyLength int) error { + // Caution: Increasing any of the values in the below block has a high chance in old passwords that cannot be verified. + if memory < 8 { + return fmt.Errorf("Memory (argon2id) input of %d is invalid, it must be 8 or higher", memory) + } + + if parallelism < 1 { + return fmt.Errorf("Parallelism (argon2id) input of %d is invalid, it must be 1 or higher", parallelism) + } + + if memory < parallelism*8 { + return fmt.Errorf("Memory (argon2id) input of %d is invalid with a parallelism input of %d, it must be %d (parallelism * 8) or higher", memory, parallelism, parallelism*8) + } + + if keyLength < 16 { + return fmt.Errorf("Key length (argon2id) input of %d is invalid, it must be 16 or higher", keyLength) + } + + if iterations < 1 { + return fmt.Errorf("Iterations (argon2id) input of %d is invalid, it must be 1 or more", iterations) + } + + // Caution: Increasing any of the values in the above block has a high chance in old passwords that cannot be verified. + return nil +}