From ea5c27fcd5964560df67a855a93046f485bf2cf2 Mon Sep 17 00:00:00 2001 From: DQ Date: Thu, 5 Sep 2019 14:31:26 +0800 Subject: [PATCH] Enhance: Upgrade encrypt alg to sha256 previous sha1 will still used for old password Signed-off-by: DQ --- .../postgresql/0010_1.9.0_schema.up.sql | 4 +- src/common/dao/dao_test.go | 14 +++++- src/common/dao/register.go | 6 +-- src/common/dao/user.go | 48 ++++++++++++------- src/common/models/user.go | 17 +++---- src/common/utils/encrypt.go | 22 +++++++-- src/common/utils/test/database.go | 1 - src/common/utils/utils_test.go | 20 ++++++-- src/core/api/user.go | 16 ++++--- src/core/controllers/base.go | 10 ++-- 10 files changed, 105 insertions(+), 53 deletions(-) diff --git a/make/migrations/postgresql/0010_1.9.0_schema.up.sql b/make/migrations/postgresql/0010_1.9.0_schema.up.sql index 5ca2c39f2..709f529cf 100644 --- a/make/migrations/postgresql/0010_1.9.0_schema.up.sql +++ b/make/migrations/postgresql/0010_1.9.0_schema.up.sql @@ -185,4 +185,6 @@ create table notification_policy ( ALTER TABLE replication_task ADD COLUMN status_revision int DEFAULT 0; DELETE FROM project_metadata WHERE deleted = TRUE; -ALTER TABLE project_metadata DROP COLUMN deleted; \ No newline at end of file +ALTER TABLE project_metadata DROP COLUMN deleted; +ALTER TABLE harbor_user ADD COLUMN password_version varchar(16) Default 'sha256'; +UPDATE harbor_user SET password_version = 'sha1'; diff --git a/src/common/dao/dao_test.go b/src/common/dao/dao_test.go index bc070245a..1725fdab4 100644 --- a/src/common/dao/dao_test.go +++ b/src/common/dao/dao_test.go @@ -324,7 +324,12 @@ func TestResetUserPassword(t *testing.T) { t.Errorf("Error occurred in UpdateUserResetUuid: %v", err) } - err = ResetUserPassword(models.User{UserID: currentUser.UserID, Password: "HarborTester12345", ResetUUID: uuid, Salt: currentUser.Salt}) + err = ResetUserPassword( + models.User{ + UserID: currentUser.UserID, + PasswordVersion: utils.SHA256, + ResetUUID: uuid, + Salt: currentUser.Salt}, "HarborTester12345") if err != nil { t.Errorf("Error occurred in ResetUserPassword: %v", err) } @@ -346,7 +351,12 @@ func TestChangeUserPassword(t *testing.T) { t.Errorf("Error occurred when get user salt") } currentUser.Salt = query.Salt - err = ChangeUserPassword(models.User{UserID: currentUser.UserID, Password: "NewHarborTester12345", Salt: currentUser.Salt}) + err = ChangeUserPassword( + models.User{ + UserID: currentUser.UserID, + Password: "NewHarborTester12345", + PasswordVersion: utils.SHA256, + Salt: currentUser.Salt}) if err != nil { t.Errorf("Error occurred in ChangeUserPassword: %v", err) } diff --git a/src/common/dao/register.go b/src/common/dao/register.go index 7f3062153..fa6a8ac91 100644 --- a/src/common/dao/register.go +++ b/src/common/dao/register.go @@ -29,10 +29,10 @@ func Register(user models.User) (int64, error) { now := time.Now() salt := utils.GenerateRandomString() sql := `insert into harbor_user - (username, password, realname, email, comment, salt, sysadmin_flag, creation_time, update_time) - values (?, ?, ?, ?, ?, ?, ?, ?, ?) RETURNING user_id` + (username, password, password_version, realname, email, comment, salt, sysadmin_flag, creation_time, update_time) + values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) RETURNING user_id` var userID int64 - err := o.Raw(sql, user.Username, utils.Encrypt(user.Password, salt), user.Realname, user.Email, + err := o.Raw(sql, user.Username, utils.Encrypt(user.Password, salt, utils.SHA256), utils.SHA256, user.Realname, user.Email, user.Comment, salt, user.HasAdminRole, now, now).QueryRow(&userID) if err != nil { return 0, err diff --git a/src/common/dao/user.go b/src/common/dao/user.go index 535887b1e..0417b44ab 100644 --- a/src/common/dao/user.go +++ b/src/common/dao/user.go @@ -23,7 +23,6 @@ import ( "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/utils" - "github.com/goharbor/harbor/src/common/utils/log" ) @@ -32,7 +31,7 @@ func GetUser(query models.User) (*models.User, error) { o := GetOrmer() - sql := `select user_id, username, password, email, realname, comment, reset_uuid, salt, + sql := `select user_id, username, password, password_version, email, realname, comment, reset_uuid, salt, sysadmin_flag, creation_time, update_time from harbor_user u where deleted = false ` @@ -76,9 +75,9 @@ func GetUser(query models.User) (*models.User, error) { // LoginByDb is used for user to login with database auth mode. func LoginByDb(auth models.AuthModel) (*models.User, error) { + var users []models.User o := GetOrmer() - var users []models.User n, err := o.Raw(`select * from harbor_user where (username = ? or email = ?) and deleted = false`, auth.Principal, auth.Principal).QueryRows(&users) if err != nil { @@ -90,12 +89,10 @@ func LoginByDb(auth models.AuthModel) (*models.User, error) { user := users[0] - if user.Password != utils.Encrypt(auth.Password, user.Salt) { + if !matchPassword(&user, auth.Password) { return nil, nil } - user.Password = "" // do not return the password - return &user, nil } @@ -165,23 +162,34 @@ func ToggleUserAdminRole(userID int, hasAdmin bool) error { func ChangeUserPassword(u models.User) error { u.UpdateTime = time.Now() u.Salt = utils.GenerateRandomString() - u.Password = utils.Encrypt(u.Password, u.Salt) - _, err := GetOrmer().Update(&u, "Password", "Salt", "UpdateTime") + u.Password = utils.Encrypt(u.Password, u.Salt, utils.SHA256) + var err error + if u.PasswordVersion == utils.SHA1 { + u.PasswordVersion = utils.SHA256 + _, err = GetOrmer().Update(&u, "Password", "PasswordVersion", "Salt", "UpdateTime") + } else { + _, err = GetOrmer().Update(&u, "Password", "Salt", "UpdateTime") + } return err } // ResetUserPassword ... -func ResetUserPassword(u models.User) error { - o := GetOrmer() - r, err := o.Raw(`update harbor_user set password=?, reset_uuid=? where reset_uuid=?`, utils.Encrypt(u.Password, u.Salt), "", u.ResetUUID).Exec() +func ResetUserPassword(u models.User, rawPassword string) error { + var rowsAffected int64 + var err error + u.UpdateTime = time.Now() + u.Password = utils.Encrypt(rawPassword, u.Salt, utils.SHA256) + u.ResetUUID = "" + if u.PasswordVersion == utils.SHA1 { + u.PasswordVersion = utils.SHA256 + rowsAffected, err = GetOrmer().Update(&u, "Password", "PasswordVersion", "ResetUUID", "UpdateTime") + } else { + rowsAffected, err = GetOrmer().Update(&u, "Password", "ResetUUID", "UpdateTime") + } if err != nil { return err } - count, err := r.RowsAffected() - if err != nil { - return err - } - if count == 0 { + if rowsAffected == 0 { return errors.New("no record be changed, reset password failed") } return nil @@ -282,3 +290,11 @@ func CleanUser(id int64) error { } return nil } + +// MatchPassword returns true is password matched +func matchPassword(u *models.User, password string) bool { + if u.Password != utils.Encrypt(password, u.Salt, u.PasswordVersion) { + return false + } + return true +} diff --git a/src/common/models/user.go b/src/common/models/user.go index c4299869f..c47ad03b8 100644 --- a/src/common/models/user.go +++ b/src/common/models/user.go @@ -23,14 +23,15 @@ const UserTable = "harbor_user" // User holds the details of a user. type User struct { - UserID int `orm:"pk;auto;column(user_id)" json:"user_id"` - Username string `orm:"column(username)" json:"username"` - Email string `orm:"column(email)" json:"email"` - Password string `orm:"column(password)" json:"password"` - Realname string `orm:"column(realname)" json:"realname"` - Comment string `orm:"column(comment)" json:"comment"` - Deleted bool `orm:"column(deleted)" json:"deleted"` - Rolename string `orm:"-" json:"role_name"` + UserID int `orm:"pk;auto;column(user_id)" json:"user_id"` + Username string `orm:"column(username)" json:"username"` + Email string `orm:"column(email)" json:"email"` + Password string `orm:"column(password)" json:"password"` + PasswordVersion string `orm:"column(password_version)" json:"password_version"` + Realname string `orm:"column(realname)" json:"realname"` + Comment string `orm:"column(comment)" json:"comment"` + Deleted bool `orm:"column(deleted)" json:"deleted"` + Rolename string `orm:"-" json:"role_name"` // if this field is named as "RoleID", beego orm can not map role_id // to it. Role int `orm:"-" json:"role_id"` diff --git a/src/common/utils/encrypt.go b/src/common/utils/encrypt.go index 473880843..e68da9430 100644 --- a/src/common/utils/encrypt.go +++ b/src/common/utils/encrypt.go @@ -19,25 +19,37 @@ import ( "crypto/cipher" "crypto/rand" "crypto/sha1" + "crypto/sha256" "encoding/base64" "errors" "fmt" + "hash" "io" "strings" "golang.org/x/crypto/pbkdf2" ) -// Encrypt encrypts the content with salt -func Encrypt(content string, salt string) string { - return fmt.Sprintf("%x", pbkdf2.Key([]byte(content), []byte(salt), 4096, 16, sha1.New)) -} - const ( // EncryptHeaderV1 ... EncryptHeaderV1 = "" + // SHA1 is the name of sha1 hash alg + SHA1 = "sha1" + // SHA256 is the name of sha256 hash alg + SHA256 = "sha256" ) +// HashAlg used to get correct alg for hash +var HashAlg = map[string]func() hash.Hash{ + SHA1: sha1.New, + SHA256: sha256.New, +} + +// Encrypt encrypts the content with salt +func Encrypt(content string, salt string, encrptAlg string) string { + return fmt.Sprintf("%x", pbkdf2.Key([]byte(content), []byte(salt), 4096, 16, HashAlg[encrptAlg])) +} + // ReversibleEncrypt encrypts the str with aes/base64 func ReversibleEncrypt(str, key string) (string, error) { keyBytes := []byte(key) diff --git a/src/common/utils/test/database.go b/src/common/utils/test/database.go index 560c950b7..970109b51 100644 --- a/src/common/utils/test/database.go +++ b/src/common/utils/test/database.go @@ -89,7 +89,6 @@ func updateUserInitialPassword(userID int, password string) error { if err != nil { return fmt.Errorf("Failed to update user encrypted password, userID: %d, err: %v", userID, err) } - } else { } return nil } diff --git a/src/common/utils/utils_test.go b/src/common/utils/utils_test.go index 437f16152..b81da95ed 100644 --- a/src/common/utils/utils_test.go +++ b/src/common/utils/utils_test.go @@ -17,6 +17,7 @@ package utils import ( "encoding/base64" "net/http/httptest" + "reflect" "strconv" "strings" "testing" @@ -91,12 +92,21 @@ func TestParseRepository(t *testing.T) { } func TestEncrypt(t *testing.T) { - content := "content" - salt := "salt" - result := Encrypt(content, salt) + tests := map[string]struct { + content string + salt string + alg string + want string + }{ + "sha1 test": {content: "content", salt: "salt", alg: SHA1, want: "dc79e76c88415c97eb089d9cc80b4ab0"}, + "sha256 test": {content: "content", salt: "salt", alg: SHA256, want: "83d3d6f3e7cacb040423adf7ced63d21"}, + } - if result != "dc79e76c88415c97eb089d9cc80b4ab0" { - t.Errorf("unexpected result: %s != %s", result, "dc79e76c88415c97eb089d9cc80b4ab0") + for name, tc := range tests { + got := Encrypt(tc.content, tc.salt, tc.alg) + if !reflect.DeepEqual(tc.want, got) { + t.Errorf("%s: expected: %v, got: %v", name, tc.want, got) + } } } diff --git a/src/core/api/user.go b/src/core/api/user.go index a58095983..eef99a999 100644 --- a/src/core/api/user.go +++ b/src/core/api/user.go @@ -17,6 +17,10 @@ package api import ( "errors" "fmt" + "net/http" + "regexp" + "strconv" + "github.com/goharbor/harbor/src/common" "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" @@ -25,9 +29,6 @@ import ( "github.com/goharbor/harbor/src/common/utils" "github.com/goharbor/harbor/src/common/utils/log" "github.com/goharbor/harbor/src/core/config" - "net/http" - "regexp" - "strconv" ) // UserAPI handles request to /api/users/{} @@ -416,20 +417,21 @@ func (ua *UserAPI) ChangePassword() { return } if changePwdOfOwn { - if user.Password != utils.Encrypt(req.OldPassword, user.Salt) { + if user.Password != utils.Encrypt(req.OldPassword, user.Salt, user.PasswordVersion) { log.Info("incorrect old_password") ua.SendForbiddenError(errors.New("incorrect old_password")) return } } - if user.Password == utils.Encrypt(req.NewPassword, user.Salt) { + if user.Password == utils.Encrypt(req.NewPassword, user.Salt, user.PasswordVersion) { ua.SendBadRequestError(errors.New("the new password can not be same with the old one")) return } updatedUser := models.User{ - UserID: ua.userID, - Password: req.NewPassword, + UserID: ua.userID, + Password: req.NewPassword, + PasswordVersion: user.PasswordVersion, } if err = dao.ChangeUserPassword(updatedUser); err != nil { ua.SendInternalServerError(fmt.Errorf("failed to change password of user %d: %v", ua.userID, err)) diff --git a/src/core/controllers/base.go b/src/core/controllers/base.go index 9dd0f18a2..301dba41f 100644 --- a/src/core/controllers/base.go +++ b/src/core/controllers/base.go @@ -17,7 +17,6 @@ package controllers import ( "bytes" "context" - "github.com/goharbor/harbor/src/core/filter" "html/template" "net" "net/http" @@ -26,6 +25,8 @@ import ( "strconv" "strings" + "github.com/goharbor/harbor/src/core/filter" + "github.com/astaxie/beego" "github.com/beego/i18n" "github.com/goharbor/harbor/src/common" @@ -252,11 +253,10 @@ func (cc *CommonController) ResetPassword() { cc.CustomAbort(http.StatusForbidden, http.StatusText(http.StatusForbidden)) } - password := cc.GetString("password") + rawPassword := cc.GetString("password") - if password != "" { - user.Password = password - err = dao.ResetUserPassword(*user) + if rawPassword != "" { + err = dao.ResetUserPassword(*user, rawPassword) if err != nil { log.Errorf("Error occurred in ResetUserPassword: %v", err) cc.CustomAbort(http.StatusInternalServerError, "Internal error.")