From 1ee442e86f76ed368f96fcdcbded9bd724bab579 Mon Sep 17 00:00:00 2001 From: Clement Michaud Date: Fri, 27 Dec 2019 17:55:00 +0100 Subject: [PATCH] Improve logs of password hashing to help troubleshoot issues. --- internal/authentication/file_user_provider.go | 3 +- .../authentication/file_user_provider_test.go | 23 +++++++++++ internal/authentication/ldap_user_provider.go | 6 +-- internal/authentication/password_hash.go | 21 +++++----- internal/authentication/password_hash_test.go | 38 +++++++++++++++++++ 5 files changed, 77 insertions(+), 14 deletions(-) diff --git a/internal/authentication/file_user_provider.go b/internal/authentication/file_user_provider.go index 9882eee71..e8511fe32 100644 --- a/internal/authentication/file_user_provider.go +++ b/internal/authentication/file_user_provider.go @@ -3,6 +3,7 @@ package authentication import ( "fmt" "io/ioutil" + "strings" "sync" "github.com/asaskevich/govalidator" @@ -68,7 +69,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 := details.HashedPassword[7:] // Remove {CRYPT} + hashedPassword := strings.ReplaceAll(details.HashedPassword, "{CRYPT}", "") ok, err := CheckPassword(password, hashedPassword) if err != nil { return false, err diff --git a/internal/authentication/file_user_provider_test.go b/internal/authentication/file_user_provider_test.go index 01aea83ea..bb07fc4bf 100644 --- a/internal/authentication/file_user_provider_test.go +++ b/internal/authentication/file_user_provider_test.go @@ -98,6 +98,16 @@ func TestShouldRaiseWhenLoadingDatabaseWithBadSchemaForFirstTime(t *testing.T) { }) } +func TestShouldSupportHashPasswordWithoutCRYPT(t *testing.T) { + WithDatabase(UserDatabaseWithouCryptContent, func(path string) { + provider := NewFileUserProvider(path) + ok, err := provider.CheckUserPassword("john", "password") + + assert.NoError(t, err) + assert.True(t, ok) + }) +} + var UserDatabaseContent = []byte(` users: john: @@ -142,3 +152,16 @@ user: - admins - dev `) + +var UserDatabaseWithouCryptContent = []byte(` +users: + john: + password: "$6$rounds=500000$jgiCMRyGXzoqpxS3$w2pJeZnnH8bwW3zzvoMWtTRfQYsHbWbD/hquuQ5vUeIyl9gdwBIt6RWk2S6afBA0DPakbeWgD/4SZPiS0hYtU/" + email: john.doe@authelia.com + groups: + - admins + - dev + james: + password: "$6$rounds=500000$jgiCMRyGXzoqpxS3$w2pJeZnnH8bwW3zzvoMWtTRfQYsHbWbD/hquuQ5vUeIyl9gdwBIt6RWk2S6afBA0DPakbeWgD/4SZPiS0hYtU/" + email: james.dean@authelia.com +`) diff --git a/internal/authentication/ldap_user_provider.go b/internal/authentication/ldap_user_provider.go index 3039546b1..4e44162d7 100644 --- a/internal/authentication/ldap_user_provider.go +++ b/internal/authentication/ldap_user_provider.go @@ -153,15 +153,15 @@ func (p *LDAPUserProvider) getUserUID(conn LDAPConnection, username string) (str } func (p *LDAPUserProvider) createGroupsFilter(conn LDAPConnection, username string) (string, error) { - if strings.Index(p.configuration.GroupsFilter, "{0}") >= 0 { + if strings.Contains(p.configuration.GroupsFilter, "{0}") { return strings.Replace(p.configuration.GroupsFilter, "{0}", username, -1), nil - } else if strings.Index(p.configuration.GroupsFilter, "{dn}") >= 0 { + } else if strings.Contains(p.configuration.GroupsFilter, "{dn}") { userDN, err := p.getUserDN(conn, username) if err != nil { return "", err } return strings.Replace(p.configuration.GroupsFilter, "{dn}", userDN, -1), nil - } else if strings.Index(p.configuration.GroupsFilter, "{uid}") >= 0 { + } else if strings.Contains(p.configuration.GroupsFilter, "{uid}") { userUID, err := p.getUserUID(conn, username) if err != nil { return "", err diff --git a/internal/authentication/password_hash.go b/internal/authentication/password_hash.go index 346ac9641..4c3fc6824 100644 --- a/internal/authentication/password_hash.go +++ b/internal/authentication/password_hash.go @@ -22,26 +22,27 @@ type PasswordHash struct { Hash string } -// passwordHashFromString extracts all characteristics of a hash given its string representation. -func passwordHashFromString(hash string) (*PasswordHash, error) { - // Only supports salted sha 512. - if hash[:3] != "$6$" { - return nil, errors.New("Authelia only supports salted SHA512 hashing") - } +// ParseHash extracts all characteristics of a hash given its string representation. +func ParseHash(hash string) (*PasswordHash, error) { parts := strings.Split(hash, "$") if len(parts) != 5 { - return nil, errors.New("Cannot parse the hash") + return nil, fmt.Errorf("Cannot parse the hash %s", hash) + } + + // Only supports salted sha 512. + if parts[1] != "6" { + return nil, fmt.Errorf("Authelia only supports salted SHA512 hashing ($6$), not $%s$", parts[1]) } roundsKV := strings.Split(parts[2], "=") if len(roundsKV) != 2 { - return nil, errors.New("Cannot find the number of rounds") + return nil, errors.New("Cannot match pattern 'rounds=' to find the number of rounds") } rounds, err := strconv.ParseInt(roundsKV[1], 10, 0) if err != nil { - return nil, fmt.Errorf("Cannot find the number of rounds in the hash: %s", err.Error()) + return nil, fmt.Errorf("Cannot find the number of rounds from %s using pattern 'rounds='. Cause: %s", roundsKV[1], err.Error()) } return &PasswordHash{ @@ -78,7 +79,7 @@ func HashPassword(password string, salt string) string { // CheckPassword check a password against a hash. func CheckPassword(password string, hash string) (bool, error) { - passwordHash, err := passwordHashFromString(hash) + passwordHash, err := ParseHash(hash) if err != nil { return false, err } diff --git a/internal/authentication/password_hash_test.go b/internal/authentication/password_hash_test.go index 2da589f34..422b9c25a 100644 --- a/internal/authentication/password_hash_test.go +++ b/internal/authentication/password_hash_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestShouldHashPassword(t *testing.T) { @@ -17,3 +18,40 @@ func TestShouldCheckPassword(t *testing.T) { assert.NoError(t, err) assert.True(t, ok) } + +func TestCannotParseHash(t *testing.T) { + ok, err := CheckPassword("password", "$6$roSnSL3fEVkK0yHFQ.oFFAd8D4OhPAy18K5U61Z2eBhxQXExGU/eknXlY1") + + assert.EqualError(t, err, "Cannot parse the hash $6$roSnSL3fEVkK0yHFQ.oFFAd8D4OhPAy18K5U61Z2eBhxQXExGU/eknXlY1") + assert.False(t, ok) +} + +func TestOnlySupportSHA512(t *testing.T) { + ok, err := CheckPassword("password", "$8$rounds=50000$aFr56HjK3DrB8t3S$zhPQiS85cgBlNhUKKE6n/AHMlpqrvYSnSL3fEVkK0yHFQ.oFFAd8D4OhPAy18K5U61Z2eBhxQXExGU/eknXlY1") + + assert.EqualError(t, err, "Authelia only supports salted SHA512 hashing ($6$), not $8$") + assert.False(t, ok) +} + +func TestCannotFindNumberOfRounds(t *testing.T) { + ok, err := CheckPassword("password", "$6$rounds50000$aFr56HjK3DrB8t3S$zhPQiS85cgBlNhUKKE6n/AHMlpqrvYSnSL3fEVkK0yHFQ.oFFAd8D4OhPAy18K5U61Z2eBhxQXExGU/eknXlY1") + + assert.EqualError(t, err, "Cannot match pattern 'rounds=' to find the number of rounds") + assert.False(t, ok) +} + +func TestNumberOfRoundsNotInt(t *testing.T) { + ok, err := CheckPassword("password", "$6$rounds=abc$aFr56HjK3DrB8t3S$zhPQiS85cgBlNhUKKE6n/AHMlpqrvYSnSL3fEVkK0yHFQ.oFFAd8D4OhPAy18K5U61Z2eBhxQXExGU/eknXlY1") + + assert.EqualError(t, err, "Cannot find the number of rounds from abc using pattern 'rounds='. Cause: strconv.ParseInt: parsing \"abc\": invalid syntax") + assert.False(t, ok) +} + +func TestShouldCheckPasswordHashedWithAuthelia(t *testing.T) { + password := "my;secure*password" + hash := HashPassword(password, "") + equal, err := CheckPassword(password, hash) + + require.NoError(t, err) + assert.True(t, equal) +}