Added passlib for more flexible password hashing #3506

heartsucker · 2018-06-08T16:51:58Z

Status

Ready for review

Description of Changes

Fixes #2918

Adds passlib library for handing password hashing and verification
Replace scrypt with argon2
Adds database column & migration for new hashes

Note: This shouldn't be merged until after the 0.8.0 release is branched off develop

Testing

Check the manual tests.

make test

Do a manual test

git checkout develop
make build-debs
vagrant up /staging/

# add a user, login

git checkout passlib
make build-debs
vagrant provision /staging/

# check can still login
# add new user

Deployment

This includes legacy support for old password hashing schemes, so old journalists accounts will still be able to log in. This adds a new apt dependency, but it's in the security lists so we won't error out.

Checklist

If you made changes to the server application code:

Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made changes to documentation:

Doc linting (make docs-lint) passed locally

codecov-io · 2018-06-08T17:33:29Z

Codecov Report

Merging #3506 into develop will increase coverage by 0.14%.
The diff coverage is 82%.

@@             Coverage Diff             @@
##           develop    #3506      +/-   ##
===========================================
+ Coverage    85.69%   85.83%   +0.14%     
===========================================
  Files           40       41       +1     
  Lines         2629     2655      +26     
  Branches       284      287       +3     
===========================================
+ Hits          2253     2279      +26     
- Misses         310      312       +2     
+ Partials        66       64       -2

Impacted Files	Coverage Δ
securedrop/create-dev-data.py	`0% <ø> (ø)`	⬆️
securedrop/qa_loader.py	`84.93% <100%> (+0.42%)`	⬆️
...s/2d0ce3ee5bdc_added_passphrase_hash_column_to_.py	`57.14% <57.14%> (ø)`
securedrop/models.py	`91.18% <90.62%> (+1.39%)`	⬆️

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 300915d...1874ce7. Read the comment docs.

heartsucker · 2018-06-08T22:15:51Z

Ok, not gonna lie I am actually blown away that I managed to get a PR in that passed CI on the first attempt. This isn't a useful thing to say, but it's late, and I expected to have build error emails. Anyway.

ghost · 2018-06-09T06:24:00Z

docs/development/setup_development.rst

@@ -103,7 +103,7 @@ created by default when running ``make dev``. In addition, sources and
 submissions are present. The test users have the following credentials:

 * **Username:** ``journalist`` or ``dellsberg``
-* **Password:** ``WEjwn8ZyczDhQSK24YKM8C9a``


This password is used for the demo and published at http://demo.securedrop.club/ and http://i18n.securedrop.club/. I'm not against changing it because it is consistent with the password constraints and will pass validation.

The only downside I can see is that it is slightly more complicated to copy/paste.

It would be nice to also have a merge request against the demo to change the index page

If we really want to keep it, I can do that, but it would mean adding (a small amount) of complexity to get code. It's already very terse, and we'd basically end up factoring out a single function call to get this behavior.

I am in favor of changing to a passphrase instead of the legacy password.

redshiftzero

(haven't tested this, just a few comments from a first pass through the diff)

redshiftzero · 2018-06-14T16:39:53Z

install_files/securedrop-app-code/DEBIAN/control

@@ -6,5 +6,5 @@ Homepage: https://securedrop.org
 Package: securedrop-app-code
 Version: 0.8.0~rc1
 Architecture: amd64
-Depends: python-pip,apparmor-utils,gnupg2,haveged,python,secure-delete,sqlite3,apache2-mpm-worker,libapache2-mod-wsgi,libapache2-mod-xsendfile,redis-server,supervisor,securedrop-keyring,securedrop-config
+Depends: python-pip,apparmor-utils,gnupg2,haveged,python,secure-delete,sqlite3,apache2-mpm-worker,libapache2-mod-wsgi,libapache2-mod-xsendfile,redis-server,supervisor,securedrop-keyring,securedrop-config,libpython2.7-dev


libpython2.7-dev is in security lists 👍

redshiftzero · 2018-06-15T23:48:11Z

securedrop/models.py

@@ -31,6 +32,9 @@
 # precisely control which code paths are exercised.
 if os.environ.get('SECUREDROP_ENV') == 'test':
    LOGIN_HARDENING = False
+    ARGON2_PARAMS = dict(memory_cost=2**3, rounds=1, parallelism=1)


hey is this strictly necessary (i.e. did this provide a significant speedup)? I'm asking because having test-only code paths in prod code is in my opinion an anti-pattern and this if/else in particular is one that we can hopefully snip out some point soon

heartsucker@7bee4bf4317f:/home/heartsucker/code/freedomofpress/securedrop/securedrop$ python -m timeit 'from passlib.hash import argon2; argon2.using(memory_cost=2**3, rounds=1, parallelism=1).hash("test")' 10000 loops, best of 3: 189 usec per loop heartsucker@7bee4bf4317f:/home/heartsucker/code/freedomofpress/securedrop/securedrop$ python -m timeit 'from passlib.hash import argon2; argon2.using(memory_cost=2**16, rounds=4, parallelism=1).hash("test")' 10 loops, best of 3: 284 msec per loop

So I'm gonna say no, not necessary.

This was removed.

redshiftzero · 2018-06-15T23:51:33Z

securedrop/models.py

+            self.passphrase_hash = \
+                argon2.using(**ARGON2_PARAMS).hash(passphrase)
+            self.pw_hash = None
+            self.pw_salt = None


setting pw_salt to None is such an important implementation detail that it should have a comment to indicate to everyone that in passlib.hash.argon2 salts are auto-generated and stored in the passphrase_hash column

Fixed in both place.

redshiftzero · 2018-06-15T23:52:43Z

securedrop/qa_loader.py

@@ -64,6 +64,12 @@ def new_journalist():
                                         nullable=False),
                            pw,
                            random_bool())
+    if random_bool():
+        # to add legacy passwords back in


redshiftzero · 2018-06-20T04:55:12Z

Hey @heartsucker, can you confirm you were able to run through your test plan successfully? I'm having trouble upgrading in staging VMs:

# dpkg -i /root/securedrop-app-code-0.8.0~rc1-amd64.deb
(Reading database ... 51586 files and directories currently installed.)
Preparing to unpack .../securedrop-app-code-0.8.0~rc1-amd64.deb ...
Unpacking securedrop-app-code (0.8.0~rc1) over (0.8.0~rc1) ...
dpkg: dependency problems prevent configuration of securedrop-app-code:
 securedrop-app-code depends on libpython2.7-dev; however:
  Package libpython2.7-dev is not installed.

dpkg: error processing package securedrop-app-code (--install):
 dependency problems - leaving unconfigured
Errors were encountered while processing:
 securedrop-app-code
# apt-cache policy libpython2.7-dev
libpython2.7-dev:
  Installed: (none)
  Candidate: 2.7.6-8ubuntu0.4
  Version table:
     2.7.6-8ubuntu0.4 0
        500 http://security.ubuntu.com/ubuntu/ trusty-security/main amd64 Packages

heartsucker · 2018-06-20T12:16:44Z

@redshiftzero You are right. I was using a dirty vagrant box when I did that test. I pushed a change that should fix it.

heartsucker · 2018-06-20T16:14:20Z

Aww crap. I overwrote my fixes for your comments with a force push. This is gonna need a fix before you re-review.

conorsch · 2018-06-20T16:15:13Z

@heartsucker Recoverable via git reflog ?

heartsucker · 2018-06-21T06:28:58Z

@conorsch Possibly, but given that I don't access to one of the machines the code is on, fixing again is probably easier. :)

heartsucker · 2018-06-21T20:28:35Z

Can someone kick the CI job since I think this is a transient error with PyPI. It's been acting up with me today.

emkll

I've tested an upgrade scenario in staging VMs with this branch, and I can confirm that upon a new login, passwords are hashes and stored with argon2 in the passphrase_hash column, and that previous scrypt password/salts (pw_hash and pw_salt) are nulled in the database upon login of an existing journalist. The parameter selection makes sense to me. I have a couple of comments that I don't think should block merge but that we should consider in the near future:

A backup of a pre-migrated database causes an error 500 with an OperationalError: (sqlite3.OperationalError) no such column: journalists.passphrase_hash. Reinstalling (and therefore running the alembic migration again) fixes this problem. Perhaps reinstalling the securedrop-app-code package as part of the backup script might be an easy way to fix this and ensure forward compatibility for backups.
The argon2 hashes are stored in a newly created column, and the previous hash columns are nulled. It seems like a new column / migration is required if we decide to update parameters or algorithm. Would it make sense to create a column containing an identifier to the type of hash (and params)?
Finally, something we should at least be aware of is that passlib appears to be the work of a single developer, and there have been no commits in the past year.

I'll let someone else take one final look at this before merging, otherwise these changes (and the very careful tests) look good to me 👍 !

heartsucker · 2018-06-29T08:34:14Z

Perhaps reinstalling the securedrop-app-code package as part of the backup script might be an easy way to fix this and ensure forward compatibility for backups.

This being the back job that ansible does?

It seems like a new column / migration is required if we decide to update parameters or algorithm.

If we update the params, we can always do:

if self.passphrase_hash.startswith('$argon2i$known_old_args$'):
    new_hash = argon2.using(**ARGS).hash(submitted_pw)
else:
    new_hash = None

valid = argon2.verify(submitted_pw)
if valid and new_hash:
    self.passphrase_hash = new_hash

return valid

This works because the params are stored as part of the hash string:

$argon2i$v=19$m=65536,t=4,p=2$N+ac856zNuZ8r7U25nwvJQ$oBmgTQZXZvrM+nzpMyXyWw

Finally, something we should at least be aware of is that passlib appears to be the work of a single developer, and there have been no commits in the past year.

Should we reach out and discuss maintainability? Also for a lib like this, I imagine there's not a whole lot to do once the basic features are in place.

heartsucker · 2018-07-10T13:00:59Z

Booping this PR for final sign off.

zenmonkeykstop · 2018-07-12T21:13:48Z

Test plan fails for me at the vagrant provision /staging/ step - looks like libpython2-dev is not getting installed:

failed: [app-staging] (item=linux-image-generic-lts-xenial) => {"changed": false, "cmd": ["apt-get", "remove", "-y", "linux-image-generic-lts-xenial"], "delta": "0:00:00.551679", "end": "2018-07-12 21:07:06.030297", "item": "linux-image-generic-lts-xenial", "msg": "non-zero return code", "rc": 100, "start": "2018-07-12 21:07:05.478618", "stderr": "E: Unmet dependencies. Try 'apt-get -f install' with no packages (or specify a solution).", "stderr_lines": ["E: Unmet dependencies. Try 'apt-get -f install' with no packages (or specify a solution)."], "stdout": "Reading package lists...\nBuilding dependency tree...\nReading state information...\nPackage 'linux-image-generic-lts-xenial' is not installed, so not removed\nYou might want to run 'apt-get -f install' to correct these:\nThe following packages have unmet dependencies:\n securedrop-app-code : Depends: libpython2.7-dev but it is not going to be installed", "stdout_lines": ["Reading package lists...", "Building dependency tree...", "Reading state information...", "Package 'linux-image-generic-lts-xenial' is not installed, so not removed", "You might want to run 'apt-get -f install' to correct these:", "The following packages have unmet dependencies:", " securedrop-app-code : Depends: libpython2.7-dev but it is not going to be installed"]}

conorsch · 2018-07-25T18:44:12Z

Needs a rebase for conflicts with alembic, also for passing CI given merge of #3654. Following up on my previous comment about the liberal installation of dependencies, will test the proposed resolution locally and push up a patch if that resolves.

heartsucker · 2018-07-27T14:58:47Z

@conorsch @zenmonkeykstop I think we actually don't need the last commit I added that included the dependencies. We only need it for exactly the test case above when updating a staging box from a non-libpython2.7-dev-having VM to one that requires it. Adding a fix will just be cruft. I think it's sufficient to leave it out.

redshiftzero

this looks great. prior to merge we just need to:

get CI passing (see my two comments inline re: the tests CI job)
once that is good, @zenmonkeykstop can run through the upgrade testing scenario on this PR (this PR is a perfect candidate for the upgrade scenario ✨) and confirm upgrade is smooth

since my other PR created the need for the changes described inline, lmk if you want me to add a commit addressing the two comments described inline, happy to do so

redshiftzero · 2018-07-27T16:48:03Z

securedrop/alembic/versions/2d0ce3ee5bdc_added_passphrase_hash_column_to_.py

+
+# revision identifiers, used by Alembic.
+revision = '2d0ce3ee5bdc'
+down_revision = 'faac8092c123'


test CI job is failing here as down_revision needs to be updated to fccf57ceef02 after merge of #3619

redshiftzero · 2018-07-27T16:49:55Z

securedrop/tests/migrations/migration_2d0ce3ee5bdc.py

+            'flagged': bool_or_none(),
+            'last_updated': random_datetime(nullable=True),
+            'pending': bool_or_none(),
+            'interaction_count': random.randint(0, 1000),


and we'll need to add the UUID column here otherwise the unique constraint on UUID will cause an integrity error if UUID is null everywhere in the sources table

nothing breaks here, so we know this is safe to remove

this removes additional code paths that we shouldn't have to maintain

heartsucker · 2018-07-28T18:05:09Z

@redshiftzero @conorsch Ok thiiiiiiissss time I think I got it :D

redshiftzero · 2018-07-30T18:00:01Z

These changes look good, thanks @heartsucker!

When you get a chance @zenmonkeykstop can you give this an upgrade test? Once you confirm a smooth upgrade, I'll approve this for merge.

zenmonkeykstop · 2018-07-30T20:13:29Z

Tested as follows:

Cloned fresh copy of freedomofpress:securedrop and ran:

 make build-debs # needed or else apt server setup fails in upgrade scenario
 molecule converge -s upgrade

logged into app-staging VM and used manage.py to add 'admin' user
verified app version in journalist interface is 0.8.0
logged into journalist interface with admin user and created journo user (did not log in as journo)
switched to passlib branch and ran:

 make build-debs
 molecule side-effect -s upgrade

verified app version is 0.9.0~rc1
logged into journalist interface with journo user successfully
logged in with admin user successfully
created a new journo2 user, logged in as journo2 user successfully.

Looks good to me based on the above testing.

redshiftzero · 2018-07-30T20:14:13Z

Beautiful, thanks for the crystal clear report @zenmonkeykstop!

zenmonkeykstop · 2018-07-30T23:09:27Z

Egg on face time: I took a look at the app db after finishing testing and realized the passphrase_hash column hadn't been created. Looks like there was a flaw in my upgrade testing - the debs built on the develop branch before switching to the passlib branch were being used for the upgrade! Revised procedure is as follows:

On passlib branch, ran:

$ make build-debs 
$ molecule converge -s upgrade

logged into app-staging VM and used manage.py to add 'admin' user
logged into journalist interface with admin user, verified app version is 0.8.0, and created journo user (did NOT log in with journo)
Ran:

$ molecule side-effect -s upgrade

verified app version is 0.9.0~rc1
logged into journalist interface with journo user successfully
logged in with admin user successfully
created a new journo2 user, logged in as journo2 user successfully.
logged into app-staging VM, checked sqlite db, verified passphrase_hash field populated and pw_hash field empty for all users above

Luckily the results are the same, no functional change, but this time the db change is verified as well!

conorsch · 2018-07-30T23:13:46Z

Fantastic, thanks for following up, @zenmonkeykstop! Good to have the initial results confirmed now. N.B. I opened #3669 to track docs creating for the upgrade testing scenario. Had we had strong docs for that flow, we could have minimized the confusion around what specifically was being tested and how.

heartsucker requested a review from conorsch as a code owner June 8, 2018 16:51

heartsucker requested a review from a user June 8, 2018 16:51

heartsucker requested review from msheiny and redshiftzero as code owners June 8, 2018 16:51

ghost reviewed Jun 9, 2018

View reviewed changes

ghost added feature app labels Jun 9, 2018

heartsucker force-pushed the passlib branch from e94f8a3 to 85564e5 Compare June 10, 2018 13:34

eloquence added this to the 0.9 milestone Jun 13, 2018

redshiftzero reviewed Jun 15, 2018

View reviewed changes

heartsucker force-pushed the passlib branch from 85564e5 to b542cbd Compare June 17, 2018 17:13

heartsucker force-pushed the passlib branch from b542cbd to 5238190 Compare June 20, 2018 12:15

heartsucker force-pushed the passlib branch from 5238190 to ed39b65 Compare June 21, 2018 19:38

redshiftzero requested a review from emkll June 27, 2018 18:26

emkll reviewed Jun 28, 2018

View reviewed changes

heartsucker force-pushed the passlib branch from ed39b65 to 0abcd16 Compare July 3, 2018 09:19

heartsucker force-pushed the passlib branch from 178b918 to 5017aa4 Compare July 27, 2018 14:58

redshiftzero reviewed Jul 27, 2018

View reviewed changes

heartsucker added 13 commits July 28, 2018 18:13

added passlib and argon2_cffi as app dependencies

6bd9960

remove scrypt param injection

a75018c

nothing breaks here, so we know this is safe to remove

added argon2 support for password setting/verifying

dc9a31e

remove special cases of password handling in dev env

a2b22ab

this removes additional code paths that we shouldn't have to maintain

added db migration and tests for passphrase_hash

966ff62

added libpython2.7-dev to dependencies

58e4cb8

updated qa loader to account for legacy password cases

82eee17

added migration-on-login to new passphrase hashing method

dfa7482

tests for passphrase migration

4da3e08

added libpython2.7-dev to ansible apt dependencies

e901b4d

retarget down revision

0668ae2

updated test case and added uuid column

543ec3f

better schema comparison

1874ce7

heartsucker force-pushed the passlib branch from 5017aa4 to 1874ce7 Compare July 28, 2018 16:59

redshiftzero approved these changes Jul 30, 2018

View reviewed changes

redshiftzero merged commit d5bb54b into develop Jul 30, 2018

redshiftzero deleted the passlib branch July 30, 2018 20:15

redshiftzero mentioned this pull request Jul 31, 2018

Scrypt params are defined in multiple places #3051

Closed

legoktm mentioned this pull request Oct 19, 2022

Journalist passphrases should be hashed with Argon2id instead of Argon2i #6655

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added passlib for more flexible password hashing #3506

Added passlib for more flexible password hashing #3506

heartsucker commented Jun 8, 2018

codecov-io commented Jun 8, 2018 •

edited

Loading

heartsucker commented Jun 8, 2018

ghost Jun 9, 2018

heartsucker Jun 9, 2018

ghost Jun 9, 2018 •

edited by ghost

Loading

redshiftzero left a comment

redshiftzero Jun 14, 2018

redshiftzero Jun 15, 2018

heartsucker Jun 16, 2018

heartsucker Jun 17, 2018

redshiftzero Jun 15, 2018

heartsucker Jun 17, 2018

redshiftzero Jun 15, 2018

redshiftzero commented Jun 20, 2018

heartsucker commented Jun 20, 2018

heartsucker commented Jun 20, 2018

conorsch commented Jun 20, 2018

heartsucker commented Jun 21, 2018

heartsucker commented Jun 21, 2018

emkll left a comment

heartsucker commented Jun 29, 2018

heartsucker commented Jul 10, 2018

zenmonkeykstop commented Jul 12, 2018 •

edited

Loading

conorsch commented Jul 25, 2018

heartsucker commented Jul 27, 2018

redshiftzero left a comment

redshiftzero Jul 27, 2018

redshiftzero Jul 27, 2018

heartsucker commented Jul 28, 2018

redshiftzero commented Jul 30, 2018

zenmonkeykstop commented Jul 30, 2018

redshiftzero commented Jul 30, 2018

zenmonkeykstop commented Jul 30, 2018 •

edited

Loading

conorsch commented Jul 30, 2018

Added passlib for more flexible password hashing #3506

Added passlib for more flexible password hashing #3506

Conversation

heartsucker commented Jun 8, 2018

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the server application code:

If you made changes to documentation:

codecov-io commented Jun 8, 2018 • edited Loading

Codecov Report

heartsucker commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jun 9, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

redshiftzero 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

redshiftzero commented Jun 20, 2018

heartsucker commented Jun 20, 2018

heartsucker commented Jun 20, 2018

conorsch commented Jun 20, 2018

heartsucker commented Jun 21, 2018

heartsucker commented Jun 21, 2018

emkll left a comment

Choose a reason for hiding this comment

heartsucker commented Jun 29, 2018

heartsucker commented Jul 10, 2018

zenmonkeykstop commented Jul 12, 2018 • edited Loading

conorsch commented Jul 25, 2018

heartsucker commented Jul 27, 2018

redshiftzero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heartsucker commented Jul 28, 2018

redshiftzero commented Jul 30, 2018

zenmonkeykstop commented Jul 30, 2018

redshiftzero commented Jul 30, 2018

zenmonkeykstop commented Jul 30, 2018 • edited Loading

conorsch commented Jul 30, 2018

codecov-io commented Jun 8, 2018 •

edited

Loading

ghost Jun 9, 2018 •

edited by ghost

Loading

zenmonkeykstop commented Jul 12, 2018 •

edited

Loading

zenmonkeykstop commented Jul 30, 2018 •

edited

Loading