From f2a210e3fe991885833a627a9e45b6f0e4669b34 Mon Sep 17 00:00:00 2001 From: Gage Hugo Date: Wed, 21 Feb 2018 15:45:17 -0600 Subject: [PATCH] Handle empty token key files 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. This change adds a check for empty files with a valid numerical name within the key repository when rotating keys and loading keys. If an empty file exists, it will be ignored when loading keys, reported in the logs, and overwritten with a valid key upon rotation. Change-Id: Ic19dd02d38e8f6a05c8951ec3dd13659aab98259 Closes-Bug: 1728907 --- keystone/common/token_utils.py | 45 ++++++++++--------- .../tests/unit/token/test_fernet_provider.py | 31 +++++++++++++ .../notes/bug-1728907-bab6769ab46bd8aa.yaml | 10 +++++ 3 files changed, 64 insertions(+), 22 deletions(-) create mode 100644 releasenotes/notes/bug-1728907-bab6769ab46bd8aa.yaml 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.