From 73bd2e447909567dab37759eeab8aa45de4beb6b Mon Sep 17 00:00:00 2001 From: James Elliott Date: Thu, 14 May 2020 15:55:03 +1000 Subject: [PATCH] [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 | 2 +- docs/configuration/authentication/file.md | 2 +- docs/configuration/index.md | 3 +- go.mod | 2 +- go.sum | 2 ++ internal/authentication/password_hash.go | 36 +++++++++++-------- internal/authentication/password_hash_test.go | 34 +++++++----------- internal/commands/hash.go | 9 +++-- .../configuration/validator/authentication.go | 4 +-- .../validator/authentication_test.go | 7 ---- 10 files changed, 50 insertions(+), 51 deletions(-) diff --git a/cmd/authelia/main.go b/cmd/authelia/main.go index e234feed6..6ada8e1a7 100644 --- a/cmd/authelia/main.go +++ b/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) }, } diff --git a/docs/configuration/authentication/file.md b/docs/configuration/authentication/file.md index 1e1a9d9a3..be2ccfff5 100644 --- a/docs/configuration/authentication/file.md +++ b/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 diff --git a/docs/configuration/index.md b/docs/configuration/index.md index 849c37736..5f791f2e0 100644 --- a/docs/configuration/index.md +++ b/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 diff --git a/go.mod b/go.mod index da2b5f855..208ce37ab 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index f4afb5333..3682409db 100644 --- a/go.sum +++ b/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= diff --git a/internal/authentication/password_hash.go b/internal/authentication/password_hash.go index b9a70b057..d9b3c435d 100644 --- a/internal/authentication/password_hash.go +++ b/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 diff --git a/internal/authentication/password_hash_test.go b/internal/authentication/password_hash_test.go index 488f93801..3436e9f29 100644 --- a/internal/authentication/password_hash_test.go +++ b/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) { diff --git a/internal/commands/hash.go b/internal/commands/hash.go index a8bfab762..cde188cb0 100644 --- a/internal/commands/hash.go +++ b/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), diff --git a/internal/configuration/validator/authentication.go b/internal/configuration/validator/authentication.go index 42f3d3ab4..ef6eac53c 100644 --- a/internal/configuration/validator/authentication.go +++ b/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 { diff --git a/internal/configuration/validator/authentication_test.go b/internal/configuration/validator/authentication_test.go index 8669dfe4a..a6b91658e 100644 --- a/internal/configuration/validator/authentication_test.go +++ b/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)