Switch plaintext scratch tokens to use hash instead #4331

techknowlogick · 2018-06-28T03:01:02Z

As title

codecov-io · 2018-06-28T03:08:53Z

Codecov Report

Merging #4331 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #4331      +/-   ##
=========================================
- Coverage   20.21%   20.2%   -0.01%     
=========================================
  Files         156     156              
  Lines       31130   31140      +10     
=========================================
  Hits         6292    6292              
- Misses      23880   23890      +10     
  Partials      958     958

Impacted Files	Coverage Δ
routers/user/auth.go	`0% <0%> (ø)`	⬆️
routers/user/setting/security_twofa.go	`0% <0%> (ø)`	⬆️
models/twofactor.go	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac968c3...c001b14. Read the comment docs.

jonasfranz · 2018-07-05T12:20:11Z

models/migrations/v70.go

+)
+
+func addScratchHash(x *xorm.Engine) error {
+	// TeamUnit see models/twofactor.go


TeamUnit should be TwoFactor

jonasfranz · 2018-07-05T12:21:06Z

models/migrations/v70.go

+
+		for _, tfa := range tfas {
+			// generate salt
+			salt, _ := generateSalt()


error shouldn't be ignored

jonasfranz · 2018-07-05T12:25:29Z

routers/user/auth.go

@@ -304,7 +305,7 @@ 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 = ""
+		twofa.GenerateScratchToken()


error shouldn't be ignored

jonasfranz · 2018-07-05T14:28:13Z

models/migrations/v70.go

-			salt, _ := generateSalt()
+			salt, err := generateSalt()
+			if err != nil {
+				return err


Should a single error of generating a salt fail the complete migration? I think logging the error might work too? What is your opinion?

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.

bkcsoft · 2018-07-08T23:43:25Z

models/migrations/v70.go

+)
+
+func addScratchHash(x *xorm.Engine) error {
+	// TwoFactor see models/twofactor.go


might wanna put the commit ID in this comment as well, since that is where it's defined 🙂

When this PR is squashed and merged the commit ID will change to one we won't know.

bkcsoft · 2018-07-08T23:45:19Z

models/migrations/v70.go

+		ID               int64 `xorm:"pk autoincr"`
+		UID              int64 `xorm:"UNIQUE"`
+		Secret           string
+		ScratchToken     string


This was removed in models/twofactor.go 🤔

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.

bkcsoft · 2018-07-08T23:46:23Z

models/migrations/v70.go

+
+}
+
+func generateSalt() (string, error) {


If these have to be used outside models/ then they should be Exported, not copied 😱

("these"== includes the hashToken-function as well)

These are in here as copies because if the logic changes in models/ then it may affect potential other migrations in the future.

bkcsoft · 2018-07-08T23:48:07Z

models/twofactor.go

+	return token, nil
+}
+
+func generateSalt() (string, error) {


I'm very sure we have this function (and hashToken) defined somewhere else (related to hashing user passwords...). Find those and use them (Export if necessary)

We have hashPassword which is pretty much the same as this, but with the name hashPassword it is a bit misleading as the token is not a password.

I'd suggest moving the "crypto" part for hashing into the module package and reuse it for every hash generation we need!?

bkcsoft · 2018-07-08T23:49:49Z

routers/user/auth.go

@@ -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")


Was this incorrect prior to this change? 🤔 (If so we need to sort these routes out...)

This was missed when the user settings pages were refactored.

Will open a new PR for this line.

lafriks · 2018-07-11T13:33:53Z

models/migrations/v70.go

+		}
+	}
+
+	if err := dropTableColumns(sess, "two_factor", "scratch_token"); err != nil {


dropping column on table that has uncommited changes in transaction will raise error in MSSQL. See #4360 for more details

techknowlogick · 2018-07-13T19:32:19Z

@lafriks @bkcsoft @daviian updated.

lafriks · 2018-07-19T17:54:40Z

Our used xorm version has bug that can not start new transaction in same session, probably new PRs would be needed to update xorm

appleboy · 2018-07-26T07:39:50Z

conflicts

Prepare for wild/v1.6 branch * BREAKING * Respect email privacy option in user search via API (go-gitea#4512) * Simply remove tidb and deps (go-gitea#3993) * Swagger.v1.json template (go-gitea#3572) * FEATURE * Pull request review/approval and comment on code (go-gitea#3748) * Added dependencies for issues (go-gitea#2196) (go-gitea#2531) * Add the ability to have built in themes in Gitea and provide dark theme arc-green (go-gitea#4198) * Add sudo functionality to the API (go-gitea#4809) * Add oauth providers via cli (go-gitea#4591) * Disable merging a WIP Pull request (go-gitea#4529) * Force user to change password (go-gitea#4489) * Add letsencrypt to Gitea (go-gitea#4189) * Add push webhook support for mirrored repositories (go-gitea#4127) * Add csv file render support defaultly (go-gitea#4105) * Add Recaptcha functionality to Gitea (go-gitea#4044) * BUGFIXES * Fix release creation via API (go-gitea#5076) * Remove links from topics in edit mode (go-gitea#5026) * Fix missing AppSubUrl in few more templates (fixup) (go-gitea#5021) * Fix missing AppSubUrl in some templates (go-gitea#5020) * Hide outdated comments in file view (go-gitea#5017) * Upgrade gopkg.in/testfixtures.v2 (go-gitea#4999) * Disable debug routes unless PPROF is enabled in configuration (go-gitea#4995) * Fix user menu item styling (go-gitea#4985) * Fix layout of the topics editing form (go-gitea#4971) * Fix null pointer dereference in ParseCommitWithSignature (go-gitea#4962) * Fix url in discord webhook (go-gitea#4953) * Detect charset and convert non UTF-8 files for display (go-gitea#4950) * Make sure to catch the right error so it is displayed on the UI (go-gitea#4945) * Fix(topics): don't redirect to explore page. (go-gitea#4938) * Fix bug forget to remove Stopwatch when remove repository (go-gitea#4928) * Fix bug when repo remained bare if multiple branches pushed in single push (go-gitea#4923) * Fix: Let's Encrypt configuration settings (go-gitea#4911) * Fix: Crippled diff (go-gitea#4726) (go-gitea#4900) * Fix trimming of markup section names (go-gitea#4863) * Issues api allow pulls and fix go-gitea#4832 (go-gitea#4852) * Do not autocreate directory for new users/orgs (go-gitea#4828) (go-gitea#4849) * Fix redirect with non-ascii branch names (go-gitea#4764) (go-gitea#4810) * Fix missing release title in webhook (go-gitea#4783) (go-gitea#4796) * User shouldn't be able to approve or reject his/her own PR (go-gitea#4729) * Make sure to reset commit count in the cache on mirror syncing (go-gitea#4720) * Fixed bug where team with admin privelege type doesn't get any unit (go-gitea#4719) * Fix incorrect caption of webhook setting (go-gitea#4701) (go-gitea#4717) * Allow WIP marker to contains < or > (go-gitea#4709) * Hide org/create menu item in Dashboard if user has no rights (go-gitea#4678) (go-gitea#4680) * Site admin could create repos even MAX_CREATION_LIMIT=0 (go-gitea#4645) * Fix custom templates being ignored (go-gitea#4638) * Fix starring icon after semantic ui update (go-gitea#4628) * Fix Split-View line adjustment (go-gitea#4622) * Fix integer constant overflows in tests (go-gitea#4616) * Push whitelist now doesn't apply to branch deletion (go-gitea#4601) (go-gitea#4607) * Fix bugs when too many IN variables (go-gitea#4594) * Fix failure on creating pull request with assignees (go-gitea#4419) (go-gitea#4583) * Fix panic issue on update avatar email (go-gitea#4580) (go-gitea#4581) * Fix status code label for a successful webhook (go-gitea#4540) * An inactive user shouldn't be able to be added as a collaborator (go-gitea#4535) * Don't fail silently if trying to add a collaborator twice (go-gitea#4533) * Fix incorrect MergeWhitelistTeamIDs check in CanUserMerge function (go-gitea#4519) (go-gitea#4525) * Fix out-of-transaction query in removeOrgUser (go-gitea#4521) (go-gitea#4522) * Fix migration from older releases (go-gitea#4495) * Accept 'Data:' in commit graph (go-gitea#4487) * Update xorm to latest version and fix correct `user` table referencing in sql (go-gitea#4473) * Relative URLs for LibreJS page (go-gitea#4460) * Redirect to correct page after using scratch token (go-gitea#4458) * Fix column droping for MSSQL that need new transaction for that (go-gitea#4440) * Replace src with raw to fix image paths (go-gitea#4377) * Add default merge options when creating new repository (go-gitea#4369) * Fix docker build (go-gitea#4358) * Fixes repo membership check in API (go-gitea#4341) * Dep upgrade mysql lib (go-gitea#4161) * Fix some issues with special chars in branch names (go-gitea#3767) * Responsive design fixes (go-gitea#4508) * ENHANCEMENT * Fix milestones sorted wrongly (go-gitea#4987) * Allow api to create tags for releases if they don't exist (go-gitea#4890) * Fix go-gitea#4877 to follow the OpenID Connect Audiences spec (go-gitea#4878) * Enforce token on api routes [fixed critical security issue go-gitea#4357] (go-gitea#4840) * Update legacy branch and tag URLs in dashboard to new format (go-gitea#4812) * Slack webhook channel name cannot be empty or just contain an hashtag (go-gitea#4786) * Add whitespace handling to PR-comparsion (go-gitea#4683) * Make reverse proxy auth optional (go-gitea#4643) * MySQL TLS (go-gitea#4642) * Make sure to set PR split view when creating/previewing a pull request (go-gitea#4617) * Log user in after a successful sign up (go-gitea#4615) * Fix typo IsPullReuqestBroken -> IsPullRequestBroken (go-gitea#4578) * Allow admin toggle forcing a password change for newly created users (go-gitea#4563) * Update jQuery to v1.12.4 (go-gitea#4551) * Env var GITEA_PUSHER_EMAIL (go-gitea#4516) * Feat(repo): support search repository by topic name (go-gitea#4505) * Small improvements to dependency UI (go-gitea#4503) * Make max commits in graph configurable (go-gitea#4498) * Add valid for lfs oid (go-gitea#4461) * Add shortcut to save wiki page (go-gitea#4452) * Allow administrator to create repository for any organization (go-gitea#4368) * Fix repository last updated time update when delete a user who watched the repo (go-gitea#4363) * Switch plaintext scratch tokens to use hash instead (go-gitea#4331) * Increase default TOTP secret size to 320 bits (go-gitea#4287) * Keep preseeded database password (go-gitea#4284) * Implemented hover text showing user FullName (go-gitea#4261) * Add ability to delete a token (go-gitea#4235) * Fix typos in i18n variable names. (go-gitea#4080) * Api: repos/search: add parameters to control the sort order (go-gitea#3964) * Add missing path in the Docker app.ini template (go-gitea#2181) * Add file name and branch to page title (go-gitea#4902) * Offline use of google fonts (go-gitea#4872) * Add missing History link to directory listings v2 (go-gitea#4829) * Locale for Edit and Remove due date issue (go-gitea#4802) * Disable 'May Import Local Repository' when is disabled by setting (Is… (go-gitea#4780) * API /admin/users/{username} missing parameter (go-gitea#4775) * Display error when adding a user to a team twice (go-gitea#4746) * Remove UsePrivilegeSeparation from the Docker sshd_config, see go-gitea#2876 (go-gitea#4722) * Focus title input when clicking helper link (go-gitea#4696) * Add vendor to user reserved words and format words list according alphabet (go-gitea#4685) * Add gitea/issues link to 500 page (go-gitea#4654) * Hide home button when landing page is not set to home (go-gitea#4651) * Remove link to GitHub issues in 404 template (go-gitea#4639) * Cmd/serve: pprof cpu and memory profile dumps to disk (go-gitea#4560) * Add flash message after an account has been successfully activated (go-gitea#4510) * Prevent html entity escaping on delete branch (go-gitea#4471) * Locale for button Edit on protected branch (go-gitea#4442) * Update notification icon (go-gitea#4343) * Added front-end topics validation (go-gitea#4316) * Don't display buttons if there are no system notifications (go-gitea#4280) * Issue due date api (go-gitea#3890) * SECURITY * Improve URL validation for external wiki and external issues (go-gitea#4710) * Make cookies HttpOnly and obey COOKIE_SECURE flag (go-gitea#4706) * Don't disclose emails of all users when sending out emails (go-gitea#4664) * Check that repositories can only be migrated to own user or organizations (go-gitea#4366) * TRANSLATION * Fix punctuation in English translation (go-gitea#4958) * Fix translation (go-gitea#4355)

switch plaintext tokens to use hash instead

2fca6d9

techknowlogick added type/enhancement An improvement of existing functionality topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! pr/wip This PR is not ready for review labels Jun 28, 2018

techknowlogick added this to the 1.6.0 milestone Jun 28, 2018

remove attrib that wasn't needed

050c96f

bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 28, 2018

techknowlogick added 2 commits June 27, 2018 23:11

correct function call

412e27b

show scratch token once to user

fd4445f

techknowlogick removed the pr/wip This PR is not ready for review label Jun 28, 2018

techknowlogick changed the title ~~[WIP] Switch plaintext scratch tokens to use hash instead~~ Switch plaintext scratch tokens to use hash instead Jun 28, 2018

techknowlogick added 4 commits June 27, 2018 23:20

fix error

e46e0bc

correct redirect URL

d9d8e0c

add copyright

fa2b6e5

Merge branch 'master' into tklk/issue-3389

5cb253a

jonasfranz suggested changes Jul 5, 2018

View reviewed changes

techknowlogick added 3 commits July 5, 2018 10:21

fix copy paste issue

7caff96

Don't ignore error

25ca551

don't ignore error

c703bd5

jonasfranz reviewed Jul 5, 2018

View reviewed changes

jonasfranz approved these changes Jul 5, 2018

View reviewed changes

bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 5, 2018

Merge branch 'master' into tklk/issue-3389

b66fb81

bkcsoft suggested changes Jul 8, 2018

View reviewed changes

Merge branch 'master' into tklk/issue-3389

e2e1636

lafriks requested changes Jul 11, 2018

View reviewed changes

techknowlogick added 2 commits July 12, 2018 11:42

Merge branch 'master' into tklk/issue-3389

66528b9

commit changes before dropping columns

5cbf3b8

techknowlogick added 4 commits July 13, 2018 10:47

make fmt

d499fc8

remove gensalt func

6165857

remove gen salt function

dc55b92

Merge branch 'master' into tklk/issue-3389

8ff84a4

Merge branch 'master' into tklk/issue-3389

a95cf37

bkcsoft approved these changes Jul 26, 2018

View reviewed changes

bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 26, 2018

lafriks approved these changes Jul 26, 2018

View reviewed changes

techknowlogick added 2 commits July 27, 2018 08:36

Rename v70.go to v71.go

e07acd1

Merge branch 'master' into tklk/issue-3389

c001b14

techknowlogick merged commit adf3f00 into go-gitea:master Jul 27, 2018

techknowlogick deleted the tklk/issue-3389 branch July 27, 2018 12:54

lafriks added the type/changelog Adds the changelog for a new Gitea version label Nov 10, 2018

go-gitea locked and limited conversation to collaborators Nov 24, 2020

delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch plaintext scratch tokens to use hash instead #4331

Switch plaintext scratch tokens to use hash instead #4331

techknowlogick commented Jun 28, 2018 •

edited

Loading

codecov-io commented Jun 28, 2018 •

edited

Loading

jonasfranz Jul 5, 2018

jonasfranz Jul 5, 2018

jonasfranz Jul 5, 2018

jonasfranz Jul 5, 2018

techknowlogick Jul 5, 2018

bkcsoft Jul 8, 2018

techknowlogick Jul 9, 2018

bkcsoft Jul 8, 2018

techknowlogick Jul 9, 2018

bkcsoft Jul 8, 2018

bkcsoft Jul 8, 2018

techknowlogick Jul 10, 2018

bkcsoft Jul 8, 2018

techknowlogick Jul 9, 2018

daviian Jul 12, 2018

bkcsoft Jul 8, 2018

techknowlogick Jul 9, 2018

techknowlogick Jul 17, 2018

lafriks Jul 11, 2018

techknowlogick commented Jul 13, 2018

lafriks commented Jul 19, 2018

appleboy commented Jul 26, 2018

Switch plaintext scratch tokens to use hash instead #4331

Switch plaintext scratch tokens to use hash instead #4331

Conversation

techknowlogick commented Jun 28, 2018 • edited Loading

codecov-io commented Jun 28, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techknowlogick commented Jul 13, 2018

lafriks commented Jul 19, 2018

appleboy commented Jul 26, 2018

techknowlogick commented Jun 28, 2018 •

edited

Loading

codecov-io commented Jun 28, 2018 •

edited

Loading