diff --git a/keystone/common/token_utils.py b/keystone/common/token_utils.py index cc176b4aa4..4ff6dff6c4 100644 --- a/keystone/common/token_utils.py +++ b/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 diff --git a/keystone/tests/unit/token/test_fernet_provider.py b/keystone/tests/unit/token/test_fernet_provider.py index 2c91603d20..ab2ca066d6 100644 --- a/keystone/tests/unit/token/test_fernet_provider.py +++ b/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) diff --git a/releasenotes/notes/bug-1728907-bab6769ab46bd8aa.yaml b/releasenotes/notes/bug-1728907-bab6769ab46bd8aa.yaml new file mode 100644 index 0000000000..f2c89d8300 --- /dev/null +++ b/releasenotes/notes/bug-1728907-bab6769ab46bd8aa.yaml @@ -0,0 +1,10 @@ +--- +fixes: + - | + [`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.