[FIX] Hash Password Cmd Not Encoding Provided Salt (#999)

* using authelia hash-password if you provide a salt it doesn't encode it as a base64 string
* this causes invalid salts to be stored if a user manually provided one instead of reliance on the automatic generation
* additionally bumped the minimum required salt length to 8 as per reference spec
* additionally removed the maximum salt length as per reference spec (actually 2^32-1 per int32)
* see docs:
  * https://tools.ietf.org/html/draft-irtf-cfrg-argon2-10
  * https://github.com/P-H-C/phc-winner-argon2
  * https://github.com/P-H-C/phc-string-format
* encode all salts
* fix edge case of false positive in CheckPassword
* bump crypt version and fix tests

cmd/authelia/main.go

@@ -133,7 +133,7 @@ func main() {
 		Use:   "version",
 		Short: "Show the version of Authelia",
 		Run: func(cmd *cobra.Command, args []string) {
-			fmt.Printf("Authelia version %s, build %s", BuildTag, BuildCommit)
+			fmt.Printf("Authelia version %s, build %s\n", BuildTag, BuildCommit)
 		},
 	}
 

docs/configuration/authentication/file.md

@@ -178,7 +178,7 @@ parameters below, or for a more in depth understanding see the referenced docume
 
 #### salt_length
   - Value Type: Int
-  - Possible Value: between `2` and `16`
+  - Possible Value: `8` or higher.
   - Recommended: `16`
   - What it Does: Adjusts the length of the random salt we add to the password, there
    is no reason not to set this to 16

docs/configuration/index.md

@@ -26,7 +26,8 @@ You may also optionally validate your configuration against this validation proc
 option with the Authelia binary as shown below. Keep in mind if you're using [secrets](./secrets.md) you will have to 
 manually provide these if you don't want to get certain validation errors (specifically requesting you provide one of 
 the secret values). You can choose to ignore them if you know what you're doing. This command is useful prior to 
-upgrading to prevent configuration changes from impacting downtime in an upgrade.
+upgrading to prevent configuration changes from impacting downtime in an upgrade. This process does not validate
+integrations, it only checks that your configuration syntax is valid.
 
     $ authelia validate-config configuration.yml
     

go.mod

@@ -23,7 +23,7 @@ require (
 	github.com/otiai10/copy v1.1.1
 	github.com/pelletier/go-toml v1.4.0 // indirect
 	github.com/pquerna/otp v1.2.0
-	github.com/simia-tech/crypt v0.4.2
+	github.com/simia-tech/crypt v0.4.3
 	github.com/sirupsen/logrus v1.6.0
 	github.com/spf13/cobra v0.0.7
 	github.com/spf13/viper v1.7.0

go.sum

@@ -288,6 +288,8 @@ github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg
 github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc=
 github.com/simia-tech/crypt v0.4.2 h1:ZQFyCxgImhXpyxWNXEtBfAmV6T8dT1w481fpm8blQww=
 github.com/simia-tech/crypt v0.4.2/go.mod h1:DMwvjPTzsiHrjqHVW5HvIbF4vUUzMCYDKVLsPWmLdTo=
+github.com/simia-tech/crypt v0.4.3 h1:aljHxrQWZFUuTWGhLsCwr+0fwCBqDjEaRVyq69PfltY=
+github.com/simia-tech/crypt v0.4.3/go.mod h1:DMwvjPTzsiHrjqHVW5HvIbF4vUUzMCYDKVLsPWmLdTo=
 github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
 github.com/sirupsen/logrus v1.5.0 h1:1N5EYkVAPEywqZRJd7cwnRtCb6xJx7NH3T3WUTF980Q=
 github.com/sirupsen/logrus v1.5.0/go.mod h1:+F7Ogzej0PZc/94MaYx/nvG9jOFMD2osvC3s+Squfpo=

internal/authentication/password_hash.go

@@ -123,7 +123,7 @@ func HashPassword(password, salt string, algorithm CryptAlgo, iterations, memory
 	}
 
 	if salt == "" {
-		salt = utils.RandomString(saltLength, HashingPossibleSaltCharacters)
+		salt = crypt.Base64Encoding.EncodeToString([]byte(utils.RandomString(saltLength, HashingPossibleSaltCharacters)))
 	}
 
 	settings = getCryptSettings(salt, algorithm, iterations, memory, parallelism, keyLength)
@@ -136,17 +136,22 @@ func HashPassword(password, salt string, algorithm CryptAlgo, iterations, memory
 
 // CheckPassword check a password against a hash.
 func CheckPassword(password, hash string) (ok bool, err error) {
-	passwordHash, err := ParseHash(hash)
+	expectedHash, err := ParseHash(hash)
 	if err != nil {
 		return false, err
 	}
 
-	expectedHash, err := HashPassword(password, passwordHash.Salt, passwordHash.Algorithm, passwordHash.Iterations, passwordHash.Memory, passwordHash.Parallelism, passwordHash.KeyLength, len(passwordHash.Salt))
+	passwordHashString, err := HashPassword(password, expectedHash.Salt, expectedHash.Algorithm, expectedHash.Iterations, expectedHash.Memory, expectedHash.Parallelism, expectedHash.KeyLength, len(expectedHash.Salt))
 	if err != nil {
 		return false, err
 	}
 
-	return hash == expectedHash, nil
+	passwordHash, err := ParseHash(passwordHashString)
+	if err != nil {
+		return false, err
+	}
+
+	return passwordHash.Key == expectedHash.Key, nil
 }
 
 func getCryptSettings(salt string, algorithm CryptAlgo, iterations, memory, parallelism, keyLength int) (settings string) {
@@ -165,17 +170,20 @@ func getCryptSettings(salt string, algorithm CryptAlgo, iterations, memory, para
 // 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)
+		if saltLength < 8 {
+			return fmt.Errorf("Salt length input of %d is invalid, it must be 8 or higher", 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
+	}
+
+	decodedSalt, err := crypt.Base64Encoding.DecodeString(salt)
+	if err != nil {
+		return fmt.Errorf("Salt input of %s is invalid, only base64 strings are valid for input", salt)
+	}
+
+	if len(decodedSalt) < 8 {
+		return fmt.Errorf("Salt input of %s is invalid (%d characters), it must be 8 or more characters", decodedSalt, len(decodedSalt))
 	}
 
 	return nil

internal/authentication/password_hash_test.go

@@ -45,6 +45,13 @@ func TestShouldHashArgon2idPassword(t *testing.T) {
 	assert.Equal(t, schema.DefaultCIPasswordConfiguration.KeyLength, parameters.GetInt("k", HashingDefaultArgon2idKeyLength))
 }
 
+func TestShouldValidateArgon2idHashWithTEqualOne(t *testing.T) {
+	hash := "$argon2id$v=19$m=1024,t=1,p=1,k=16$c2FsdG9uY2U$Sk4UjzxXdCrBcyyMYiPEsQ"
+	valid, err := CheckPassword("apple", hash)
+	assert.True(t, valid)
+	assert.NoError(t, err)
+}
+
 // This checks the method of hashing (for argon2id) supports all the characters we allow in Authelia's hash function.
 func TestArgon2idHashSaltValidValues(t *testing.T) {
 	var err error
@@ -57,7 +64,7 @@ func TestArgon2idHashSaltValidValues(t *testing.T) {
 	for _, salt := range datas {
 		hash, err = HashPassword("password", salt, HashingAlgorithmArgon2id, 1, 8, 1, 32, 16)
 		assert.NoError(t, err)
-		assert.Equal(t, fmt.Sprintf("$argon2id$v=19$m=8,p=1$%s$", salt), hash[0:40])
+		assert.Equal(t, fmt.Sprintf("$argon2id$v=19$m=8,t=1,p=1$%s$", salt), hash[0:44])
 	}
 }
 
@@ -138,32 +145,17 @@ func TestShouldNotHashPasswordDueToSaltLength(t *testing.T) {
 		schema.DefaultCIPasswordConfiguration.Parallelism, schema.DefaultCIPasswordConfiguration.KeyLength, 0)
 
 	assert.Equal(t, "", hash)
-	assert.EqualError(t, err, "Salt length input of 0 is invalid, it must be 2 or higher")
-
-	hash, err = HashPassword("password", "", HashingAlgorithmArgon2id,
-		schema.DefaultCIPasswordConfiguration.Iterations, schema.DefaultCIPasswordConfiguration.Memory*1024,
-		schema.DefaultCIPasswordConfiguration.Parallelism, schema.DefaultCIPasswordConfiguration.KeyLength, 20)
-
-	assert.Equal(t, "", hash)
-	assert.EqualError(t, err, "Salt length input of 20 is invalid, it must be 16 or lower")
-}
-
-func TestShouldNotHashPasswordDueToSaltCharLengthTooLong(t *testing.T) {
-	hash, err := HashPassword("password", "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789", HashingAlgorithmArgon2id,
-		schema.DefaultCIPasswordConfiguration.Iterations, schema.DefaultCIPasswordConfiguration.Memory*1024,
-		schema.DefaultCIPasswordConfiguration.Parallelism, schema.DefaultCIPasswordConfiguration.KeyLength,
-		schema.DefaultCIPasswordConfiguration.SaltLength)
-	assert.Equal(t, "", hash)
-	assert.EqualError(t, err, "Salt input of abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 is invalid (62 characters), it must be 16 or fewer characters")
+	assert.EqualError(t, err, "Salt length input of 0 is invalid, it must be 8 or higher")
 }
 
 func TestShouldNotHashPasswordDueToSaltCharLengthTooShort(t *testing.T) {
-	hash, err := HashPassword("password", "a", HashingAlgorithmArgon2id,
+	// The salt 'YQ' is the base64 value for 'a' which is why the length is 1.
+	hash, err := HashPassword("password", "YQ", HashingAlgorithmArgon2id,
 		schema.DefaultCIPasswordConfiguration.Iterations, schema.DefaultCIPasswordConfiguration.Memory*1024,
 		schema.DefaultCIPasswordConfiguration.Parallelism, schema.DefaultCIPasswordConfiguration.KeyLength,
 		schema.DefaultCIPasswordConfiguration.SaltLength)
 	assert.Equal(t, "", hash)
-	assert.EqualError(t, err, "Salt input of a is invalid (1 characters), it must be 2 or more characters")
+	assert.EqualError(t, err, "Salt input of a is invalid (1 characters), it must be 8 or more characters")
 }
 
 func TestShouldNotHashPasswordWithNonBase64CharsInSalt(t *testing.T) {
@@ -172,7 +164,7 @@ func TestShouldNotHashPasswordWithNonBase64CharsInSalt(t *testing.T) {
 		schema.DefaultCIPasswordConfiguration.Parallelism, schema.DefaultCIPasswordConfiguration.KeyLength,
 		schema.DefaultCIPasswordConfiguration.SaltLength)
 	assert.Equal(t, "", hash)
-	assert.EqualError(t, err, "Salt input of abc&123 is invalid, only characters [a-zA-Z0-9+/] are valid for input")
+	assert.EqualError(t, err, "Salt input of abc&123 is invalid, only base64 strings are valid for input")
 }
 
 func TestShouldNotParseHashWithNoneBase64CharsInKey(t *testing.T) {

internal/commands/hash.go

@@ -2,7 +2,9 @@ package commands
 
 import (
 	"fmt"
+	"log"
 
+	"github.com/simia-tech/crypt"
 	"github.com/spf13/cobra"
 
 	"github.com/authelia/authelia/internal/authentication"
@@ -44,12 +46,15 @@ var HashPasswordCmd = &cobra.Command{
 		} else {
 			algorithm = authentication.HashingAlgorithmArgon2id
 		}
+		if salt != "" {
+			salt = crypt.Base64Encoding.EncodeToString([]byte(salt))
+		}
 
 		hash, err = authentication.HashPassword(args[0], salt, algorithm, iterations, memory*1024, parallelism, keyLength, saltLength)
 		if err != nil {
-			fmt.Printf("Error occurred during hashing: %s", err)
+			log.Fatalf("Error occurred during hashing: %s\n", err)
 		} else {
-			fmt.Printf("Password hash: %s", hash)
+			fmt.Printf("Password hash: %s\n", hash)
 		}
 	},
 	Args: cobra.MinimumNArgs(1),

internal/configuration/validator/authentication.go

@@ -43,10 +43,8 @@ func validateFileAuthenticationBackend(configuration *schema.FileAuthenticationB
 		switch {
 		case configuration.Password.SaltLength == 0:
 			configuration.Password.SaltLength = schema.DefaultPasswordConfiguration.SaltLength
-		case configuration.Password.SaltLength < 2:
+		case configuration.Password.SaltLength < 8:
 			validator.Push(fmt.Errorf("The salt length must be 2 or more, you configured %d", configuration.Password.SaltLength))
-		case configuration.Password.SaltLength > 16:
-			validator.Push(fmt.Errorf("The salt length must be 16 or less, you configured %d", configuration.Password.SaltLength))
 		}
 
 		if configuration.Password.Algorithm == argon2id {

internal/configuration/validator/authentication_test.go

@@ -109,13 +109,6 @@ func (suite *FileBasedAuthenticationBackend) TestShouldRaiseErrorWhenSaltLengthT
 	assert.EqualError(suite.T(), suite.validator.Errors()[0], "The salt length must be 2 or more, you configured -1")
 }
 
-func (suite *FileBasedAuthenticationBackend) TestShouldRaiseErrorWhenSaltLengthTooHigh() {
-	suite.configuration.File.Password.SaltLength = 20
-	ValidateAuthenticationBackend(&suite.configuration, suite.validator)
-	assert.Len(suite.T(), suite.validator.Errors(), 1)
-	assert.EqualError(suite.T(), suite.validator.Errors()[0], "The salt length must be 16 or less, you configured 20")
-}
-
 func (suite *FileBasedAuthenticationBackend) TestShouldRaiseErrorWhenBadAlgorithmDefined() {
 	suite.configuration.File.Password.Algorithm = "bogus"
 	ValidateAuthenticationBackend(&suite.configuration, suite.validator)