[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

@@ -58,3 +58,5 @@ const argon2id = "argon2id"
 const sha512 = "sha512"
 
 const testPassword = "my;secure*password"
+
+const fileAuthenticationMode = 0600

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

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)
 		})
 	})

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
+}