-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Switch plaintext scratch tokens to use hash instead #4331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 20 commits
2fca6d9
050c96f
412e27b
fd4445f
e46e0bc
d9d8e0c
fa2b6e5
5cb253a
7caff96
25ca551
c703bd5
b66fb81
e2e1636
66528b9
5cbf3b8
d499fc8
6165857
dc55b92
8ff84a4
a95cf37
e07acd1
c001b14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
// Copyright 2018 The Gitea Authors. All rights reserved. | ||
// Use of this source code is governed by a MIT-style | ||
// license that can be found in the LICENSE file. | ||
|
||
package migrations | ||
|
||
import ( | ||
"crypto/sha256" | ||
"fmt" | ||
|
||
"github.com/go-xorm/xorm" | ||
"golang.org/x/crypto/pbkdf2" | ||
|
||
"code.gitea.io/gitea/modules/generate" | ||
"code.gitea.io/gitea/modules/util" | ||
) | ||
|
||
func addScratchHash(x *xorm.Engine) error { | ||
// TwoFactor see models/twofactor.go | ||
type TwoFactor struct { | ||
ID int64 `xorm:"pk autoincr"` | ||
UID int64 `xorm:"UNIQUE"` | ||
Secret string | ||
ScratchToken string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was removed in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still needed as below we take the plaintext scratch token, and then transform it into the hash. You'll see at the bottom of this file where the column is dropped. |
||
ScratchSalt string | ||
ScratchHash string | ||
LastUsedPasscode string `xorm:"VARCHAR(10)"` | ||
CreatedUnix util.TimeStamp `xorm:"INDEX created"` | ||
UpdatedUnix util.TimeStamp `xorm:"INDEX updated"` | ||
} | ||
|
||
if err := x.Sync2(new(TwoFactor)); err != nil { | ||
return fmt.Errorf("Sync2: %v", err) | ||
} | ||
|
||
sess := x.NewSession() | ||
defer sess.Close() | ||
|
||
if err := sess.Begin(); err != nil { | ||
return err | ||
} | ||
|
||
// transform all tokens to hashes | ||
const batchSize = 100 | ||
for start := 0; ; start += batchSize { | ||
tfas := make([]*TwoFactor, 0, batchSize) | ||
if err := x.Limit(batchSize, start).Find(&tfas); err != nil { | ||
return err | ||
} | ||
if len(tfas) == 0 { | ||
break | ||
} | ||
|
||
for _, tfa := range tfas { | ||
// generate salt | ||
salt, err := generate.GetRandomString(10) | ||
if err != nil { | ||
return err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should a single error of generating a salt fail the complete migration? I think logging the error might work too? What is your opinion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should fail the migration, as if it just logs then a user might miss that and be unable to log into their Gitea install if generating salt fails. |
||
} | ||
tfa.ScratchSalt = salt | ||
tfa.ScratchHash = hashToken(tfa.ScratchToken, salt) | ||
|
||
if _, err := sess.ID(tfa.ID).Cols("scratch_salt, scratch_hash").Update(tfa); err != nil { | ||
return fmt.Errorf("couldn't add in scratch_hash and scratch_salt: %v", err) | ||
} | ||
|
||
} | ||
} | ||
|
||
// Commit and begin new transaction for dropping columns | ||
if err := sess.Commit(); err != nil { | ||
return err | ||
} | ||
if err := sess.Begin(); err != nil { | ||
return err | ||
} | ||
|
||
if err := dropTableColumns(sess, "two_factor", "scratch_token"); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dropping column on table that has uncommited changes in transaction will raise error in MSSQL. See #4360 for more details |
||
return err | ||
} | ||
return sess.Commit() | ||
|
||
} | ||
|
||
func hashToken(token, salt string) string { | ||
tempHash := pbkdf2.Key([]byte(token), []byte(salt), 10000, 50, sha256.New) | ||
return fmt.Sprintf("%x", tempHash) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
// Copyright 2014 The Gogs Authors. All rights reserved. | ||
// Copyright 2018 The Gitea Authors. All rights reserved. | ||
// Use of this source code is governed by a MIT-style | ||
// license that can be found in the LICENSE file. | ||
|
||
|
@@ -305,7 +306,11 @@ func TwoFactorScratchPost(ctx *context.Context, form auth.TwoFactorScratchAuthFo | |
// Validate the passcode with the stored TOTP secret. | ||
if twofa.VerifyScratchToken(form.Token) { | ||
// Invalidate the scratch token. | ||
twofa.ScratchToken = "" | ||
_, err = twofa.GenerateScratchToken() | ||
if err != nil { | ||
ctx.ServerError("UserSignIn", err) | ||
return | ||
} | ||
if err = models.UpdateTwoFactor(twofa); err != nil { | ||
ctx.ServerError("UserSignIn", err) | ||
return | ||
|
@@ -320,7 +325,7 @@ func TwoFactorScratchPost(ctx *context.Context, form auth.TwoFactorScratchAuthFo | |
|
||
handleSignInFull(ctx, u, remember, false) | ||
ctx.Flash.Info(ctx.Tr("auth.twofa_scratch_used")) | ||
ctx.Redirect(setting.AppSubURL + "/user/settings/two_factor") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this incorrect prior to this change? 🤔 (If so we need to sort these routes out...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was missed when the user settings pages were refactored. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will open a new PR for this line. |
||
ctx.Redirect(setting.AppSubURL + "/user/settings/security") | ||
return | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might wanna put the commit ID in this comment as well, since that is where it's defined 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this PR is squashed and merged the commit ID will change to one we won't know.