Merge "Handle empty token key files"

keystone/common/token_utils.py

@@ -167,6 +167,27 @@ class TokenUtils(object):
 
         LOG.info('Become a valid new key: %s', valid_key_file)
 
+    def _get_key_files(self, key_repo):
+        key_files = dict()
+        keys = dict()
+        for filename in os.listdir(key_repo):
+            path = os.path.join(key_repo, str(filename))
+            if os.path.isfile(path):
+                with open(path, 'r') as key_file:
+                    try:
+                        key_id = int(filename)
+                    except ValueError:  # nosec : name is not a number
+                        pass
+                    else:
+                        key = key_file.read()
+                        if len(key) == 0:
+                            LOG.warning('Ignoring empty key found in key '
+                                        'repository: %s', path)
+                            continue
+                        key_files[key_id] = path
+                        keys[key_id] = key
+        return key_files, keys
+
     def initialize_key_repository(self, keystone_user_id=None,
                                   keystone_group_id=None):
         """Create a key repository and bootstrap it with a key.
@@ -208,16 +229,7 @@ class TokenUtils(object):
 
         """
         # read the list of key files
-        key_files = dict()
-        for filename in os.listdir(self.key_repository):
-            path = os.path.join(self.key_repository, str(filename))
-            if os.path.isfile(path):
-                try:
-                    key_id = int(filename)
-                except ValueError:  # nosec : name isn't a number
-                    pass
-                else:
-                    key_files[key_id] = path
+        key_files, _ = self._get_key_files(self.key_repository)
 
         LOG.info('Starting key rotation with %(count)s key files: '
                  '%(list)s', {
@@ -277,18 +289,7 @@ class TokenUtils(object):
             return []
 
         # build a dictionary of key_number:encryption_key pairs
-        keys = dict()
-        for filename in os.listdir(self.key_repository):
-            path = os.path.join(self.key_repository, str(filename))
-            if os.path.isfile(path):
-                with open(path, 'r') as key_file:
-                    try:
-                        key_id = int(filename)
-                    except ValueError:  # nosec : filename isn't a number,
-                        # ignore this file since it's not a key.
-                        pass
-                    else:
-                        keys[key_id] = key_file.read()
+        _, keys = self._get_key_files(self.key_repository)
 
         if len(keys) != self.max_active_keys:
             # Once the number of keys matches max_active_keys, this log entry

keystone/tests/unit/token/test_fernet_provider.py

@@ -634,6 +634,24 @@ class TestFernetKeyRotation(unit.TestCase):
         # Assert that the key repository is now expanded.
         self.assertEqual(self.key_repository_size, 3)
 
+    def test_rotation_empty_file(self):
+        active_keys = 2
+        self.assertRepositoryState(expected_size=active_keys)
+        empty_file = os.path.join(CONF.fernet_tokens.key_repository, '2')
+        with open(empty_file, 'w'):
+            pass
+        key_utils = token_utils.TokenUtils(
+            CONF.fernet_tokens.key_repository,
+            CONF.fernet_tokens.max_active_keys,
+            'fernet_tokens'
+        )
+        # Rotate the keys to overwrite the empty file
+        key_utils.rotate_keys()
+        self.assertTrue(os.path.isfile(empty_file))
+        keys = key_utils.load_keys()
+        self.assertEqual(3, len(keys))
+        self.assertTrue(os.path.getsize(empty_file) > 0)
+
     def test_non_numeric_files(self):
         evil_file = os.path.join(CONF.fernet_tokens.key_repository, '99.bak')
         with open(evil_file, 'w'):
@@ -673,3 +691,16 @@ class TestLoadKeys(unit.TestCase):
         keys = key_utils.load_keys()
         self.assertEqual(2, len(keys))
         self.assertValidFernetKeys(keys)
+
+    def test_empty_files(self):
+        empty_file = os.path.join(CONF.fernet_tokens.key_repository, '2')
+        with open(empty_file, 'w'):
+            pass
+        key_utils = token_utils.TokenUtils(
+            CONF.fernet_tokens.key_repository,
+            CONF.fernet_tokens.max_active_keys,
+            'fernet_tokens'
+        )
+        keys = key_utils.load_keys()
+        self.assertEqual(2, len(keys))
+        self.assertValidFernetKeys(keys)

releasenotes/notes/bug-1728907-bab6769ab46bd8aa.yaml

@@ -0,0 +1,10 @@
+---
+fixes:
+  - |
+    [`bug 1728907 <https://bugs.launchpad.net/keystone/+bug/1728907>`_]
+    In some rare cases, an empty key file can get created within the fernet
+    key repository. When keystone tries to load the keys from disk, it will
+    fail with an invalid fernet key ValueError. Keystone now handles empty
+    key files when loading and rotating keys. If an empty file exists, it
+    will be ignored when loaded, reported as a warning in the log, and
+    overwritten with a valid key upon rotation.