Add support for Bcrypt password hashing #1160

pierrekilly · 2018-02-12T22:14:51Z

Overview

Add support for Bcrypt password hashing.

Testing recommendations

Add the following configuration entry:

Section: couch_httpd_auth
Name: password_scheme
Value: bcrypt

Then you can add a user

curl -v -X PUT http://localhost:5984/_users/org.couchdb.user:jan -H "Accept: application/json" -H "Content-Type: application/json" -d '{"name": "jan", "password": "apple", "roles": [], "type": "user"}'

and check that this user can authenticate and that his password is hashed using bcrypt:

$ curl -v -X POST http://localhost:5984/_session -H "Accept: application/json" -H "Content-Type: application/json" -d '{"name": "jan", "password": "apple"}' 
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying ::1...
* TCP_NODELAY set
* connect to ::1 port 5984 failed: Connection refused
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 5984 (#0)
> POST /_session HTTP/1.1
> Host: localhost:5984
> User-Agent: curl/7.58.0
> Accept: application/json
> Content-Type: application/json
> Content-Length: 36
> 
* upload completely sent off: 36 out of 36 bytes
< HTTP/1.1 200 OK
< Set-Cookie: AuthSession=amFuOjVBODIwREYxOiXb3vYQn-vkNlUhKjx-P4hHO513; Version=1; Path=/; HttpOnly
< Server: CouchDB/2.2.0-3b53c1c92 (Erlang OTP/20)
< Date: Mon, 12 Feb 2018 21:58:09 GMT
< Content-Type: application/json
< Content-Length: 36
< Cache-Control: must-revalidate
< 
{"ok":true,"name":"jan","roles":[]}
* Connection #0 to host localhost left intact

$ curl -v -X GET http://localhost:5984/_session -H "Accept: application/json" -H "Cookie: AuthSession=amFuOjVBODIwREYxOiXb3vYQn-vkNlUhKjx-P4hHO513; Version=1; Path=/; HttpOnly"
Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying ::1...
* TCP_NODELAY set
* connect to ::1 port 5984 failed: Connection refused
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 5984 (#0)
> GET /_session HTTP/1.1
> Host: localhost:5984
> User-Agent: curl/7.58.0
> Accept: application/json
> Cookie: AuthSession=amFuOjVBODIwREYxOiXb3vYQn-vkNlUhKjx-P4hHO513; Version=1; Path=/; HttpOnly
> 
< HTTP/1.1 200 OK
< Set-Cookie: AuthSession=amFuOjVBODIwRTM0OmLuURvWLMnj_YNXuhae6UD3xHkh; Version=1; Path=/; HttpOnly
< Server: CouchDB/2.2.0-3b53c1c92 (Erlang OTP/20)
< Date: Mon, 12 Feb 2018 21:59:16 GMT
< Content-Type: application/json
< Content-Length: 158
< Cache-Control: must-revalidate
< 
{"ok":true,"userCtx":{"name":"jan","roles":[]},"info":{"authentication_db":"_users","authentication_handlers":["cookie","default"],"authenticated":"cookie"}}
* Connection #0 to host localhost left intact

Related Issues or Pull Requests

There will be a PR from the documentation changes from my fork: https://github.com/pierrekilly/couchdb-documentation/tree/bcrypt-hashing

Checklist

Code is written and works correctly;
Changes are covered by tests;
Documentation reflects the changes;
I have forked and updated the documentation but I have an issue with it: in the Hashing passwords section in the Security chapter I would like to add that starting from the next release bcrypt is supported but the next release doesn't exists yet, so the make html task fails. I still open this PR as I would like this code to be reviewed already.

wohali · 2018-02-13T02:23:12Z

For Apache CouchDB to accept this code, we will need to import the erlangpack/bcrypt repo and perform due diligence on the LICENSE file. At a glance, it looks OK - just saying that this won't hit master with a non-Apache hosted repo in rebar.config. This likely means your code would need to be merged to a feature branch, and have the stated concern worked there, before it would be merged to master.

I can't speak to the desirability or the technical efficacy of the patch; someone else will need to comment. I know there have been arguments on bcrypt vs. pbkdf2 in the past, but I can't remember where they went.

What motivated this work?

wohali · 2018-02-13T02:25:02Z

src/couch/src/couch_users_db.erl

@@ -75,7 +76,7 @@ save_doc(#doc{body={Body}} = Doc) ->
        Body3 = proplists:delete(?PASSWORD, Body2),
        Doc#doc{body={Body3}};
    {ClearPassword, "pbkdf2"} ->
-        Iterations = list_to_integer(config:get("couch_httpd_auth", "iterations", "1000")),
+        Iterations = list_to_integer(config:get("couch_httpd_auth", "iterations", "10000")),


You can't change the default here without good reason; there are likely reasons it is set to 1000 not 10000. It is also an unrelated change to the proposed addition of bcrypt, isn't it? This is the pbkdf2 section.

Any change to a config:get block must be matched with parallel changes to rel/overlay/etc/default.ini.

That's right. I simply noticed that the default value in the authenticate function in couch_httpd_auth.erl is 10000 so I thought this was a misspelling.
Wasn't aware of rel/overlay/etc/default.ini but I guess I should just roll back this change, should I?

wohali · 2018-02-13T02:26:31Z

src/couch/src/couch_passwords.erl

-        ++ Iterations).
+        ++ Iterations);
+hash_admin_password("bcrypt", ClearPassword) ->
+    Iterations = list_to_integer(config:get("couch_httpd_auth", "iterations", "12")),


You can't have different defaults for different algorithms. Either be consistent with the existing default value (which I believe is 10) or propose a change with justification.

Ho, I guess I should rename the parameter into log_rounds as the real amount if iterations performed is actually 2^log_rounds.
I'll change it.

wohali · 2018-02-13T02:27:02Z

src/couch/src/couch_users_db.erl

@@ -84,6 +85,13 @@ save_doc(#doc{body={Body}} = Doc) ->
        Body3 = ?replace(Body2, ?SALT, Salt),
        Body4 = proplists:delete(?PASSWORD, Body3),
        Doc#doc{body={Body4}};
+    {ClearPassword, "bcrypt"} ->
+        Iterations = list_to_integer(config:get("couch_httpd_auth", "iterations", "12")),


Same comment as before - inconsistent default vs. pbkdf2's default.

Same as before, will rename it as log_rounds to prevent misunderstanding.

pierrekilly · 2018-02-13T13:26:28Z

Thanks @wohali for your review! I have to say this is the first time I contribute to a big opensource project, and the first time I work with Erlang!

So If I understand well, I should at least do the following:

not use the default iterations that is used for PBKDF2 but create a new log_rounds config entry with its own default
rollback my change in pbkdf2's default iterations (if something should be fixed there, it should be fixed in a dedicated branch / commit / PR)

Once this is done, how should I proceed to work on a feature branch? I've followed the given instructions but maybe I made something wrong?

I don't have a strong opinion about bcrypt vs. pbkdf2. I know both are great with there pros and cons. I happened to use Bcrypt so far and I have several dozens of thousands of user accounts who's passwords are stored in my MySQL database and as I want to migrate to CouchDB (because it's awesome!), the best for me is to have it support Bcrypt, so I can just import my user accounts without requiring any actions from them.

wohali · 2018-02-13T15:19:19Z

Hi @pierrekilly ,

Oops, I wasn't clear in my comments, sorry about that.

You should use the same value for iterations for bcrypt and pbkdf2.

You should not change the default in the same commit. I see the advice that recommends not using less than 10 for the iterations value online, so I understand your going with 12, but since this will affect both algorithms, we need to be sure this will not cause undue concern for users of pbkdf2 which should remain the default.

If you change the default value you must update rel/overlay/etc/default.ini with the new default value as well. This file must always match the default values as specified in the code.

Someone else more focused on this section of the code will have to help this PR along further, I'm afraid. That may take some time. If it stalls out too long, you should subscribe to dev@couchdb.apache.org and send an email asking for someone to look over your contribution.

wohali · 2018-02-13T15:37:29Z

And because this is your first contribution - THANK YOU for contributing to CouchDB! We're always happy to have new collaborators on our project. :D

rnewson · 2018-02-18T13:00:26Z

src/couch/src/couch_httpd_auth.erl

@@ -309,7 +309,12 @@ handle_session_req(#httpd{method='POST', mochi_req=MochiReq}=Req, AuthModule) ->
            Secret = ?l2b(ensure_cookie_auth_secret()),
            UserSalt = couch_util:get_value(<<"salt">>, UserProps),
            CurrentTime = make_cookie_time(),
-            Cookie = cookie_auth_cookie(Req, ?b2l(UserName), <<Secret/binary, UserSalt/binary>>, CurrentTime),
+            Cookie = case UserSalt =:= undefined of


suggest case UserSalt of undefined -> ... ; _ ->.

rnewson · 2018-02-18T13:02:16Z

+1 from me though I made one style comment.

Before this merges though;

appropriate squashing
we'll need to create an apache mirror of the bcrypt repo and point to that.

janl · 2018-02-18T15:05:53Z

just saying that this won't hit master with a non-Apache hosted repo in rebar.config.

Quick expanding on this. We only include sources that we have a copy of, so we can ensure that we can make releases with all features in perpetuity.

That means if/when we accept this PR, we’ll keep a copy of erlang-bcrypt on ASF git infrastructure and this GitHub mirror.

This is something team is going to handle, so no additional requirement for you, @pierrekilly.

dch · 2018-02-19T19:17:48Z

@pierrekilly welcome & nice work on your first PR!

Some background info which may be useful to help you understand what happens next,
as your PR requires the project to include a 3rd party library. We already have quite a few
of these, so it's fine to do this but we need to take a few precautions.

Your tidy changes require a NIF (C code) addition to the CouchDB distribution, so
we need to make sure of 3 things:

that this runs on all our common platforms, from linux, windows and BSDs
the NIF interface doesn't adversely impact the VM's behaviour
that the licensing of that code, and its provenance, meets our standards

The Apache Software Foundation, as policy, releases sources artefact that contains
only ALv2 licensed components, or those that fall under a sufficiently "free"
licence that we can include it in a release -- there's a pre-approved list https://apache.org/legal/resolved.html#category-a here

@dch is happy to do the IP clearance dance assuming we're OK on the NIF side of things. Can anybody look into that? the NIF is an R13B04 era style, does this need any changes?

janl · 2018-02-20T19:40:18Z

@dch is happy to do the IP clearance dance assuming we're OK on the NIF side of things. Can anybody look into that? the NIF is an R13B04 era style, does this need any changes?

Thanks @dch, but this doesn’t require IP clearance, as we just keep a copy, we don’t intend to continue developing the dependency code and we don’t intend to slap our license on it. It just needs to satisfy category A as you point out. With the git mirror in place via @rnewson, this is good to go.

janl · 2018-02-23T13:41:53Z

We now have https://github.com/apache/couchdb-erlang-bcrypt

janl · 2018-02-23T14:07:54Z

I updated the dependency location and rebased master.

janl · 2018-02-23T14:11:49Z

Added LICENSE and NOTICE entries

janl · 2018-02-23T14:41:11Z

janl · 2018-02-25T16:59:20Z

@pierrekilly see https://github.com/apache/couchdb/blob/master/test/javascript/tests/users_db_security.js for getting started

pierrekilly · 2018-02-26T20:22:47Z

Just pushed more tests
A quick explanation about them: I have modified the current test in the file @janl pointed to be applied also to both pbkdf2 and bcrypt + I have added the third scenario asked.
Hope everything is fine but if there's any issue or work remaining, please let me know.

wohali · 2018-02-27T20:10:02Z

Great work here. I'm unable to comment on the code quality, but if @janl and @rnewson are happy, you've pleased the right people.

As someone else has probably mentioned, this needs to be squashed before landing.

Sorry to bring this up again... @janl and I discussed the whole iterations vs. log_rounds being the same or separate. I guess if both encryption types can be supported at once, it can make sense for them to be separate. But if only one type of encryption is supported at the time of creation of a new user, then the parameters would be best to be unified into the single iterations existing config value - the POLA comes into effect.

Jan said he'll think about this and drop by in a day or two to comment.

pierrekilly · 2018-02-27T20:44:24Z

Very well!

Just one precision about this: in case of PBKDF2, with iterations set to 10, there will be 10 iterations. In case of bcrypt, the same 10 means there will be 2^10 iterations.

As the number do not represent exactly the same thing, I preferred to keep them separated, but the decision is up to you guys :-)

nickva · 2018-02-27T21:53:22Z

@pierrekilly good point on it being a different types of parameter. It's not just linear difference. Maybe call it bcrypt_work_factor. In other words it seems work factor parameters are specific for the algorithm. scrypt in turn seems to have a factor which is a CPU/memory cost parameter which doesn't seem to exactly map to bcrypt rounds or iterations in PBKDF2

rnewson · 2018-02-27T23:34:22Z

definitely a separate parameter as they are not comparable.

pierrekilly · 2018-03-01T09:25:02Z

Ho, and I forgot to mention that logRounds is actually the name used in bcrypt itself, so that name seemd appropriate to me.

janl · 2018-03-08T15:35:40Z

I screwed up the branch here trying to resolve rebase conflicts, apologies. I’ll open a new one with a squashed commit.

wohali · 2018-07-18T16:08:58Z

@pierrekilly @janl unfortunately bcrypt is terminally broken on Windows, as it is missing the BSD libc extensions (queue.h) and a useful pthreads implementation.

Further, it seems to me that it makes no sense for our NIF to link to a function that is, itself, launching pthreads. Surely this will wreak havoc with the Erlang scheduler? /cc @davisp

I am unfortunately going to have to remove this functionality to unblock the 2.2 release for now, as we can't hold up the release waiting for a rewrite.

I have restored the bcrypt branch so that work can be ongoing for this in a future release. I will attempt to cherry-pick any bcrypt changes that landed post-bcrypt to that branch as a convenience.

Since both PRs have landed, I will open a new ticket for this issue to track work going forward.

wohali · 2018-07-18T16:17:32Z

The new issue is at #1455

wohali reviewed Feb 13, 2018

View reviewed changes

rnewson reviewed Feb 18, 2018

View reviewed changes

janl force-pushed the bcrypt-hashing branch from 8992f18 to 22f97c9 Compare February 23, 2018 14:07

janl added this to the 2.2.0 milestone Feb 28, 2018

janl closed this Mar 8, 2018

janl force-pushed the bcrypt-hashing branch from 3409203 to cd598d8 Compare March 8, 2018 14:50

janl mentioned this pull request Mar 8, 2018

Add bcrypt hashing option #1212

Merged

wohali mentioned this pull request Jul 18, 2018

Fix the Windows build #1449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Bcrypt password hashing #1160

Add support for Bcrypt password hashing #1160

pierrekilly commented Feb 12, 2018

wohali commented Feb 13, 2018

wohali Feb 13, 2018

pierrekilly Feb 13, 2018

wohali Feb 13, 2018

pierrekilly Feb 13, 2018 •

edited

wohali Feb 13, 2018

pierrekilly Feb 13, 2018 •

edited

pierrekilly commented Feb 13, 2018

wohali commented Feb 13, 2018

wohali commented Feb 13, 2018

rnewson Feb 18, 2018

rnewson commented Feb 18, 2018

janl commented Feb 18, 2018

dch commented Feb 19, 2018

janl commented Feb 20, 2018

janl commented Feb 23, 2018

janl commented Feb 23, 2018

janl commented Feb 23, 2018

janl commented Feb 23, 2018 •

edited

janl commented Feb 25, 2018

pierrekilly commented Feb 26, 2018

wohali commented Feb 27, 2018

pierrekilly commented Feb 27, 2018

nickva commented Feb 27, 2018

rnewson commented Feb 27, 2018

pierrekilly commented Mar 1, 2018

janl commented Mar 8, 2018

wohali commented Jul 18, 2018

wohali commented Jul 18, 2018

Add support for Bcrypt password hashing #1160

Add support for Bcrypt password hashing #1160

Conversation

pierrekilly commented Feb 12, 2018

Overview

Testing recommendations

Related Issues or Pull Requests

Checklist

wohali commented Feb 13, 2018

wohali Feb 13, 2018

Choose a reason for hiding this comment

pierrekilly Feb 13, 2018

Choose a reason for hiding this comment

wohali Feb 13, 2018

Choose a reason for hiding this comment

pierrekilly Feb 13, 2018 • edited

Choose a reason for hiding this comment

wohali Feb 13, 2018

Choose a reason for hiding this comment

pierrekilly Feb 13, 2018 • edited

Choose a reason for hiding this comment

pierrekilly commented Feb 13, 2018

wohali commented Feb 13, 2018

wohali commented Feb 13, 2018

rnewson Feb 18, 2018

Choose a reason for hiding this comment

rnewson commented Feb 18, 2018

janl commented Feb 18, 2018

dch commented Feb 19, 2018

janl commented Feb 20, 2018

janl commented Feb 23, 2018

janl commented Feb 23, 2018

janl commented Feb 23, 2018

janl commented Feb 23, 2018 • edited

janl commented Feb 25, 2018

pierrekilly commented Feb 26, 2018

wohali commented Feb 27, 2018

pierrekilly commented Feb 27, 2018

nickva commented Feb 27, 2018

rnewson commented Feb 27, 2018

pierrekilly commented Mar 1, 2018

janl commented Mar 8, 2018

wohali commented Jul 18, 2018

wohali commented Jul 18, 2018

pierrekilly Feb 13, 2018 •

edited

pierrekilly Feb 13, 2018 •

edited

janl commented Feb 23, 2018 •

edited