Force journalists to use diceware passwords #1509

heartsucker · 2016-12-23T15:37:38Z

This is the initial work for fixing #980 (superseding #1493). Forces journalists to use auto-generated diceware passwords and does not allow user input for passwords.

This PR is not ready for merge, but is here for initial functionality review before fixing all of the unit/functional tests.

psivesely

Pretty good overall. I think we can get this one merged after one round of review if you can make the changes below and in-line, as well as add some new specific tests + fix existing ones.

Even though a call to crypto_util.genrandomid(7) will never produce a password of greater than 128 characters, it seems sensible to still check generated passwords against this value as a defense against future regressions.
s/KeePass/KeePassX\ database/g. Also, adding the word "immediately" before save, and maybe even putting "immediately save this new password to your KeePassX database" in bold would help ensure users/admins do not lose their password... Or, we can instruct users to copy the passphrase to their KeePassX database and then copy it from there back into the confirmation prompt--this goes for both ways a password is changed (see Prevent automated submissions (CAPTCHA) #3).
There is an edge case where the server undergoes a regular reboot or otherwise loses connection to the client after a password change via admin_edit_user. It is possible the password is changed, but the flashed message never makes it back to the admin, effectively locking everyone out of the account. Having a confirmation such as how the user is forced to retype their password in new_password is way to prevent this.
I'm not sure "encrypted USB" should be the go-to for transferring passwords. If the admin and journo see each other in person, Diceware passphrases can just be copied by hand w/o introducing another USB and assoc. risks. If they don't see each other in person regularly, then we'll need to recommend an alternative (let's discuss!).
In some of your try/except blocks you put additional statements and/or expressions in the try block than you are trying to protect (e.g., flash). A better place for these is an else block (see https://docs.python.org/3/tutorial/errors.html#handling-exceptions).

psivesely · 2017-01-04T23:16:19Z

securedrop/db.py

        # Enforce a reasonable maximum length for passwords to avoid DoS
-        if len(password) > self.MAX_PASSWORD_LEN:
+        if len(password) > Journalist.MAX_PASSWORD_LEN:


Since we're referencing a class attribute here, it makes sense to make this a classmethod instead of a staticmethod.

I think this does need to be static so it can be preemptively used without creating a Journalist instance. As in, we check the password N times, then make one instance and set it.

Or is that not a good approach?

I think this does need to be static so it can be preemptively used without creating a Journalist instance.

You can do the same thing with a class method. The main advantage in this case is that, by writing cls.MAX_PASSWORD_LEN, even if the Journalist class where to change names, the method would still work w/o requiring you to replace Journalist in the method body with the new class name. In this case it's unlikely we'll change the name of the Journalist class, but I still think it's good practice. This SO answer http://stackoverflow.com/a/12179752 might give a little more info. on when it's better to use one or the other.

Ah yes, you're totally right.

psivesely · 2017-01-04T23:16:48Z

securedrop/db.py

@@ -214,7 +214,12 @@ class BadTokenException(Exception):
    """Raised when a user logins in with an incorrect TOTP token"""


-class InvalidPasswordLength(Exception):
+class PasswordError(Exception):
+


Nit: I'd get rid of the empty line here and add a period to the docstring.

psivesely · 2017-01-05T00:27:17Z

securedrop/journalist.py

-            flash("Passwords didn't match", "error")
+        while True:
+            password = crypto_util.genrandomid(7)
+            if len(password) > Journalist.MAX_PASSWORD_LEN:


Should use Journalist.check_password_acceptable here the same way you did in manage.py.

psivesely · 2017-01-05T22:11:38Z

securedrop/manage.py

            continue

-        if password == password_again:
+    while True:
+        print("This journalist's password is: {}".format(password))


Currently manage.py is only used to create admin accounts. So your message about both the admin and the Journalist storing this entry doesn't make sense in this context.

psivesely · 2017-01-05T23:52:38Z

securedrop/journalist.py

@@ -306,24 +306,25 @@ def edit_account():
    user = g.user

    if request.method == 'POST':


There is no more new_password form in edit_account.html (instead you made a new view), so you can basically get rid of the entire body of edit_account except the return statement. And you'll want to remove POST from the app.route decorator as well.

heartsucker · 2017-01-10T23:03:18Z

@fowlslegs In reply to 3, the edge case about a reboot / server failure between the password being changed and the flashed message being send to the client, I think the best thing to do would be to redirect a confirmation page before changing the password (but persist all the other changes). The admin would have to click "yes, confirm password change" on a page with text saying "this is the new password, here's where to save it." Or something to that effect. Then the password change is persisted, and a confirmation is flashed. Done.

I'm only asking because it seems like UI/UX is now becoming a priority, and I figured I should check in before making this change. @ninavizz might want to give input here too.

psivesely · 2017-01-11T20:53:10Z

Maybe it is possible to use a JS window.confirm popup box if JS is enabled, or otherwise fall back to this confirm password change page? This is part of our larger make things nice easily with JS if it's enabled, but still provide full functionality--"graceful degradation" (see #1476 it's not.

heartsucker · 2017-01-19T09:48:32Z

@fowlslegs: I made the changes. One thing doesn't work. I'm trying to use the JS to add a field to the form, but for the life of me I can't get it to work. Do you think you could take a peek? i think everything else works here.

Edit: Just kidding. Got it. :D

psivesely · 2017-01-19T20:27:07Z

If you think this is ready can you fix the existing tests and write new ones to thoroughly test the new functionality?

heartsucker · 2017-01-24T13:45:04Z

Hey @fowlslegs, can i get some assistance with Selenium. Right now I have correct behavior in the browser, but I can't get it to work in the functional tests. Right now, I'm dying at tests/functional/journalist_navigation_steps.py:291. I have reset the password, but I can only log in with the old password. This does not happen in the browser. I've tried reloading the ORM object and a few other things, but it seems like the old password is... cached somehow?

heartsucker · 2017-01-25T12:41:50Z

Just kidding. Nailed it. :D

psivesely · 2017-01-26T05:52:25Z

Nice! I'll try to re-review Friday. Prob too busy tomorrow.

heartsucker · 2017-01-28T14:46:51Z

@fowlslegs Whenever you get around to checking this, there was an error on the VM:

OSError: [Errno 39] Directory not empty: '/tmp/securedrop'

I think restarting the build should get this green.

psivesely · 2017-01-28T23:47:38Z

Build restarted!

heartsucker · 2017-02-06T18:06:40Z

@fowlslegs I'm rebasing this at the moment and think there might be a better solution than the JS window.confirm you proposed. I think it would be easier to implement something like the two step form like #delete-all-confirm on the source interface. It would be one less route, one less HTML page, and one less thing to test with Selenium. Good idea, bad idea?

psivesely · 2017-02-06T22:18:34Z

@heartsucker Don't feel annoying pinging me if it's been a couple of days after I said I would review and I haven't. Everyone forgets.

Anyway, to answer your question, sounds like a good idea to me! Ping me when you've got that in and then I'll finally review.

heartsucker · 2017-02-06T22:33:13Z

@fowlslegs No worries. I was busy with other tickets and would have been just endlessly rebasing this. :)

heartsucker · 2017-08-20T04:11:58Z

Note: the function commit_account_changes is no longer called by the endpoints this PR affects. This is where the most of the coverage decrease comes from. The endpoint for admin_edit_user has no tests. This should be addressed elsewhere.

redshiftzero · 2017-08-22T18:05:37Z

securedrop/journalist.py

@@ -188,13 +184,10 @@ def admin_add_user():
                                      otp_secret=otp_secret)
                db_session.add(new_user)
                db_session.commit()
-            except InvalidPasswordLength:
+            except PasswordError:
+                flash('There was an error with the autogenerated passoword. '


passoword -> password

redshiftzero · 2017-08-22T18:07:45Z

Looks like a bug has crept in - I'm trying to make an admin from the command line and I get:

vagrant@development:/vagrant/securedrop$ ./manage.py add_admin
Traceback (most recent call last):
  File "./manage.py", line 409, in <module>
    _run_from_commandline()
  File "./manage.py", line 402, in _run_from_commandline
    rc = args.func(args)
TypeError: add_admin() takes no arguments (1 given)

(Relevant: #1982)

heartsucker · 2017-08-22T18:10:45Z

Fixed the bug. Travis should be happy in a minute.

redshiftzero · 2017-08-22T18:15:30Z

Cool - taking another look!

Also writing out here to summarize our discussion in person: since we've had a long back and forth in this PR, once the basic functionality and sufficient testing is down, I'm going to make two followup issues: one for design/UI cleanup and one for documentation. Note that we should tackle updating the documentation today when this is merged (even without updated screenshots, which we can do prior to release) so that any developers trying to work on SecureDrop are not confused by this new functionality.

redshiftzero

Approving this! Thanks for bearing with us on the long back and forth here @heartsucker . This is a solid improvement and I'm going to merge this in and any modifications we can do in a subsequent PR. I know the documentation updates were not added here, but I've made followup #2167 for that and we should get that done and merged in today.

redshiftzero · 2017-08-22T18:56:33Z

Staging failure looks like a flake, restarting

redshiftzero · 2017-08-22T20:51:35Z

Note: Coverage is decreasing here because @heartsucker has removed no cover from functions that were improperly excluded from the coverage computation in manage.py and added some test coverage for them. See coverage output here. I am not going to block merge on this and further test coverage for manage.py should be addressed in #1982.

In PR #2141, we added a flag in pytest to generate automatic screenshots for each page of the source and journalist interface in each translated language. At the same time we merged in #1509 which automatically generated diceware passphrases. Unfortunately there was a lingering reference to a password (that is now automatically generated) in one of the functions used to generate the screenshots, which caused two test failures.

heartsucker force-pushed the diceware-for-journalists branch from 1c476de to 502237e Compare December 26, 2016 18:50

psivesely suggested changes Jan 6, 2017

View reviewed changes

psivesely added journalist interface (JI) PR: Needs unit/functional tests PR: Pending additional work labels Jan 6, 2017

psivesely self-assigned this Jan 6, 2017

heartsucker force-pushed the diceware-for-journalists branch from 502237e to ceb4a40 Compare January 19, 2017 09:47

heartsucker force-pushed the diceware-for-journalists branch 2 times, most recently from 047d4a3 to 7856d37 Compare January 19, 2017 10:17

heartsucker force-pushed the diceware-for-journalists branch 3 times, most recently from 1489654 to 7f5a0bc Compare January 24, 2017 13:41

heartsucker force-pushed the diceware-for-journalists branch from 7f5a0bc to 45fe083 Compare January 25, 2017 12:33

heartsucker force-pushed the diceware-for-journalists branch 2 times, most recently from 2594850 to 303a244 Compare January 28, 2017 12:01

heartsucker force-pushed the diceware-for-journalists branch 2 times, most recently from 5fb0e56 to 54c93c0 Compare February 1, 2017 08:01

heartsucker force-pushed the diceware-for-journalists branch from 54c93c0 to e302bc7 Compare February 7, 2017 16:48

heartsucker force-pushed the diceware-for-journalists branch 4 times, most recently from 5c084ca to 6c0e045 Compare August 20, 2017 03:54

heartsucker force-pushed the diceware-for-journalists branch from 6c0e045 to 310b922 Compare August 20, 2017 18:44

redshiftzero reviewed Aug 22, 2017

View reviewed changes

heartsucker force-pushed the diceware-for-journalists branch from 310b922 to 00bc3dc Compare August 22, 2017 18:07

force journalist to use diceware passwords

b85bffc

heartsucker force-pushed the diceware-for-journalists branch from 00bc3dc to b85bffc Compare August 22, 2017 18:09

heartsucker mentioned this pull request Aug 22, 2017

Write unit tests for manage commands #1982

Open

redshiftzero mentioned this pull request Aug 22, 2017

Update documentation and screenshots to explain passwords are autogenerated #2167

Closed

redshiftzero approved these changes Aug 22, 2017

View reviewed changes

heartsucker mentioned this pull request Aug 22, 2017

Add integration tests for admin_edit_user #2168

Closed

redshiftzero merged commit dbd665e into freedomofpress:develop Aug 22, 2017

heartsucker deleted the diceware-for-journalists branch August 22, 2017 21:07

redshiftzero mentioned this pull request Aug 24, 2017

Fix two page layout screenshot failures #2183

Closed

This was referenced Sep 12, 2017

Merge final 0.4.3 post-release changes into develop #2285

Merged

Merge final 0.4.3 post-release changes into master #2286

Merged

eloquence mentioned this pull request Mar 4, 2021

Document passphrase requirements for SecureDrop accounts freedomofpress/securedrop-workstation-docs#71

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force journalists to use diceware passwords #1509

Force journalists to use diceware passwords #1509

heartsucker commented Dec 23, 2016

psivesely left a comment

psivesely Jan 4, 2017

heartsucker Jan 10, 2017

psivesely Jan 10, 2017

heartsucker Jan 10, 2017

psivesely Jan 4, 2017

psivesely Jan 5, 2017

psivesely Jan 5, 2017

psivesely Jan 5, 2017

heartsucker commented Jan 10, 2017

psivesely commented Jan 11, 2017

heartsucker commented Jan 19, 2017 •

edited

Loading

psivesely commented Jan 19, 2017

heartsucker commented Jan 24, 2017

heartsucker commented Jan 25, 2017

psivesely commented Jan 26, 2017

heartsucker commented Jan 28, 2017

psivesely commented Jan 28, 2017

heartsucker commented Feb 6, 2017

psivesely commented Feb 6, 2017

heartsucker commented Feb 6, 2017

heartsucker commented Aug 20, 2017 •

edited

Loading

redshiftzero Aug 22, 2017

redshiftzero commented Aug 22, 2017

heartsucker commented Aug 22, 2017

redshiftzero commented Aug 22, 2017

redshiftzero left a comment

redshiftzero commented Aug 22, 2017

redshiftzero commented Aug 22, 2017 •

edited

Loading

		@@ -306,24 +306,25 @@ def edit_account():
		user = g.user

		if request.method == 'POST':

Force journalists to use diceware passwords #1509

Force journalists to use diceware passwords #1509

Conversation

heartsucker commented Dec 23, 2016

psivesely 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

heartsucker commented Jan 10, 2017

psivesely commented Jan 11, 2017

heartsucker commented Jan 19, 2017 • edited Loading

psivesely commented Jan 19, 2017

heartsucker commented Jan 24, 2017

heartsucker commented Jan 25, 2017

psivesely commented Jan 26, 2017

heartsucker commented Jan 28, 2017

psivesely commented Jan 28, 2017

heartsucker commented Feb 6, 2017

psivesely commented Feb 6, 2017

heartsucker commented Feb 6, 2017

heartsucker commented Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

redshiftzero commented Aug 22, 2017

heartsucker commented Aug 22, 2017

redshiftzero commented Aug 22, 2017

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero commented Aug 22, 2017

redshiftzero commented Aug 22, 2017 • edited Loading

heartsucker commented Jan 19, 2017 •

edited

Loading

heartsucker commented Aug 20, 2017 •

edited

Loading

redshiftzero commented Aug 22, 2017 •

edited

Loading