[Account plugin] add cache to authentication method #1413

Stanley · 2017-12-06T07:49:47Z

When debugging my application, I've noticed very poor performance when logged in using account plugin. It turns out that the biggest bottle neck is bcrypt and its hashpw method (https://github.com/Kinto/kinto/blob/master/kinto/plugins/accounts/authentication.py#L18). Making sure it is executed no more than once per request drastically reduces response times.

Before:

> ab -n 100 -A admin:admin 127.0.0.1:8888/v1/
This is ApacheBench, Version 2.3 <$Revision: 1807734 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient).....done


Server Software:        waitress
Server Hostname:        127.0.0.1
Server Port:            8888

Document Path:          /v1/
Document Length:        697 bytes

Concurrency Level:      1
Time taken for tests:   39.127 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      95200 bytes
HTML transferred:       69700 bytes
Requests per second:    2.56 [#/sec] (mean)
Time per request:       391.272 [ms] (mean)
Time per request:       391.272 [ms] (mean, across all concurrent requests)
Transfer rate:          2.38 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   387  391   4.8    390     427
Waiting:      386  391   4.8    390     427
Total:        387  391   4.8    390     427

Percentage of the requests served within a certain time (ms)
  50%    390
  66%    391
  75%    392
  80%    393
  90%    395
  95%    399
  98%    402
  99%    427
 100%    427 (longest request)

After:

> ab -n 100 -A admin:admin 127.0.0.1:8888/v1/
This is ApacheBench, Version 2.3 <$Revision: 1807734 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient).....done


Server Software:        waitress
Server Hostname:        127.0.0.1
Server Port:            8888

Document Path:          /v1/
Document Length:        697 bytes

Concurrency Level:      1
Time taken for tests:   0.392 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      95200 bytes
HTML transferred:       69700 bytes
Requests per second:    255.05 [#/sec] (mean)
Time per request:       3.921 [ms] (mean)
Time per request:       3.921 [ms] (mean, across all concurrent requests)
Transfer rate:          237.11 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     2    4   4.1      3      43
Waiting:        2    4   4.1      3      43
Total:          2    4   4.1      3      43

Percentage of the requests served within a certain time (ms)
  50%      3
  66%      4
  75%      4
  80%      4
  90%      5
  95%      5
  98%      6
  99%     43
 100%     43 (longest request)

Add documentation.
Add tests.
Add a changelog entry.
Add your name in the contributors file.
If you changed the HTTP API, update the API_VERSION constant and add an API changelog entry in the docs
If you added a new configuration setting, update the kinto.tpl file with it.

Natim · 2017-12-06T09:14:24Z

@Stanley thank you for investigating this. Are you willing to take over and fix the tests (basically remove the cache key when changing the password or deleting the account) or should we do it?

Stanley · 2017-12-06T10:27:07Z

Sure, I'll do it :)

Natim · 2017-12-06T13:45:11Z

Thanks, let me know if you need help or if you want to pair on it. I am really exited by the feature to land 👍

Stanley · 2017-12-07T08:04:32Z

Done. All tests are passing :) Anything else?

Natim · 2017-12-07T08:30:44Z

Can you add a line in the CHANGELOG and your name in the CONTRIBUTORS file?

Stanley · 2017-12-11T07:32:50Z

Done :)

gabisurita

Nice work! I recently notice this behavior when trying to switch from basicauth to the accounts plugin and to be honest I've been a long time trying to think of a solution. What worked for my was increasing the batch limit and holding more records at the client side.

I'm not by far a security expert, but in this case, don't we face the same security issue as we did by not rotating a key when using the embedded basicauth (see #691), but this time only at the cache layer? I mean, if an attacker gain access the cache layer and the HMAC secret, than we can have a password leakage.

I'm not saying this shouldn't land and to be honest I think that's a quite unlikely scenario. I'd be probably ok with running this in production, but maybe it would be safer to allow configuring and disabling this on settings.

gabisurita · 2017-12-11T23:39:22Z

kinto/plugins/accounts/authentication.py

+    # Check if password is valid (it is a very expensive computation)
+    if bcrypt.checkpw(pwd_str, hashed):  # Match!
+        # Remember result so we don't have to calculate bcrypt hash next time.
+        cache.set(cache_key, "1", ttl=30)  # Time to live: 30 seconds


I think it would be great to expose the TTL in settings.

Can you think of a use case for that?

One can increase this value to have more cache hits (e.g across periodic requests). Refreshing the TTL at each request would also be a great feature IMO.

Thought about it too, but couldn't figure out how to implement cache that would expire after one request. Any ideas?

Oh sorry @gabisurita I missunderstood your comment yeah definitely 👍

Natim · 2017-12-12T19:22:33Z

if an attacker gain access the cache layer and the HMAC secret, than we can have a password leakage.

No because it is a hmac and it is not reversible, however we can rotate the hmac key with this plugin since we can wipe the cache whenever we want.

gabisurita · 2017-12-13T01:00:43Z

@Natim Sorry, I wasn't clear at this point. We're not leaking raw passwords but since we have a constant key/salt, one can brute force it with a dictionary attack over the hash. The problem isn't about being reversible, but being deterministic. I agree having a rotating secret and wiping out the cache periodically is a nice way to go, but it stills doesn't fix this problem, as an attacker can have access to both the hash and the seed at the same time.

Natim · 2017-12-13T14:54:39Z

The problem isn't about being reversible, but being deterministic.

Oh right, we should use the username hash as the cache key and then have a random salt and a saltedhashed password as the value.

Thanks for detecting this!

almet · 2018-01-26T18:13:00Z

Probably this hasn't survived the end of year crazyness :-) Here's a little bump -- what's missing on this to go ahead?

Natim · 2018-01-26T18:25:03Z

I guess we should merge it !!

leplatrem · 2018-02-02T10:30:01Z

We can merge once the following is done:

unit tests that assert a cache key is set (or bcrypt is not called on second attempt)
unit tests that assert what happens when user/pass is changed/deleted
the TTL value in settings
a cache refresh on each request (basically call cache.set())

@Stanley are you still interested in working on this and go through those last steps? Let us know, maybe somebody can help and take over!

Thanks!

@leplatrem

@leplatrem review.

leplatrem

Thanks for bringing this back to life!

There are a few things to fix though :)

leplatrem · 2018-02-20T09:45:03Z

kinto/plugins/accounts/authentication.py

+    settings = request.registry.settings
+    hmac_secret = settings['userid_hmac_secret']
+    cache_key = utils.hmac_digest(hmac_secret, ACCOUNT_CACHE_KEY.format(username))
+    cache_ttl = settings.get('account_cache_ttl_seconds', 30)


This setting should be documented and could be added to settings template IMO

I guess you mean that https://github.com/Kinto/kinto/pull/1413/files#diff-2193261fa1533ff793bf90b447434bc8R32 is not sufficient?

leplatrem · 2018-02-20T09:45:46Z

kinto/plugins/accounts/authentication.py

+    cache = request.registry.cache
+    cache_result = cache.get(cache_key)
+
+    # Username and password is correct. No need to compare hashes


I don't think is correct applies here. It would probably be was already verified

leplatrem · 2018-02-20T09:46:07Z

kinto/plugins/accounts/authentication.py

+
+    # Username and password is correct. No need to compare hashes
+    if cache_result:
+        cache.expire(cache_key, cache_ttl)


Add a comment to say we refresh the cache ttl on each request?

leplatrem · 2018-02-20T09:47:49Z

kinto/plugins/accounts/views.py

+    cache = request.registry.cache
+    settings = request.registry.settings
+    # Extract username and password from current user
+    username, password = extract_http_basic_credentials(request)


Shouldn't it be request.matchdict['id'] instead ? Since we make sure they match in the resource code?

leplatrem · 2018-02-20T09:48:17Z

tests/plugins/test_accounts.py

+
+            mocked_bcrypt.checkpw.assert_called_once()
+
+    def test_authentication_check_bcrypt_again_if_password_changes(self):


nit: checks

leplatrem · 2018-02-20T09:51:16Z

tests/plugins/test_accounts.py

+        resp = self.app.get('/', headers=get_user_headers('me', 'bouh'))
+        assert resp.json['user']['id'] == 'account:me'
+
+        time.sleep(2)


We should not slow the tests by two seconds here, we can do better like call .cache.expire(cache_key, 1) first and then make sure it's >1 or whatever

I don't understand how setting cache.expire helps here can you elaborate?

leplatrem · 2018-02-20T09:52:10Z

CHANGELOG.rst

@@ -9,6 +9,7 @@ This document describes changes between each past release.
 **New features**

 - Add Openid connect support (#939, #1425). See `demo <https://github.com/leplatrem/kinto-oidc-demo>`_
+- Account plugin now caches authentication method (#1413)


Method? Hmm, I would say verification

leplatrem · 2018-02-20T10:43:11Z

docs/api/1.x/accounts.rst

@@ -28,6 +28,9 @@ Add the following settings to the ``.ini`` file:
    # Allow anyone to create accounts.
    kinto.account_create_principals = system.Everyone

+    # Set the session time to live in seconds for the session


nit: session time and for the session does not sound super clear in the same sentence :)

Natim · 2018-02-20T14:04:41Z

Thanks a lot @Stanley for starting this !!!

Account plugin: add cache to authentication method

120f3b5

Stanley added 3 commits December 6, 2017 16:24

Account plugin: remove cache on password change (Kinto#1413)

6428df4

Account plugin: skip check for password change

ab774d6

Accoutn plugin: fix flake8 errors (Kinto#1413)

c43ebce

Update CHANGELOG

95a4df0

gabisurita reviewed Dec 11, 2017

View reviewed changes

leplatrem added the missing-tests label Feb 2, 2018

Rémy HUBSCHER added 2 commits February 20, 2018 10:40

Add tests.

00ef4bf

@leplatrem review.

Update from master

2d111c0

Natim requested a review from leplatrem February 20, 2018 09:41

leplatrem requested changes Feb 20, 2018

View reviewed changes

@leplatrem review.

04d9833

leplatrem approved these changes Feb 20, 2018

View reviewed changes

Fix flake8 and comment.

76d0f3f

Natim merged commit f564ed9 into Kinto:master Feb 20, 2018

leplatrem mentioned this pull request Apr 5, 2018

Account verification and cache #1583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Account plugin] add cache to authentication method #1413

[Account plugin] add cache to authentication method #1413

Stanley commented Dec 6, 2017

Natim commented Dec 6, 2017

Stanley commented Dec 6, 2017

Natim commented Dec 6, 2017

Stanley commented Dec 7, 2017

Natim commented Dec 7, 2017 •

edited

Loading

Stanley commented Dec 11, 2017

gabisurita left a comment •

edited

Loading

gabisurita Dec 11, 2017

Natim Dec 12, 2017

Natim Dec 12, 2017

gabisurita Dec 13, 2017 •

edited

Loading

Stanley Dec 13, 2017

Natim Jan 26, 2018 •

edited

Loading

Natim commented Dec 12, 2017

gabisurita commented Dec 13, 2017 •

edited

Loading

Natim commented Dec 13, 2017 •

edited

Loading

almet commented Jan 26, 2018

Natim commented Jan 26, 2018

leplatrem commented Feb 2, 2018 •

edited by Natim

Loading

leplatrem left a comment

leplatrem Feb 20, 2018

Natim Feb 20, 2018

leplatrem Feb 20, 2018

leplatrem Feb 20, 2018

leplatrem Feb 20, 2018

leplatrem Feb 20, 2018

leplatrem Feb 20, 2018

Natim Feb 20, 2018

Natim Feb 20, 2018

leplatrem Feb 20, 2018

leplatrem Feb 20, 2018

Natim commented Feb 20, 2018


		mocked_bcrypt.checkpw.assert_called_once()

		def test_authentication_check_bcrypt_again_if_password_changes(self):

[Account plugin] add cache to authentication method #1413

[Account plugin] add cache to authentication method #1413

Conversation

Stanley commented Dec 6, 2017

Before:

After:

Natim commented Dec 6, 2017

Stanley commented Dec 6, 2017

Natim commented Dec 6, 2017

Stanley commented Dec 7, 2017

Natim commented Dec 7, 2017 • edited Loading

Stanley commented Dec 11, 2017

gabisurita left a comment • edited Loading

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

gabisurita Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Natim Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Natim commented Dec 12, 2017

gabisurita commented Dec 13, 2017 • edited Loading

Natim commented Dec 13, 2017 • edited Loading

almet commented Jan 26, 2018

Natim commented Jan 26, 2018

leplatrem commented Feb 2, 2018 • edited by Natim Loading

leplatrem 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Natim commented Feb 20, 2018

Natim commented Dec 7, 2017 •

edited

Loading

gabisurita left a comment •

edited

Loading

gabisurita Dec 13, 2017 •

edited

Loading

Natim Jan 26, 2018 •

edited

Loading

gabisurita commented Dec 13, 2017 •

edited

Loading

Natim commented Dec 13, 2017 •

edited

Loading

leplatrem commented Feb 2, 2018 •

edited by Natim

Loading