Memoize calls to bcrypt.checkpw

This intentionally high CPU overhead function is called for every API
and JSON-RPC request when Basic HTTP authentication is enabled. With the
recent indirection enablement this is causing a performance regression
for Metal3 due to the extra JSON-RPC calls. This change would improve
the performance of all branches of Metal3 if backported.

Change-Id: I2740035d2882aacddca9c541362d6e533140650f
Closes-Bug: #2121105
Signed-off-by: Steve Baker <sbaker@redhat.com>

ironic/common/auth_basic.py

@@ -15,6 +15,7 @@
 
 import base64
 import binascii
+import functools
 import logging
 
 import bcrypt
@@ -91,6 +92,16 @@ def authenticate(auth_file, username, password):
     unauthorized()
 
 
+@functools.lru_cache(maxsize=256)
+def _checkpw(password, hashed):
+    """Wrapped bcrypt.checkpw for caching
+
+    Keep an in-memory cache of bcrypt.checkpw responses to avoid the
+    high CPU cost of repeatedly checking the same values
+    """
+    return bcrypt.checkpw(password, hashed)
+
+
 def auth_entry(entry, password):
     """Compare a password with a single user auth file entry
 
@@ -102,7 +113,7 @@ def auth_entry(entry, password):
     """
     username, encrypted = parse_entry(entry)
 
-    if not bcrypt.checkpw(password, encrypted):
+    if not _checkpw(password, encrypted):
         LOG.info('Password for %s does not match', username)
         unauthorized()
 

ironic/tests/unit/api/test_middleware.py

@@ -15,15 +15,16 @@
 Tests to assert that various incorporated middleware works as expected.
 """
 
+import bcrypt
 from http import client as http_client
 import os
 import tempfile
+from unittest import mock
 
 from oslo_config import cfg
 import oslo_middleware.cors as cors_middleware
 
 from ironic.tests.unit.api import base
-from ironic.tests.unit.api import utils
 from ironic.tests.unit.db import utils as db_utils
 
 
@@ -132,7 +133,7 @@ class TestBasicAuthMiddleware(base.BaseApiTest):
 
     def setUp(self):
         super(TestBasicAuthMiddleware, self).setUp()
-        self.environ = {'fake.cache': utils.FakeMemcache()}
+        self.environ = {}
         self.fake_db_node = db_utils.get_test_node(chassis_id=None)
 
     def test_not_authenticated(self):
@@ -148,6 +149,20 @@ class TestBasicAuthMiddleware(base.BaseApiTest):
         response = self.get_json('/chassis', headers=auth_header)
         self.assertEqual({'chassis': []}, response)
 
+    @mock.patch.object(bcrypt, 'checkpw', autospec=True)
+    def test_authenticated_cached(self, mock_checkpw):
+        def checkpw(password, hashed):
+            return password == b'myPassword'
+        mock_checkpw.side_effect = checkpw
+
+        auth_header = {'Authorization': 'Basic bXlOYW1lOm15UGFzc3dvcmQ='}
+        for i in range(10):
+            response = self.get_json('/chassis', headers=auth_header)
+            self.assertEqual({'chassis': []}, response)
+        # call count will be zero or one, depending on already cached
+        # results from other tests
+        self.assertLessEqual(mock_checkpw.call_count, 1)
+
     def test_public_unauthenticated(self):
         response = self.get_json('/')
         self.assertEqual('v1', response['id'])

releasenotes/notes/bcrypt_cache-d78775ff02f2d970.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    Performance of Basic HTTP authentication has been improved by keeping a
+    memory cache of bcrypt password checks. This improves the performance of
+    Ironic conductor with JSON-RPC, and API access when using Basic HTTP
+    authentication.