commit f13c08e9f45d7776cb264b17ec41bc4ff51fc0b9 Author: Andreas Gerstmayr Date: Thu Nov 25 18:49:52 2021 +0100 notifications: use HMAC-SHA256 to generate time limit codes * changes the time limit code generation function to use HMAC-SHA256 instead of SHA-1 * multiple new testcases diff --git a/pkg/services/notifications/codes.go b/pkg/services/notifications/codes.go index ea9beb30cc..1ddf05dc69 100644 --- a/pkg/services/notifications/codes.go +++ b/pkg/services/notifications/codes.go @@ -1,48 +1,53 @@ package notifications import ( - "crypto/sha1" // #nosec + "crypto/hmac" + "crypto/sha256" "encoding/hex" "fmt" + "strconv" "time" - "github.com/unknwon/com" - "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" ) -const timeLimitCodeLength = 12 + 6 + 40 +const timeLimitStartDateLength = 12 +const timeLimitMinutesLength = 6 +const timeLimitHmacLength = 64 +const timeLimitCodeLength = timeLimitStartDateLength + timeLimitMinutesLength + timeLimitHmacLength // create a time limit code -// code format: 12 length date time string + 6 minutes string + 40 sha1 encoded string -func createTimeLimitCode(data string, minutes int, startInf interface{}) (string, error) { +// code format: 12 length date time string + 6 minutes string + 64 HMAC-SHA256 encoded string +func createTimeLimitCode(payload string, minutes int, startStr string) (string, error) { format := "200601021504" var start, end time.Time - var startStr, endStr string + var endStr string - if startInf == nil { + if startStr == "" { // Use now time create code start = time.Now() startStr = start.Format(format) } else { // use start string create code - startStr = startInf.(string) - start, _ = time.ParseInLocation(format, startStr, time.Local) - startStr = start.Format(format) + var err error + start, err = time.ParseInLocation(format, startStr, time.Local) + if err != nil { + return "", err + } } end = start.Add(time.Minute * time.Duration(minutes)) endStr = end.Format(format) - // create sha1 encode string - sh := sha1.New() - if _, err := sh.Write([]byte(data + setting.SecretKey + startStr + endStr + - com.ToStr(minutes))); err != nil { - return "", err + // create HMAC-SHA256 encoded string + key := []byte(setting.SecretKey) + h := hmac.New(sha256.New, key) + if _, err := h.Write([]byte(payload + startStr + endStr)); err != nil { + return "", fmt.Errorf("cannot create hmac: %v", err) } - encoded := hex.EncodeToString(sh.Sum(nil)) + encoded := hex.EncodeToString(h.Sum(nil)) code := fmt.Sprintf("%s%06d%s", startStr, minutes, encoded) return code, nil @@ -50,30 +55,29 @@ func createTimeLimitCode(data string, minutes int, startInf interface{}) (string // verify time limit code func validateUserEmailCode(user *models.User, code string) (bool, error) { - if len(code) <= 18 { + if len(code) < timeLimitCodeLength { return false, nil } - minutes := setting.EmailCodeValidMinutes code = code[:timeLimitCodeLength] // split code - start := code[:12] - lives := code[12:18] - if d, err := com.StrTo(lives).Int(); err == nil { - minutes = d + startStr := code[:timeLimitStartDateLength] + minutesStr := code[timeLimitStartDateLength : timeLimitStartDateLength+timeLimitMinutesLength] + minutes, err := strconv.Atoi(minutesStr) + if err != nil { + return false, fmt.Errorf("invalid time limit code: %v", err) } - // right active code - data := com.ToStr(user.Id) + user.Email + user.Login + user.Password + user.Rands - retCode, err := createTimeLimitCode(data, minutes, start) + // verify code + payload := strconv.FormatInt(user.Id, 10) + user.Email + user.Login + user.Password + user.Rands + expectedCode, err := createTimeLimitCode(payload, minutes, startStr) if err != nil { return false, err } - fmt.Printf("code : %s\ncode2: %s", retCode, code) - if retCode == code && minutes > 0 { + if hmac.Equal([]byte(code), []byte(expectedCode)) && minutes > 0 { // check time is expired or not - before, _ := time.ParseInLocation("200601021504", start, time.Local) + before, _ := time.ParseInLocation("200601021504", startStr, time.Local) now := time.Now() if before.Add(time.Minute*time.Duration(minutes)).Unix() > now.Unix() { return true, nil @@ -94,15 +98,15 @@ func getLoginForEmailCode(code string) string { return string(b) } -func createUserEmailCode(u *models.User, startInf interface{}) (string, error) { +func createUserEmailCode(user *models.User, startStr string) (string, error) { minutes := setting.EmailCodeValidMinutes - data := com.ToStr(u.Id) + u.Email + u.Login + u.Password + u.Rands - code, err := createTimeLimitCode(data, minutes, startInf) + payload := strconv.FormatInt(user.Id, 10) + user.Email + user.Login + user.Password + user.Rands + code, err := createTimeLimitCode(payload, minutes, startStr) if err != nil { return "", err } // add tail hex username - code += hex.EncodeToString([]byte(u.Login)) + code += hex.EncodeToString([]byte(user.Login)) return code, nil } diff --git a/pkg/services/notifications/codes_test.go b/pkg/services/notifications/codes_test.go index d2b1f3a617..bea88e0bf5 100644 --- a/pkg/services/notifications/codes_test.go +++ b/pkg/services/notifications/codes_test.go @@ -1,19 +1,129 @@ package notifications import ( + "fmt" + "strconv" "testing" + "time" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" . "github.com/smartystreets/goconvey/convey" + "github.com/stretchr/testify/require" ) +func TestTimeLimitCodes(t *testing.T) { + user := &models.User{Id: 10, Email: "t@a.com", Login: "asd", Password: "1", Rands: "2"} + + format := "200601021504" + mailPayload := strconv.FormatInt(user.Id, 10) + user.Email + user.Login + user.Password + user.Rands + tenMinutesAgo := time.Now().Add(-time.Minute * 10) + + tests := []struct { + desc string + payload string + start time.Time + minutes int + valid bool + }{ + { + desc: "code generated 10 minutes ago, 5 minutes valid", + payload: mailPayload, + start: tenMinutesAgo, + minutes: 5, + valid: false, + }, + { + desc: "code generated 10 minutes ago, 9 minutes valid", + payload: mailPayload, + start: tenMinutesAgo, + minutes: 9, + valid: false, + }, + { + desc: "code generated 10 minutes ago, 10 minutes valid", + payload: mailPayload, + start: tenMinutesAgo, + minutes: 10, + // code was valid exactly 10 minutes since evaluating the tenMinutesAgo assignment + // by the time this test is run the code is already expired + valid: false, + }, + { + desc: "code generated 10 minutes ago, 11 minutes valid", + payload: mailPayload, + start: tenMinutesAgo, + minutes: 11, + valid: true, + }, + { + desc: "code generated 10 minutes ago, 20 minutes valid", + payload: mailPayload, + start: tenMinutesAgo, + minutes: 20, + valid: true, + }, + { + desc: "code generated 10 minutes ago, 20 minutes valid, tampered payload", + payload: mailPayload[:len(mailPayload)-1] + "x", + start: tenMinutesAgo, + minutes: 20, + valid: false, + }, + } + + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + code, err := createTimeLimitCode(test.payload, test.minutes, test.start.Format(format)) + require.NoError(t, err) + + isValid, err := validateUserEmailCode(user, code) + require.NoError(t, err) + require.Equal(t, test.valid, isValid) + }) + } + + t.Run("tampered minutes", func(t *testing.T) { + code, err := createTimeLimitCode(mailPayload, 5, tenMinutesAgo.Format(format)) + require.NoError(t, err) + + // code is expired + isValid, err := validateUserEmailCode(user, code) + require.NoError(t, err) + require.Equal(t, false, isValid) + + // let's try to extend the code by tampering the minutes + code = code[:12] + fmt.Sprintf("%06d", 20) + code[18:] + isValid, err = validateUserEmailCode(user, code) + require.NoError(t, err) + require.Equal(t, false, isValid) + }) + + t.Run("tampered start string", func(t *testing.T) { + code, err := createTimeLimitCode(mailPayload, 5, tenMinutesAgo.Format(format)) + require.NoError(t, err) + + // code is expired + isValid, err := validateUserEmailCode(user, code) + require.NoError(t, err) + require.Equal(t, false, isValid) + + // let's try to extend the code by tampering the start string + oneMinuteAgo := time.Now().Add(-time.Minute) + + code = oneMinuteAgo.Format(format) + code[12:] + isValid, err = validateUserEmailCode(user, code) + require.NoError(t, err) + require.Equal(t, false, isValid) + }) +} + func TestEmailCodes(t *testing.T) { Convey("When generating code", t, func() { setting.EmailCodeValidMinutes = 120 user := &models.User{Id: 10, Email: "t@a.com", Login: "asd", Password: "1", Rands: "2"} - code, err := createUserEmailCode(user, nil) + code, err := createUserEmailCode(user, "") So(err, ShouldBeNil) Convey("getLoginForCode should return login", func() { @@ -27,7 +137,7 @@ func TestEmailCodes(t *testing.T) { So(isValid, ShouldBeTrue) }) - Convey("Cannot verify in-valid code", func() { + Convey("Cannot verify invalid code", func() { code = "ASD" isValid, err := validateUserEmailCode(user, code) So(err, ShouldBeNil) diff --git a/pkg/services/notifications/notifications.go b/pkg/services/notifications/notifications.go index beea82f43e..5a575d1415 100644 --- a/pkg/services/notifications/notifications.go +++ b/pkg/services/notifications/notifications.go @@ -149,7 +149,7 @@ func (ns *NotificationService) sendEmailCommandHandler(cmd *models.SendEmailComm } func (ns *NotificationService) sendResetPasswordEmail(cmd *models.SendResetPasswordEmailCommand) error { - code, err := createUserEmailCode(cmd.User, nil) + code, err := createUserEmailCode(cmd.User, "") if err != nil { return err } diff --git a/pkg/services/notifications/notifications_test.go b/pkg/services/notifications/notifications_test.go index e7680c3943..fb73e332ea 100644 --- a/pkg/services/notifications/notifications_test.go +++ b/pkg/services/notifications/notifications_test.go @@ -1,12 +1,14 @@ package notifications import ( + "regexp" "testing" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" . "github.com/smartystreets/goconvey/convey" + "github.com/stretchr/testify/require" ) func TestNotifications(t *testing.T) { @@ -25,13 +27,28 @@ func TestNotifications(t *testing.T) { So(err, ShouldBeNil) Convey("When sending reset email password", func() { - err := ns.sendResetPasswordEmail(&models.SendResetPasswordEmailCommand{User: &models.User{Email: "asd@asd.com"}}) + user := models.User{Email: "asd@asd.com", Login: "asd@asd.com"} + err := ns.sendResetPasswordEmail(&models.SendResetPasswordEmailCommand{User: &user}) So(err, ShouldBeNil) sentMsg := <-ns.mailQueue So(sentMsg.Body, ShouldContainSubstring, "body") So(sentMsg.Subject, ShouldEqual, "Reset your Grafana password - asd@asd.com") So(sentMsg.Body, ShouldNotContainSubstring, "Subject") + + // find code in mail + r, _ := regexp.Compile(`code=(\w+)`) + match := r.FindString(sentMsg.Body) + code := match[len("code="):] + + // verify code + bus.AddHandler("test", func(query *models.GetUserByLoginQuery) error { + query.Result = &user + return nil + }) + query := models.ValidateResetPasswordCodeQuery{Code: code} + err = ns.validateResetPasswordCode(&query) + require.NoError(t, err) }) }) }