Password check #988

umang-malik · 2018-02-26T20:02:16Z

Whenever the user sets/changes his password, his password is checked in the "pwned passwords" database (https://www.troyhunt.com/ive-just-launched-pwned-passwords-version-2/). The strength of the password based on the characters is also checked.
If the password is found weak on the basis of the above, then the user is suggested to use a better password.
This PR is with reference to #986.

Changaco

This is a good start, but more work is required.

You put an error message in out['msg'], that's incorrect since it'll be displayed to the user as a success notification. Instead we should show a confirmation warning to the user with response.render('templates/confirm.spt', ...).
Your 4th commit sends the full hash to the pwnedpasswords API, we want to use the /range endpoint instead.
You only implemented checking the password when it's being modified, but we also want to check during log-in. We'll need to store the time of the last check in our DB so we don't query the API too often.

umang-malik · 2018-02-27T15:14:30Z

Ask user for confirmation using response.render('templates/confirm.spt', ...). Using out['msg'] for successful change in password.
Changed to /range endpoint of pwnedpasswords instead of sending the entire hash.

@Changaco, I don't understand why we need to check the strength of the password during log-in. The password has already been assessed while setting/changing it.
Also, I am not sure why we need to store the time of last check in our DB. There is already a restriction upon changing passwords, which itself restricts the API querying.

Changaco · 2018-02-27T15:44:20Z

We need to check during log-in because:

most people never change their password
we already have more than 3400 passwords in our database
the pwnedpasswords database can grow, a password that isn't in it now may end up in it later

Changaco · 2018-02-27T16:22:42Z

www/%username/settings/edit.spt

+ msg = _(
+ "Are you sure you want to change your password?"
+ )
+ raise response.render('templates/confirm.spt', state, cls='warning', msg=msg, back_to=back_to)


This confirmation warning is unnecessary, we should only warn the user when the password is weak or compromised.

Changaco · 2018-02-27T16:27:06Z

www/%username/settings/edit.spt

+ "The new password is not strong enough. We recommend you to choose a combination of letters, symbols, and numbers."
+ "Are you sure you want to change your password?"
+ )
+ raise response.render('templates/confirm.spt', state, msg=msg, back_to=back_to)


We need at least two different warning messages here, one when the password is weak, and another when the password is compromised (i.e. when it was found in the pwned database).

Changaco · 2018-02-27T16:29:05Z

www/%username/settings/edit.spt

 p.update_password(body['new-password'])
- out['msg'] = _("Your password has been changed.")
+ out['msg'] = _("Your password has been changed")


This change should be reverted.

Changaco · 2018-02-27T16:44:50Z

To keep track of password checks I suggest using the add_event and get_last_event_of_type methods, with a new 'password_check' event type and an empty payload (because all we need is a timestamp, and that's the event time ts).

umang-malik · 2018-02-27T20:39:39Z

The mentioned changes have been made.

Where should I implement the login-check? I am currently doing it in liberapay/security/authentication.py but am unable to send the notification to the user once he logs in.

umang-malik · 2018-02-28T12:26:36Z

@Changaco, p.add_event(website.db.get_cursor(), 'password-check', None) line is giving an error which I am unable to fix:

  File "/home/umang/Desktop/folder/liberapay.com/env/lib/python2.7/site-packages/algorithm.py", line 321, in loop
    new_state = function(**deps.as_kwargs)
  File "/home/umang/Desktop/folder/liberapay.com/liberapay/security/authentication.py", line 195, in authenticate_user_if_possible
    p = sign_in_with_form_data(body, state)
  File "/home/umang/Desktop/folder/liberapay.com/liberapay/security/authentication.py", line 88, in sign_in_with_form_data
    p.add_event(website.db.get_cursor(), 'password-check', None)
  File "/home/umang/Desktop/folder/liberapay.com/liberapay/models/participant.py", line 1126, in add_event
    return c.one("""
AttributeError: 'CursorContextManager' object has no attribute 'one'

Also, how do I notify the user about his weak password? I tried using %if state.get('password-check') in templates/log-in-form.html but it is not working.

Changaco · 2018-02-28T12:53:06Z

@umang-malik Dropping .get_cursor() should work, i.e.: p.add_event(website.db, 'password-check', None).

To notify the user I see two possibilities:

create a new templates/password-warning.spt template and call response.render(...)
create a new emails/password-warning.spt template and call p.notify(...)

The latter should be simpler to implement, and it would allow moving the password check to a background worker, so I think it's the best option.

umang-malik · 2018-02-28T17:41:21Z

@Changaco, All the requested things have been done. 😃
The time period for password check during login has been set to 30 days for now. Also, I have currently disabled an email notification for the same.
Please review the PR.

Changaco

Good work @umang-malik, it just needs some polishing.

Changaco · 2018-03-01T08:53:24Z

.gitignore

@@ -12,7 +12,9 @@ node_modules/
 .tox/
 htmlcov/
 tests/py/fixtures/TestTranslations.yml
+tests/py/fixtures/TestLogIn.yml


This test fixture file should be committed, not ignored.

Changaco · 2018-03-01T08:57:08Z

.gitignore

 .idea/ 
+payday-1.txt*


This should be payday-*.txt, and if we're adding that then we should also add payday.pid. Alternatively we could modify the default LOG_DIR value to put all this stuff in a sub-directory, and then ignore it.

Changaco · 2018-03-01T09:12:16Z

emails/password_warning.spt

+{{ _("Update your Liberapay password") }}
+
+[---] text/html
+<p>{{ _("{0}",message) }}</p>


This isn't right, _("{0}", message) is a no-op. You shouldn't send an already-translated message to a notification template. Instead you should send the variables that the template needs to generate the message. In this case it could be either password_strength and password_leak_count, or a password_status string (possible values: 'weak', 'common', 'compromised'). The latter is probably better because it allows changing the thresholds without breaking old notifications.

Changaco · 2018-03-01T09:31:45Z

liberapay/security/authentication.py

@@ -14,6 +17,9 @@
 from liberapay.models.participant import Participant
 from liberapay.utils import get_ip_net

+import passwordmeter
+from hashlib import sha1
+import requests


Please don't break the import style like this. Imports are split into specific groups (based on PEP8), and ordered alphabetically within each group.

Changaco · 2018-03-01T09:37:21Z

liberapay/security/authentication.py

+ "You are recommended to change your password."
+ )
+ p.notify('password_warning', email=False, message=password_check_message)
+ p.add_event(website.db, 'password-check', None)


This should be moved to a separate function, and the call to that new function should be wrapped to report and mask errors, like this:

try: check_password(password) except Exception as e: website.tell_sentry(e, state)

Changaco · 2018-03-01T09:41:16Z

liberapay/security/authentication.py

@@ -57,6 +63,41 @@ def sign_in_with_form_data(body, state):
 )
 if not p:
 state['log-in.error'] = _("Bad username or password.")
+ else:
+ last_password_check = p.get_last_event_of_type('password-check')
+ if (not last_password_check) or utcnow() - last_password_check.ts > timedelta(days=30):


I don't think we need to check every 30 days, once or twice per year should be enough, e.g. days=180.

Changaco · 2018-03-01T09:51:36Z

www/%username/settings/edit.spt

+ suffix = line.split(":")[0]
+ if passhash_short + suffix == passhash:
+ count = int(line.split(":")[1].strip())
+ strength, improvements = passwordmeter.test(body['new-password'])


This is code duplication, it should be replaced with a call to a separate check_password function.

Changaco · 2018-03-01T12:49:07Z

requirements_base.txt

+ --hash=sha256:a77328ac55dbb5735da99441870251befe135f687ab707a7a178561363b27704
+globre==0.1.5 \
+ --hash=sha256:ee214204f237e9114b8f61eeb61c2abd1e665ca3b59e5a6a0b070971c0bb12e2
+


I've looked through these new dependencies, they appear to be okay (although their source code could use a good cleanup).

One issue is that passwordmeter includes its own list of ten thousand common passwords, and always loads it, even if we skip the notword factor (which we should do since it's redundant with the pwnedpasswords check). This wastes approximately 1.2MB of RAM (measured with this get_size function on my laptop).

umang-malik · 2018-03-01T17:58:17Z

@Changaco , all the changes have been made.

I have added a seperate method to check password.
The notword factor in passwordmeter has been skipped. I can't think of an easy solution to the extra memory required.
tests/py/fixtures/TestLogIn.yml contains the details of the requests made to the pwnedpasswords API. The response headers change every time the tests are run, and hence adding the file to .gitignore seems appropriate.

obsolete

Changaco · 2018-03-03T17:23:43Z

I've made lots of changes.

@umang-malik You should look at them and let me know what you think.

Screenshots:

Changaco · 2018-03-03T17:39:53Z

I dropped the password strength test because it wasn't good. See this blog post for a complete explanation.

I didn't replace passwordmeter with zxcvbn-python because its algorithms are language-specific and by default only English is supported. We can try to integrate zxcvbn in the future, but it's not a priority.

umang-malik · 2018-03-03T18:54:16Z

@Changaco, I agree with the given blog's argument. Though I think that keeping a measure of password strength would help in setting a minimum threshold for passwords, especially in languages other than English, as the pwnedpassword database is heavily dominated by the it (and other common languages).

For example, hello1234 appears ~20,000 times, as compared to 172 of bonjour1234, or that müller just appears 11 times! Some common passwords in less-common languages might pass the pwnedpasswords test.
Other than that, the pwnedpasswords test looks good enough.

umang-malik · 2018-03-03T18:55:51Z

Thanks for improving the templates. They look a lot better than mine.

Changaco · 2018-03-03T19:54:33Z

The truth is that for Liberapay anything that's not a long string of random characters is a bad password. It doesn't really matter whether it's hello1234 or correcthorsebatterystaple, or how secure the latter is compared to the former, because the only secure password is the one you can’t remember.

umang-malik · 2018-03-03T20:07:21Z

Well, I guess it shall be fine. We can always pull up a new issue in case we feel the need to do so.
P.S. All this reading about passwords has made me realize how much I need to work on my own passwords. 😅

Signed-off-by: Umang Malik <umang99m@gmail.com>

1. Check if new password exists in pwned password database. 2. Check the strength of a password and ask for a stronger one if password is weak. Signed-off-by: Umang Malik <umang99m@gmail.com>

Changed the new-password to a stronger one for the test to check if changing password works correctly. Signed-off-by: Umang Malik <umang99m@gmail.com>

Signed-off-by: Umang Malik <umang99m@gmail.com>

…n while changing password. 1. Shifted from the to '/range' api for better security. 2. Ask user for confirmation while changing password. 3. Make change to test_sign_in.py in accordance with the above. Signed-off-by: Umang Malik <umang99m@gmail.com>

Different warnings are shown depending if password is weak, commonly used or is compromised. Warning is not shown if password is strong and not compromised. Signed-off-by: Umang Malik <umang99m@gmail.com>

Signed-off-by: Umang Malik <umang99m@gmail.com>

Also fixed add_event for password-check. Signed-off-by: Umang Malik <umang99m@gmail.com>

tests/py/fixtures/TestLogIn.yml, payday-1.txt and payday-1.txt.part are generated while running tox. Signed-off-by: Umang Malik <umang99m@gmail.com>

Signed-off-by: Umang Malik <umang99m@gmail.com>

Also changed payday-1.txt* to payday-*.txt* and added payday.pid to .gitignore. Signed-off-by: Umang Malik <umang99m@gmail.com>

It checks the word against top 10 thousand passwords, which is basically redundant with the pwnedpasswords check. Signed-off-by: Umang Malik <umang99m@gmail.com>

It gives a very high score to 'qwER43@!' and a very low score to 'correcthorsebatterystaple'.

Changaco · 2018-03-04T08:33:35Z

Rebased on master and cleaned up the commit messages.

Changaco · 2018-03-04T08:43:59Z

@umang-malik In the future you can drop the Signed-off-by lines in commit messages.

Changaco · 2018-03-05T13:51:55Z

Deployed and announced.

Changaco · 2018-03-06T13:32:25Z

I've emailed Troy Hunt to let him know about this.

umang-malik mentioned this pull request Feb 27, 2018

Password check #986

Closed

Changaco requested changes Feb 27, 2018

View reviewed changes

Changaco previously requested changes Mar 1, 2018

View reviewed changes

Changaco reviewed Mar 1, 2018

View reviewed changes

umang-malik added 10 commits March 4, 2018 09:26

Add python dependencies for password checks.

ed4f48e

Signed-off-by: Umang Malik <umang99m@gmail.com>

Check new passwords.

ca93b97

1. Check if new password exists in pwned password database. 2. Check the strength of a password and ask for a stronger one if password is weak. Signed-off-by: Umang Malik <umang99m@gmail.com>

Update test_sign_in.py with a stronger password.

163b3d9

Changed the new-password to a stronger one for the test to check if changing password works correctly. Signed-off-by: Umang Malik <umang99m@gmail.com>

Remove usage of pwnedpasswords module and directly use api.

cc190b6

Signed-off-by: Umang Malik <umang99m@gmail.com>

Classify passwords and remove warning if password is strong.

0fd9fee

Different warnings are shown depending if password is weak, commonly used or is compromised. Warning is not shown if password is strong and not compromised. Signed-off-by: Umang Malik <umang99m@gmail.com>

Implement password-check during log-in and storing last check in db.

569f556

Signed-off-by: Umang Malik <umang99m@gmail.com>

Send notification to change password on login.

1b154d8

Also fixed add_event for password-check. Signed-off-by: Umang Malik <umang99m@gmail.com>

Add emails/password_warning.spt template

f8f312b

Add files generated during testing.

02d0d5e

tests/py/fixtures/TestLogIn.yml, payday-1.txt and payday-1.txt.part are generated while running tox. Signed-off-by: Umang Malik <umang99m@gmail.com>

umang-malik and others added 14 commits March 4, 2018 09:31

Add the check_password method to Participant

93964e5

Signed-off-by: Umang Malik <umang99m@gmail.com>

Use check_password() method during log-in and update_password.

d8f327b

Update emails/password_warning.spt template.

614fac9

Also changed payday-1.txt* to payday-*.txt* and added payday.pid to .gitignore. Signed-off-by: Umang Malik <umang99m@gmail.com>

Skip notword factor in passwordmeter.

8f1484a

It checks the word against top 10 thousand passwords, which is basically redundant with the pwnedpasswords check. Signed-off-by: Umang Malik <umang99m@gmail.com>

impove wording of password warning notification

991dcad

fix recording of the 'password-check' event

a4f52d1

move password change warnings to a new simplate

8307702

further improve the password warning notification

9204832

drop counter-productive password meter

338b16a

It gives a very high score to 'qwER43@!' and a very low score to 'correcthorsebatterystaple'.

clean up the check_password method

cdd94a2

better rate limiting of password operations

370df39

improve the password change warning

761d5ff

improve the tests for the password checker

83d40f8

commit the new test fixture

b90bddf

Changaco force-pushed the password-check branch from 675bb46 to b90bddf Compare March 4, 2018 08:32

Changaco merged commit 27d1c90 into liberapay:master Mar 4, 2018

umang-malik deleted the password-check branch March 4, 2018 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Password check #988

Password check #988

umang-malik commented Feb 26, 2018

Changaco left a comment •

edited

Loading

umang-malik commented Feb 27, 2018

Changaco commented Feb 27, 2018

Changaco Feb 27, 2018

Changaco Feb 27, 2018

Changaco Feb 27, 2018

Changaco commented Feb 27, 2018

umang-malik commented Feb 27, 2018 •

edited

Loading

umang-malik commented Feb 28, 2018

Changaco commented Feb 28, 2018

umang-malik commented Feb 28, 2018

Changaco left a comment

Changaco Mar 1, 2018

Changaco Mar 1, 2018

Changaco Mar 1, 2018

Changaco Mar 1, 2018

Changaco Mar 1, 2018

Changaco Mar 1, 2018

Changaco Mar 1, 2018

Changaco Mar 1, 2018

umang-malik commented Mar 1, 2018

Changaco commented Mar 3, 2018

Changaco commented Mar 3, 2018

umang-malik commented Mar 3, 2018 •

edited

Loading

umang-malik commented Mar 3, 2018

Changaco commented Mar 3, 2018

umang-malik commented Mar 3, 2018

Changaco commented Mar 4, 2018

Changaco commented Mar 4, 2018

Changaco commented Mar 5, 2018

Changaco commented Mar 6, 2018

Password check #988

Password check #988

Conversation

umang-malik commented Feb 26, 2018

Changaco left a comment • edited Loading

Choose a reason for hiding this comment

umang-malik commented Feb 27, 2018

Changaco commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Changaco commented Feb 27, 2018

umang-malik commented Feb 27, 2018 • edited Loading

umang-malik commented Feb 28, 2018

Changaco commented Feb 28, 2018

umang-malik commented Feb 28, 2018

Changaco left a 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

umang-malik commented Mar 1, 2018

Changaco commented Mar 3, 2018

Changaco commented Mar 3, 2018

umang-malik commented Mar 3, 2018 • edited Loading

umang-malik commented Mar 3, 2018

Changaco commented Mar 3, 2018

umang-malik commented Mar 3, 2018

Changaco commented Mar 4, 2018

Changaco commented Mar 4, 2018

Changaco commented Mar 5, 2018

Changaco commented Mar 6, 2018

Changaco left a comment •

edited

Loading

umang-malik commented Feb 27, 2018 •

edited

Loading

umang-malik commented Mar 3, 2018 •

edited

Loading