Skip to content
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

Use argon as default password hash algorithm #12688

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Sep 2, 2020

This is a partial repush of #10602

Changes to Password-Based Key Derivation

Currently, Gitea uses pbkdf2 as the default password hashing function. In this pull request, I replace that with argon2 as the default, for the following reasons:

pbkdf2 is uniquely vulnerable to GPU and ASIC-based attacks that argon2 and scrypt are much less vulnerable against.
pbkdf2 is vulnerable to acceleration attacks and to a host of other attacks that argon2 and scrypt are not vulnerable to.
Therefore, given the above and especially given that argon2 is a well-specified, formally studied design that has won the Password Hashing Competition, I think using it as the default instead of pbkdf2 makes complete sense.

Closes #10602

Many thanks to @KAepora for initial PR.

@zeripath zeripath added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Sep 2, 2020
@zeripath zeripath added this to the 1.13.0 milestone Sep 2, 2020
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Is it worth it to create a migration to update the default value?

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 2, 2020
@GiteaBot GiteaBot 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 Sep 2, 2020
@zeripath
Copy link
Contributor Author

zeripath commented Sep 2, 2020

Remember it's a hash. You don't have the original password so you can't write a migration you just have to let people login, test against the old hash and update to the new hash. The only other option is forcibly resetting everyone's password which we could make a doctor command for I guess?

@techknowlogick
Copy link
Member

By migration I meant just the default value for the column rather than the content of the column itself, as otherwise the sync2 will update it. Although if we wanted to also update the column content we could have something in login that catches if existing hash is using different algo and update when the user is logging in.

@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2020

Codecov Report

Merging #12688 into master will increase coverage by 0.00%.
The diff coverage is 11.81%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12688   +/-   ##
=======================================
  Coverage   43.29%   43.29%           
=======================================
  Files         646      645    -1     
  Lines       71592    71560   -32     
=======================================
- Hits        30993    30985    -8     
+ Misses      35583    35554   -29     
- Partials     5016     5021    +5     
Impacted Files Coverage Δ
models/user.go 53.56% <ø> (-0.20%) ⬇️
modules/graceful/server.go 47.00% <0.00%> (-0.41%) ⬇️
modules/highlight/highlight.go 24.69% <0.00%> (ø)
modules/migrations/gitlab.go 1.04% <0.00%> (-0.10%) ⬇️
modules/session/virtual.go 60.20% <0.00%> (ø)
routers/repo/issue.go 37.51% <0.00%> (-0.03%) ⬇️
modules/migrations/base/downloader.go 17.64% <5.12%> (-5.72%) ⬇️
modules/migrations/github.go 83.36% <100.00%> (-0.41%) ⬇️
modules/migrations/migrate.go 23.95% <100.00%> (-0.46%) ⬇️
modules/setting/setting.go 46.81% <100.00%> (ø)
... and 13 more

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 8fa7a4b...ab8eb84. Read the comment docs.

@bagasme
Copy link
Contributor

bagasme commented Sep 3, 2020

What about specific notes for migrating for pbkdf2 to argon? Simply changing app.ini is enough?

@zeripath
Copy link
Contributor Author

zeripath commented Sep 3, 2020

yes simply changing the app.ini is enough.

@techknowlogick techknowlogick merged commit 5c0697a into go-gitea:master Sep 3, 2020
@zeripath zeripath deleted the pr-10602-use-argon-as-default-password branch September 3, 2020 19:00
@silverwind
Copy link
Member

silverwind commented Sep 9, 2020

Should this have included a migration? I'm seeing these warnings (MariaDB):

2020/09/09 21:21:11 ...rm/session_schema.go:360:Sync2() [W] Table user Column passwd_hash_algo db default is 'pbkdf2', struct default is 'argon2'

@lafriks
Copy link
Member

lafriks commented Sep 9, 2020

Yeah, this should have migration to change default for column

@zeripath
Copy link
Contributor Author

zeripath commented Sep 9, 2020

Can't change default columns without recreate-table.

@zeripath
Copy link
Contributor Author

zeripath commented Sep 9, 2020

fortunately we now have recreate-table

@zeripath
Copy link
Contributor Author

zeripath commented Sep 9, 2020

so if it's really wanted we could use recreate-table in a migration

zeripath added a commit to zeripath/gitea that referenced this pull request Sep 9, 2020
go-gitea#12688 changed the default for the user table leading to sync2 warnings

Unfortunately changing defaults requires a complete table rewrite in general.

However, just dropping columns could be bad - so this PR leverages the
techniques used in recreate table to recreate from the inferred schema
and recreates the user table.

This is not necessarily the correct thing to do - but code sometimes speaks
louder than words.

Signed-off-by: Andrew Thornton <art27@cantab.net>
techknowlogick added a commit that referenced this pull request Sep 15, 2020
* Add migration for password algorithm change

#12688 changed the default for the user table leading to sync2 warnings

Unfortunately changing defaults requires a complete table rewrite in general.

However, just dropping columns could be bad - so this PR leverages the
techniques used in recreate table to recreate from the inferred schema
and recreates the user table.

This is not necessarily the correct thing to do - but code sometimes speaks
louder than words.

Signed-off-by: Andrew Thornton <art27@cantab.net>

* oops

Signed-off-by: Andrew Thornton <art27@cantab.net>

* ok lets use the shorter bits for other dbs

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Update models/migrations/v150.go

* Update models/migrations/v150.go

* fix migration

Signed-off-by: Andrew Thornton <art27@cantab.net>

* mv v150 to v151.go

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants