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 erlpass for calculating password hashes #1390

Merged
merged 14 commits into from Aug 31, 2016

Conversation

@mmzeeman
Copy link
Member

commented Aug 22, 2016

Fix #1386.

This improves the security of passwords stored by zotonic. It now uses bcrypt instead of salted sha1 password hashes. Passwords which currently use salted sha1 hashes are converted to bcrypt hashes after a user has successfully authenticated themselves.

@mention-bot

This comment has been minimized.

Copy link

commented Aug 22, 2016

@mmzeeman, thanks for your PR! By analyzing the annotation information on this pull request, we identified @arjan, @ArthurClemens and @willemdj to be potential reviewers

%% @doc Compare if a password is the same as a hash.
%% @spec hash_is_equal(Password, Hash) -> bool()
hash_is_equal(Pw, {bcrypt, Hash}) ->

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 22, 2016

Member

I think this duplication of @doc and @spec blocks is an accident? It’s causing the build failure on Travis.

And please write specs as -spec(...) so we can (later) use them with dialyzer.

This comment has been minimized.

Copy link
@mmzeeman

mmzeeman Aug 23, 2016

Author Member

Oops.. didn't spot that one before committing.

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 23, 2016

Member

No problem. If you fix it, I’m all in favour of merging this quickly so we can ship it with the next release.

Perhaps we should add a test though, as any mistakes in this change will cause users to be unable to log in.

This comment has been minimized.

Copy link
@mmzeeman

mmzeeman Aug 23, 2016

Author Member

Indeed, looking into that later today.

This comment has been minimized.

Copy link
@mmzeeman

mmzeeman Aug 23, 2016

Author Member

BTW, when I run the tests locally I see a lot of warnings. I'm not sure if this works or not.

(zotonic001_testsandbox@Edo)1> 10:07:52.494 [info] No action enabled for "growl"
(zotonic001_testsandbox@Edo)1> 10:07:52.494 [error] [testsandboxdb] Could not start mod_authentication: Missing dependencies: [acl]
(zotonic001_testsandbox@Edo)1> 10:07:52.494 [info] No action enabled for "growl"
(zotonic001_testsandbox@Edo)1> 10:07:52.494 [error] [testsandboxdb] Could not start mod_admin: Missing dependencies: [authentication]
(zotonic001_testsandbox@Edo)1> 10:07:52.494 [info] No action enabled for "growl"
(zotonic001_testsandbox@Edo)1> 10:07:52.494 [error] [testsandboxdb] Could not start mod_admin_identity: Missing dependencies: [admin]
(zotonic001_testsandbox@Edo)1> 10:07:52.494 [info] No action enabled for "growl"
(zotonic001_testsandbox@Edo)1> 10:07:52.494 [error] [testsandboxdb] Could not start mod_admin_category: Missing dependencies: [admin]
(zotonic001_testsandbox@Edo)1> 10:07:52.495 [info] No action enabled for "growl"
(zotonic001_testsandbox@Edo)1> 10:07:52.495 [error] [testsandboxdb] Could not start mod_admin_predicate: Missing dependencies: [admin]
(zotonic001_testsandbox@Edo)1> 10:07:52.495 [info] No action enabled for "growl"
(zotonic001_testsandbox@Edo)1> 10:07:52.495 [error] [testsandboxdb] Could not start mod_admin_modules: Missing dependencies: [admin]
(zotonic001_testsandbox@Edo)1> 10:07:52.495 [info] No action enabled for "growl"
(zotonic001_testsandbox@Edo)1> 10:07:52.495 [error] [testsandboxdb] Could not start mod_admin_config: Missing dependencies: [admin]

And in the end...

(zotonic001_testsandbox@Edo)1>   All 23 tests passed.
(zotonic001_testsandbox@Edo)1> Terminating due to shutdown
(zotonic001_testsandbox@Edo)1> 

Is that normal?

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 23, 2016

Member

testsandbox_basic_tests doesn’t use testsandboxdb but testsandbox.

*_db_tests seems to be excluded from the test run, probably because it requires a database. Is this the file that you were adding tests to?

Let’s solve this in #1392.

This comment has been minimized.

Copy link
@mmzeeman

mmzeeman Aug 23, 2016

Author Member

Thanks for finding this.

No, I first embedded simple unit tests in m_identity.erl only to find out these kind of tests are not run. Then I added a new test module named m_identity_db_tests.erl to src/models/tests because I also wanted to test the hash upgrading mechanism only to find out the tests implemented there are skipped.

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 29, 2016

Member

I fixed the tests in #1393, so if you rebase this on master, you can add your tests to src/tests.

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 30, 2016

Member

If you still have problems with running your test, please push it to this branch so I can have a look and help out.

This comment has been minimized.

Copy link
@mmzeeman

mmzeeman Aug 30, 2016

Author Member

Going to work on it right now.

@ddeboer ddeboer added this to the 0.20 milestone Aug 23, 2016
@ddeboer ddeboer changed the title Issue 1386 Use erlpass for calculating password hashes Aug 23, 2016
hash_is_equal(Pw, {hash, Salt, Hash}) ->
NewHash = crypto:hash(sha, [Salt, Pw]),
Hash =:= NewHash;
hash_is_equal(_, _) ->
false.


%% @doc Check if the password hash needs to be rehashed.
%% @spec needs_rehash(Hash) -> bool()

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 29, 2016

Member

Please use -spec needs_rehash(Hash) -> bool(). instead

This comment has been minimized.

Copy link
@mworrell

mworrell Aug 31, 2016

Member

I think we can do a single big update of specs?
Or at least let's add (and check) specs when we update Erlang modules.

BTW bool() should be boolean()

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 31, 2016

Member

Or at least let's add (and check) specs when we update Erlang modules.

Definitely! That’s why we’re nitpicking here on the spec definition that @mmzeeman wrote. 😄

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2016

Committed, but of course it has conflicts because of the recent cowboy branch merge.

@ddeboer

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

Merged master into this; let’s see whether we get a green light. 😉

@ddeboer ddeboer modified the milestones: 0.21, 0.20 Aug 30, 2016
@ddeboer ddeboer added the security label Aug 30, 2016

ok.

check_password_no_user() ->

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 30, 2016

Member

This is not run as it doesn’t have the _test function extension (and causes xref to complain about an unused function). Can you change that?

@ddeboer

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

Okay, merged master (including a fix to the tests after the Cowboy merge), so the failures shown here properly belong to m_identity_tests now.


MrYId = ensure_new_user("mr_y", "secret", AdminC),

?assertEqual({ok, MrYId}, m_identity:check_username_pw("mr_y", "secret", C)),

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 30, 2016

Member

This fails with {error,nouser}.

This comment has been minimized.

Copy link
@mmzeeman

mmzeeman Aug 30, 2016

Author Member

Strange, the tests run over here.

It looks like out of date output because of the module dependency warnings. I don't get those when I run the tests locally.

20:04:09.601 [error] FILEWATCHER: please install fswatch or inotify-tools to automatically load changed files
20:04:22.439 [warning] [testsandboxdb] Trying to insert duplicate uri <<"http://foo.com/id/333">>
  All 41 tests passed.
Terminating due to shutdown
@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2016

19:42:13.421 [info] DEBUG: m_identity_tests:51  [{1,1,<<"username_pw">>,<<"admin">>,true,false,undefined,{bcrypt,<<"$2a$12$m2m6y1ic9kS078J6nQyzeOka8vPKXYGwafH7QiEmyewSMeDIRxkYC">>},<<>>,<<>>,<<>>,{{2016,8,30},{19,41,59}},{{2016,8,30},{19,41,59}},undefined},{2,314,<<"username_pw">>,<<"mr_y">>,true,true,undefined,{bcrypt,<<"$2a$12$Acuirmi7xaAEBgdV3b8O5.YsF.PLZAiQUPTWmzuA856Y5eUlA1Ctu">>},<<>>,<<>>,<<>>,{{2016,8,30},{19,42,12}},{{2016,8,30},{19,42,12}},undefined}]
19:42:13.421 [info] DEBUG: m_identity_tests:52  [{1,1,<<"username_pw">>,<<"admin">>,true,false,undefined,{bcrypt,<<"$2a$12$m2m6y1ic9kS078J6nQyzeOka8vPKXYGwafH7QiEmyewSMeDIRxkYC">>},<<>>,<<>>,<<>>,{{2016,8,30},{19,41,59}},{{2016,8,30},{19,41,59}},undefined},{2,314,<<"username_pw">>,<<"mr_y">>,true,true,undefined,{bcrypt,<<"$2a$12$Acuirmi7xaAEBgdV3b8O5.YsF.PLZAiQUPTWmzuA856Y5eUlA1Ctu">>},<<>>,<<>>,<<>>,{{2016,8,30},{19,42,12}},{{2016,8,30},{19,42,12}},undefined}]
m_identity_tests: check_username_password_test...*failed*
in function m_identity_tests:'-check_username_password_test/0-fun-0-'/2 (/home/travis/build/zotonic/zotonic/_build/default/lib/zotonic/src/models/tests/m_identity_tests.erl, line 54)
in call from m_identity_tests:check_username_password_test/0 (/home/travis/build/zotonic/zotonic/_build/default/lib/zotonic/src/models/tests/m_identity_tests.erl, line 54)
**error:{assertEqual,[{module,m_identity_tests},
              {line,54},
              {expression,"m_identity : check_username_pw ( \"mr_y\" , \"secret\" , C )"},
              {expected,{ok,314}},
              {value,{error,nouser}}]}
  output:<<"">>

Weird, the check_username_pw returns nouser, but the identity table contains the user data.

@mworrell, do you have an idea what the problem might be? On my home and work machines the tests work just fine.

@@ -358,9 +364,9 @@ check_username_pw("admin", Password, Context) ->
{error, password}
end;
check_username_pw(Username, Password, Context) ->
Username1 = z_string:trim(z_string:to_lower(Username)),
Username1 = ?DEBUG(z_string:trim(z_string:to_lower(Username))),

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 31, 2016

Member

Interestingly, this value is a binary (not a ‘string’). Is that a problem?

This comment has been minimized.

Copy link
@mmzeeman

mmzeeman Aug 31, 2016

Author Member

No, mod_admin_identity was running on my local machine for testsandboxdb. It was probably started before the config changed to the minimal setup. The schema of the test site should be dropped before running the tests.

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2016

@ddeboer The tests fail because this test needs mod_admin_identity -> mod_admin -> mod_authentication -> acl module. We currently only have mod_acl_user_groups as acl module, which needs mod_content_group and other things.

What happens is that the tests start running before all modules are started.

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2016

@ddeboer To fix this for now I could add a dummy acl module to testsandboxdb so I can start mod_authentication without all the extra deps. But even then this is a race condition for all tests. The tests should just wait until all modules are started.

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2016

@ddeboer At some point in time mod_acl_user_groups must be tested too I guess.

@ddeboer

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

mod_acl_user_groups is already being tested (albeit minimally). I didn’t want to add the ACL modules to the default testsandboxdb modules, so I only start them in the ACL tests. You can use activate_await to start a Zotonic module in your test cases and wait for it to be started.

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2016

Check, didn't know that one. If that is the case then we just have to start mod_base I think, all the other modules have dependencies.

@ddeboer

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

True, but you can just leave the list of modules in config unchanged for now.

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2016

@ddeboer It is maddening.. The tests now run on a clean testsandboxdb schema, but... you still get warnings about mod_authentication not being started. The reason for this is that z_installer depends on mod_authentication... which needs an acl module...

Why does the installer need mod_authentication?

@ddeboer

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

Please push, so I can follow along. 😉

@ddeboer

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

Right, you’re talking about https://github.com/zotonic/zotonic/blob/master/src/install/z_installer.erl#L425. I guess this is a check that ensures that at least some authentication is going on in the site, to protect the data from being unintentionally publicly available.

@mworrell Is that correct? What about removing that check?

@mmzeeman We can fix this in a separate PR. If this one is green, we can merge it. 😉

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2016

Ok, now we have a timeout... is this progress?

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2016

Trying if I can run the tests without installing content groups

mod_base,
mod_authentication,
mod_admin
mod_base

This comment has been minimized.

Copy link
@ddeboer

ddeboer Aug 31, 2016

Member

Can you leave this unchanged for now?

This comment has been minimized.

Copy link
@mmzeeman

mmzeeman Aug 31, 2016

Author Member

Yup, I'll undo that.

@ddeboer

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

Possibly a Travis hick-up, as restarting the test makes it green.

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2016

In order to minimize the deps needed to run the password check database tests I created a mock acl module.

@mmzeeman

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2016

Now it fails on one of the mod_acl_user_group tests. Probably a race condition.

@ddeboer

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

Yeah, we get that one now and then on master, too, so not related to this PR. Just restarted the test.

@ddeboer ddeboer merged commit f5d31eb into master Aug 31, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ddeboer ddeboer deleted the issue-1386 branch Aug 31, 2016
@ddeboer

This comment has been minimized.

Copy link
Member

commented Aug 31, 2016

Also cherry-picked into 0.x.

ddeboer added a commit that referenced this pull request Aug 31, 2016
* core: Use bcrypt instead of salted sha1 hashes for passwords
* core: Added tests for auto upgrading password hashes
* core: Add mock acl module needed to run tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.