Better password storage #1753

nextgens · 2021-02-07T16:45:35Z

What type of PR?

Enhancement: optimization of the logic to speedup authentication requests, support the import of most hashes passlib supports.

What does this PR do?

it changes the default password cold-storage format to sha256+bcrypt
it enhances the logic to ensure that no CPU cycles are wasted when valid credentials are found
it fixes token authentication on /webdav/
it lowers the number of rounds used for token storage (on the basis that they are high-entropy: not bruteforceable and speed matters)
it introduces a new setting to set the number of rounds used by the password hashing function (CREDENTIAL_ROUNDS). The setting can be adjusted as required and existing hashes will be migrated to the new cost-factor.
it updates the version of passlib in use and enables all supported hash types (that will be converted to the current settings on first use)
it removes the PASSWORD_SCHEME setting

Related issue(s)

Prerequistes

Before we can consider review and merge, please make sure the following list is done and checked.
If an entry in not applicable, you can check it or remove it from the list.

In case of feature or enhancement: documentation updated accordingly
Unless it's docs or a minor change: add changelog entry file.

mergify · 2021-02-07T16:46:08Z

Thanks for submitting this pull request.
Bors-ng will now build test images. When it succeeds, we will continue to review and test your PR.

bors try

Note: if this build fails, read this.

bors · 2021-02-07T17:34:44Z

try

Build failed:

continuous-integration/travis-ci/push

nextgens · 2021-02-08T07:57:16Z

bors retry

bors · 2021-02-08T07:57:17Z

🔒 Permission denied

Existing reviewers: click here to make nextgens a reviewer

lub

Thank you very much, although the review took me longer than anticipated. But that's not your fault :)

I've left some thoughts and suggestions. Would be great if you could have a look at them, although I suspect you already have more experience with that part of the code than me. So maybe I missed some things.

I haven't tested the code on my system(s) yet, in case you want to incorporate some of the notes. But from reading it I didn't find any bugs.

core/admin/mailu/configuration.py

core/admin/mailu/internal/nginx.py

core/admin/mailu/internal/views/auth.py

core/admin/mailu/models.py

docs/configuration.rst

towncrier/newsfragments/1194.misc

towncrier/newsfragments/1662.feature

Pull request has been modified.

core/admin/mailu/models.py

Pull request has been modified.

lub

thank you for also adding context caching :)

nextgens · 2021-02-19T17:58:15Z

@ghostwheel42 any chance you can update your review to get it merged?

ghostwheel42

@nextgens sorry, I somehow overlooked this.
But I think there's just a bors try missing, isn't it?

Diman0

Looks good to me. Thank you for this PR. It is a great improvement for mailu.

mergify · 2021-03-08T13:39:53Z

bors r+

1753: Better password storage r=mergify[bot] a=nextgens ## What type of PR? Enhancement: optimization of the logic to speedup authentication requests, support the import of most hashes passlib supports. ## What does this PR do? - it changes the default password cold-storage format to sha256+bcrypt - it enhances the logic to ensure that no CPU cycles are wasted when valid credentials are found - it fixes token authentication on /webdav/ - it lowers the number of rounds used for token storage (on the basis that they are high-entropy: not bruteforceable and speed matters) - it introduces a new setting to set the number of rounds used by the password hashing function (CREDENTIAL_ROUNDS). The setting can be adjusted as required and existing hashes will be migrated to the new cost-factor. - it updates the version of passlib in use and enables all supported hash types (that will be converted to the current settings on first use) - it removes the PASSWORD_SCHEME setting ### Related issue(s) - close #1194 - close #1662 - close #1706 ## Prerequistes Before we can consider review and merge, please make sure the following list is done and checked. If an entry in not applicable, you can check it or remove it from the list. - [x] In case of feature or enhancement: documentation updated accordingly - [x] Unless it's docs or a minor change: add [changelog](https://mailu.io/master/contributors/guide.html#changelog) entry file. Co-authored-by: Florent Daigniere <nextgens@freenetproject.org>

bors · 2021-03-08T14:28:12Z

Build failed:

continuous-integration/travis-ci/push

Diman0 · 2021-03-08T15:57:28Z

bors retry

1753: Better password storage r=mergify[bot] a=nextgens ## What type of PR? Enhancement: optimization of the logic to speedup authentication requests, support the import of most hashes passlib supports. ## What does this PR do? - it changes the default password cold-storage format to sha256+bcrypt - it enhances the logic to ensure that no CPU cycles are wasted when valid credentials are found - it fixes token authentication on /webdav/ - it lowers the number of rounds used for token storage (on the basis that they are high-entropy: not bruteforceable and speed matters) - it introduces a new setting to set the number of rounds used by the password hashing function (CREDENTIAL_ROUNDS). The setting can be adjusted as required and existing hashes will be migrated to the new cost-factor. - it updates the version of passlib in use and enables all supported hash types (that will be converted to the current settings on first use) - it removes the PASSWORD_SCHEME setting ### Related issue(s) - close #1194 - close #1662 - close #1706 ## Prerequistes Before we can consider review and merge, please make sure the following list is done and checked. If an entry in not applicable, you can check it or remove it from the list. - [x] In case of feature or enhancement: documentation updated accordingly - [x] Unless it's docs or a minor change: add [changelog](https://mailu.io/master/contributors/guide.html#changelog) entry file. Co-authored-by: Florent Daigniere <nextgens@freenetproject.org>

bors · 2021-03-08T16:29:06Z

Build failed:

continuous-integration/travis-ci/push

Diman0 · 2021-03-09T09:37:41Z

bors retry

bors · 2021-03-09T10:29:24Z

Merge conflict.

Pull request has been modified.

This is also fixing tokens on /webdav/

shortcomings of the previous format included: - 1000x slower than it should be (no point in adding rounds since there is enough entropy: they are not bruteforceable) - vulnerable to DoS as explained in https://passlib.readthedocs.io/en/stable/lib/passlib.hash.sha256_crypt.html#security-issues

- remove PASSWORD_SCHEME altogether - introduce CREDENTIAL_ROUNDS - migrate all old hashes to the current format - auto-detect/enable all hash types that passlib supports - upgrade passlib to 1.7.4 (see Mailu#1706: ldap_salted_sha512 support)

nextgens · 2021-03-09T11:12:19Z

bors r+

bors · 2021-03-09T11:44:51Z

Build succeeded:

continuous-integration/travis-ci/push

2056: Passlib r=mergify[bot] a=ghostwheel42 ## What type of PR? minor bug-fix ## What does this PR do? compiles list of schemes using an iterator. will not fail when `scrypt` is not present in registry. ### Related issue(s) updates #1753 ## Prerequisites Before we can consider review and merge, please make sure the following list is done and checked. If an entry in not applicable, you can check it or remove it from the list. - [ ] In case of feature or enhancement: documentation updated accordingly - [ ] Unless it's docs or a minor change: add [changelog](https://mailu.io/master/contributors/workflow.html#changelog) entry file. Co-authored-by: Alexander Graf <ghostwheel42@users.noreply.github.com>

bors bot added a commit that referenced this pull request Feb 7, 2021

Try #1753:

d60c5ff

nextgens mentioned this pull request Feb 7, 2021

Make authentication fast #1745

Closed

2 tasks

lub previously requested changes Feb 8, 2021

View reviewed changes

nextgens requested a review from lub February 9, 2021 08:46

ghostwheel42 reviewed Feb 9, 2021

View reviewed changes

core/admin/mailu/models.py Outdated Show resolved Hide resolved

lub previously approved these changes Feb 11, 2021

View reviewed changes

nextgens force-pushed the better-password-storage branch 2 times, most recently from cf0ef56 to aba4e3e Compare February 12, 2021 09:39

lub previously approved these changes Feb 13, 2021

View reviewed changes

ghostwheel42 approved these changes Feb 20, 2021

View reviewed changes

This was referenced Feb 24, 2021

refining the default password scheme #1194

Closed

Support more password schemes from passlib #1662

Closed

Diman0 previously approved these changes Mar 8, 2021

View reviewed changes

nextgens added 17 commits March 9, 2021 12:04

Don't do more work than necessary

58b2cdc

Don't do more work than necessary (/webdav)

eb7895b

This is also fixing tokens on /webdav/

Misc improvements to PASSWORD_SCHEME

7137ba6

- remove PASSWORD_SCHEME altogether - introduce CREDENTIAL_ROUNDS - migrate all old hashes to the current format - auto-detect/enable all hash types that passlib supports - upgrade passlib to 1.7.4 (see Mailu#1706: ldap_salted_sha512 support)

Remove {scheme} from the DB if mailu has set it

57a6aba

towncrier

927bd2b

remove merge artifact

fda758e

Fix the test

89d88e0

Fix the tests (again)

29306d5

Move CREDENTIAL_ROUNDS to advanced settings

d0b34f8

Be specific token length

f9ed517

Refactor auth under nginx.check_credentials()

df230cb

Improve the description of CREDENTIAL_ROUNDS

20d2b62

Improve the towncrier messages

45e5cb9

Remove scheme_dict

1c5b58c

Don't need regexps anymore

5f05fee

CryptContext should be a singleton

96ae54d

nextgens force-pushed the better-password-storage branch from 0521c13 to 96ae54d Compare March 9, 2021 11:06

bors bot merged commit 7e2db9c into Mailu:master Mar 9, 2021

nextgens deleted the better-password-storage branch October 31, 2021 08:43

ghostwheel42 mentioned this pull request Nov 16, 2021

Passlib #2056

Merged

2 tasks

nextgens mentioned this pull request Jan 7, 2022

Login is failing with older password hashes in Version 1.8 #2148

Closed

2 tasks

DjVinnii mentioned this pull request Oct 19, 2022

Replace PASSWORD_SCHEME with CREDENTIAL_ROUNDS Mailu/helm-charts#196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better password storage #1753

Better password storage #1753

nextgens commented Feb 7, 2021 •

edited

mergify bot commented Feb 7, 2021

bors bot commented Feb 7, 2021

nextgens commented Feb 8, 2021

bors bot commented Feb 8, 2021

lub left a comment

lub left a comment

nextgens commented Feb 19, 2021

ghostwheel42 left a comment •

edited

Diman0 left a comment

mergify bot commented Mar 8, 2021

bors bot commented Mar 8, 2021

Diman0 commented Mar 8, 2021

bors bot commented Mar 8, 2021

Diman0 commented Mar 9, 2021

bors bot commented Mar 9, 2021

nextgens commented Mar 9, 2021

bors bot commented Mar 9, 2021

Better password storage #1753

Better password storage #1753

Conversation

nextgens commented Feb 7, 2021 • edited

What type of PR?

What does this PR do?

Related issue(s)

Prerequistes

mergify bot commented Feb 7, 2021

bors bot commented Feb 7, 2021

try

nextgens commented Feb 8, 2021

bors bot commented Feb 8, 2021

lub left a comment

Choose a reason for hiding this comment

lub left a comment

Choose a reason for hiding this comment

nextgens commented Feb 19, 2021

ghostwheel42 left a comment • edited

Choose a reason for hiding this comment

Diman0 left a comment

Choose a reason for hiding this comment

mergify bot commented Mar 8, 2021

bors bot commented Mar 8, 2021

Diman0 commented Mar 8, 2021

bors bot commented Mar 8, 2021

Diman0 commented Mar 9, 2021

bors bot commented Mar 9, 2021

nextgens commented Mar 9, 2021

bors bot commented Mar 9, 2021

nextgens commented Feb 7, 2021 •

edited

ghostwheel42 left a comment •

edited