From 2ecd62c3ea2860171843145bbd0f633294d33af0 Mon Sep 17 00:00:00 2001 From: David Christofas Date: Fri, 9 Oct 2020 13:55:13 +0200 Subject: [PATCH] change hashing algorithm from SHA-512 to bcrypt Even though SHA-512 is currently considered a secure algorithm it is not the best choice for password hashing. As this change introduces a breaking change it is beast to introduce it as early as possible to prevent us from having to implement a migration strategy Signed-off-by: David Christofas --- accounts/pkg/service/v0/accounts.go | 24 +++++++++---------- accounts/pkg/service/v0/service.go | 12 +++++----- .../unreleased/change-hashing-algorithm.md | 7 ++++++ 3 files changed, 24 insertions(+), 19 deletions(-) create mode 100644 changelog/unreleased/change-hashing-algorithm.md diff --git a/accounts/pkg/service/v0/accounts.go b/accounts/pkg/service/v0/accounts.go index acdd1c855b5..71db4d02d55 100644 --- a/accounts/pkg/service/v0/accounts.go +++ b/accounts/pkg/service/v0/accounts.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "golang.org/x/crypto/bcrypt" "path/filepath" "regexp" "sync" @@ -21,15 +22,12 @@ import ( settings "github.com/owncloud/ocis/settings/pkg/proto/v0" settings_svc "github.com/owncloud/ocis/settings/pkg/service/v0" "github.com/rs/zerolog" - "github.com/tredoe/osutil/user/crypt" "google.golang.org/genproto/protobuf/field_mask" "google.golang.org/protobuf/types/known/timestamppb" +) - // register crypt functions - _ "github.com/tredoe/osutil/user/crypt/apr1_crypt" - _ "github.com/tredoe/osutil/user/crypt/md5_crypt" - _ "github.com/tredoe/osutil/user/crypt/sha256_crypt" - _ "github.com/tredoe/osutil/user/crypt/sha512_crypt" +const ( + _hashDifficulty = 12 ) // accLock mutually exclude readers from writers on account files @@ -81,8 +79,7 @@ func (s Service) passwordIsValid(hash string, pwd string) (ok bool) { } }() - c := crypt.NewFromHash(hash) - return c.Verify(hash, []byte(pwd)) == nil + return bcrypt.CompareHashAndPassword([]byte(hash), []byte(pwd)) == nil } func (s Service) hasAccountManagementPermissions(ctx context.Context) bool { @@ -298,11 +295,12 @@ func (s Service) CreateAccount(ctx context.Context, in *proto.CreateAccountReque if acc.PasswordProfile != nil { if acc.PasswordProfile.Password != "" { // encrypt password - c := crypt.New(crypt.SHA512) - if acc.PasswordProfile.Password, err = c.Generate([]byte(acc.PasswordProfile.Password), nil); err != nil { + hashed, err := bcrypt.GenerateFromPassword([]byte(acc.PasswordProfile.Password), _hashDifficulty) + if err != nil { s.log.Error().Err(err).Str("id", id).Msg("could not hash password") return merrors.InternalServerError(s.id, "could not hash password: %v", err.Error()) } + acc.PasswordProfile.Password = string(hashed) } if err := passwordPoliciesValid(acc.PasswordProfile.PasswordPolicies); err != nil { @@ -406,13 +404,13 @@ func (s Service) UpdateAccount(ctx context.Context, in *proto.UpdateAccountReque } if in.Account.PasswordProfile.Password != "" { // encrypt password - c := crypt.New(crypt.SHA512) - if out.PasswordProfile.Password, err = c.Generate([]byte(in.Account.PasswordProfile.Password), nil); err != nil { + hashed, err := bcrypt.GenerateFromPassword([]byte(in.Account.PasswordProfile.Password), _hashDifficulty) + if err != nil { in.Account.PasswordProfile.Password = "" s.log.Error().Err(err).Str("id", id).Msg("could not hash password") return merrors.InternalServerError(s.id, "could not hash password: %v", err.Error()) } - + out.PasswordProfile.Password = string(hashed) in.Account.PasswordProfile.Password = "" } diff --git a/accounts/pkg/service/v0/service.go b/accounts/pkg/service/v0/service.go index 772210253ec..9b65b7c7601 100644 --- a/accounts/pkg/service/v0/service.go +++ b/accounts/pkg/service/v0/service.go @@ -171,7 +171,7 @@ func (s Service) createDefaultAccounts() (err error) { UidNumber: 20000, GidNumber: 30000, PasswordProfile: &proto.PasswordProfile{ - Password: "$6$rounds=35210$sa1u5Pmfo4cr23Vw$RJNGElaDB1D3xorWkfTEGm2Ko.o2QL3E0cimKx23MNxVWVFSkUUeRoC7FqC4RzYDNQBD6cKzovTEaDD.8TDkD.", + Password: "$2a$12$qGho4QmaDn4HifisABQ2ROBc41TUCIBNrqwOg3zPEYednDWkXHOKG", }, AccountEnabled: true, MemberOf: []*proto.Group{ @@ -190,7 +190,7 @@ func (s Service) createDefaultAccounts() (err error) { UidNumber: 20001, GidNumber: 30000, PasswordProfile: &proto.PasswordProfile{ - Password: "$6$rounds=81434$sa1u5Pmfo4cr23Vw$W78cyL884GmuvDpxYPvSRBVzEj02T5QhTTcI8Dv4IKvMooDFGv4bwaWMkH9HfJ0wgpEBW7Lp.4Cad0xE/MYSg1", + Password: "$2a$12$JzfOtyRiGL25w1UpmiPZ1uU7KJURoTMPSRlZQvMB90/1zqvjwEWgO", }, AccountEnabled: true, MemberOf: []*proto.Group{ @@ -209,7 +209,7 @@ func (s Service) createDefaultAccounts() (err error) { UidNumber: 20002, GidNumber: 30000, PasswordProfile: &proto.PasswordProfile{ - Password: "$6$rounds=5524$sa1u5Pmfo4cr23Vw$58bQVL/JeUlwM0RY21YKAFMvKvwKLLysGllYXox.vwKT5dHMwdzJjCxwTDMnB2o2pwexC8o/iOXyP2zrhALS40", + Password: "$2a$12$bWMHNfc92rDForMapNle/eJ1fOY0eTeRzpk1EQFUdKGI6UdTktp/a", }, AccountEnabled: true, MemberOf: []*proto.Group{ @@ -229,7 +229,7 @@ func (s Service) createDefaultAccounts() (err error) { UidNumber: 20003, GidNumber: 30000, PasswordProfile: &proto.PasswordProfile{ - Password: "$6$rounds=47068$lhw6odzXW0LTk/ao$GgxS.pIgP8jawLJBAiyNor2FrWzrULF95PwspRkli2W3VF.4HEwTYlQfRXbNQBMjNCEcEYlgZo3a.kRz2k2N0/", + Password: "$2a$12$TY7jDd1PNbZXzZJMvIFm3eXAL4wjzOl.QXJZ6GKGDpAmUcvHBNc66", }, AccountEnabled: true, MemberOf: []*proto.Group{ @@ -246,7 +246,7 @@ func (s Service) createDefaultAccounts() (err error) { UidNumber: 10000, GidNumber: 15000, PasswordProfile: &proto.PasswordProfile{ - Password: "$6$rounds=9746$sa1u5Pmfo4cr23Vw$2hnwpkTvUkWX0v6mh8Aw1pbzEXa9EUJzmrey4g2W/8arwWCwhteqU//3aWnA3S0d5T21fOKYteoqlsN1IbTcN.", + Password: "$2a$12$pV8JlGlpM9oo1RaC7kSg/uBTFoTOJ8XfjibFL2U4gOpPKVfbD6pUG", }, AccountEnabled: true, MemberOf: []*proto.Group{ @@ -262,7 +262,7 @@ func (s Service) createDefaultAccounts() (err error) { UidNumber: 10001, GidNumber: 15000, PasswordProfile: &proto.PasswordProfile{ - Password: "$6$rounds=91087$sa1u5Pmfo4cr23Vw$wPC3BbMTbP/ytlo0p.f99zJifyO70AUCdKIK9hkhwutBKGCirLmZs/MsWAG6xHjVvmnmHN5NoON7FUGv5pPaN.", + Password: "$2a$12$CSM.vkX9o7lO/uvid3XieOVtmq5nh91MFZHvHIsfRms3hLzTa2W6.", }, AccountEnabled: true, MemberOf: []*proto.Group{ diff --git a/changelog/unreleased/change-hashing-algorithm.md b/changelog/unreleased/change-hashing-algorithm.md new file mode 100644 index 00000000000..b9a0f8ffef6 --- /dev/null +++ b/changelog/unreleased/change-hashing-algorithm.md @@ -0,0 +1,7 @@ +Change: Use bcrypt to hash the user passwords + +Change the hashing algorithm from SHA-512 to bcrypt since the latter is better suitable for password hashing. +This is a breaking change. Existing deployments need to regenerate the accounts folder. + + +https://github.com/owncloud/ocis/issues/510