From 7e33521a39d5b7478008e5d6ba4c754857edbee9 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Mon, 14 Nov 2016 14:29:17 +0000 Subject: [PATCH] encryptors: Workaround mangled passphrases Prior to Ib563b0ea the passphrase used by CryptsetupEncryptor and LuksEncryptor had any leading zeros per hexadecimal digit removed, for example 0x04 or 04 would turn into 0x4 or 4. As a result any volume encrypted prior to the release of Newton used a modified passphrase that was different to that stored by the key manager being used in the environment. To correct this for LuksEncryptor volumes permission denied errors are now caught when attempting to open a volume. A second attempt to open the volume is then made using a mangled passphrase. If successful the correct passphrase is then added to the volume before the mangled passphrase is finally removed. This workaround can be removed in a future release once it is safe to assume that all LuksEncryptor volumes have had any mangled passphrases replaced in this way. This isn't possible for CryptsetupEncryptor volumes as the plain mode used by cryptsetup does not provide a way for adding and removing keys. As such on a permission denied error a second attempt is made to open the volume using a mangled passphrase. Unlike the above workaround this cannot be removed in a future release. Change-Id: I7096463c5eba951dd6322ee6965435e877ca0371 Partial-bug: #1633518 --- os_brick/encryptors/cryptsetup.py | 25 +++++- os_brick/encryptors/luks.py | 48 ++++++++++ os_brick/tests/encryptors/test_base.py | 4 - os_brick/tests/encryptors/test_cryptsetup.py | 45 ++++++++-- os_brick/tests/encryptors/test_luks.py | 95 ++++++++++++++++++-- 5 files changed, 196 insertions(+), 21 deletions(-) diff --git a/os_brick/encryptors/cryptsetup.py b/os_brick/encryptors/cryptsetup.py index 701ffd358..37439e56f 100644 --- a/os_brick/encryptors/cryptsetup.py +++ b/os_brick/encryptors/cryptsetup.py @@ -14,12 +14,13 @@ # under the License. +import array import binascii import os from os_brick.encryptors import base from os_brick import exception -from os_brick.i18n import _LW +from os_brick.i18n import _LW, _LI from oslo_concurrency import processutils from oslo_log import log as logging @@ -127,6 +128,17 @@ class CryptsetupEncryptor(base.VolumeEncryptor): check_exit_code=True, run_as_root=True, root_helper=self._root_helper) + def _get_mangled_passphrase(self, key): + """Convert the raw key into a list of unsigned int's and then a string + """ + # NOTE(lyarwood): This replicates the methods used prior to Newton to + # first encode the passphrase as a list of unsigned int's before + # decoding back into a string. This method strips any leading 0's + # of the resulting hex digit pairs, resulting in a different + # passphrase being returned. + encoded_key = array.array('B', key).tolist() + return ''.join(hex(x).replace('0x', '') for x in encoded_key) + def attach_volume(self, context, **kwargs): """Shadows the device and passes an unencrypted version to the instance. @@ -139,7 +151,16 @@ class CryptsetupEncryptor(base.VolumeEncryptor): key = self._get_key(context).get_encoded() passphrase = self._get_passphrase(key) - self._open_volume(passphrase, **kwargs) + try: + self._open_volume(passphrase, **kwargs) + except processutils.ProcessExecutionError as e: + if e.exit_code == 2: + # NOTE(lyarwood): Workaround bug#1633518 by attempting to use + # a mangled passphrase to open the device.. + LOG.info(_LI("Unable to open %s with the current passphrase, " + "attempting to use a mangled passphrase to open " + "the volume."), self.dev_path) + self._open_volume(self._get_mangled_passphrase(key), **kwargs) # modify the original symbolic link to refer to the decrypted device self._execute('ln', '--symbolic', '--force', diff --git a/os_brick/encryptors/luks.py b/os_brick/encryptors/luks.py index dd0087a4a..cae133fbe 100644 --- a/os_brick/encryptors/luks.py +++ b/os_brick/encryptors/luks.py @@ -100,6 +100,44 @@ class LuksEncryptor(cryptsetup.CryptsetupEncryptor): run_as_root=True, check_exit_code=True, root_helper=self._root_helper) + def _unmangle_volume(self, key, passphrase, **kwargs): + """Workaround for bug#1633518 + + First identify if a mangled passphrase is used and if found then + replace with the correct unmangled version of the passphrase. + """ + mangled_passphrase = self._get_mangled_passphrase(key) + self._open_volume(mangled_passphrase, **kwargs) + self._close_volume(**kwargs) + LOG.debug("%s correctly opened with a mangled passphrase, replacing " + "this with the original passphrase", self.dev_path) + + # NOTE(lyarwood): Now that we are sure that the mangled passphrase is + # used attempt to add the correct passphrase before removing the + # mangled version from the volume. + + # luksAddKey currently prompts for the following input : + # Enter any existing passphrase: + # Enter new passphrase for key slot: + # Verify passphrase: + self._execute('cryptsetup', 'luksAddKey', self.dev_path, + process_input=''.join([mangled_passphrase, '\n', + passphrase, '\n', passphrase]), + run_as_root=True, check_exit_code=True, + root_helper=self._root_helper) + + # Verify that we can open the volume with the current passphrase + # before removing the mangled passphrase. + self._open_volume(passphrase, **kwargs) + self._close_volume(**kwargs) + + # luksRemoveKey only prompts for the key to remove. + self._execute('cryptsetup', 'luksRemoveKey', self.dev_path, + process_input=mangled_passphrase, + run_as_root=True, check_exit_code=True, + root_helper=self._root_helper) + LOG.debug("%s mangled passphrase successfully replaced", self.dev_path) + def attach_volume(self, context, **kwargs): """Shadows the device and passes an unencrypted version to the instance. @@ -125,6 +163,16 @@ class LuksEncryptor(cryptsetup.CryptsetupEncryptor): self.dev_path) self._format_volume(passphrase, **kwargs) self._open_volume(passphrase, **kwargs) + elif e.exit_code == 2: + # NOTE(lyarwood): Workaround bug#1633518 by replacing any + # mangled passphrases that are found on the volume. + # TODO(lyarwood): Remove workaround during R. + LOG.warning(_LW("%s is not usable with the current " + "passphrase, attempting to use a mangled " + "passphrase to open the volume."), + self.dev_path) + self._unmangle_volume(key, passphrase, **kwargs) + self._open_volume(passphrase, **kwargs) else: raise diff --git a/os_brick/tests/encryptors/test_base.py b/os_brick/tests/encryptors/test_base.py index 3d6df6d83..b9d8d19ed 100644 --- a/os_brick/tests/encryptors/test_base.py +++ b/os_brick/tests/encryptors/test_base.py @@ -13,9 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. -import array from castellan.tests.unit.key_manager import fake -import codecs import mock from os_brick import encryptors @@ -35,8 +33,6 @@ class VolumeEncryptorTestCase(base.TestCase): ":volume-fake_uuid-lun-1", }, } - _hex = codecs.getdecoder("hex_codec")('0' * 32)[0] - self.encryption_key = array.array('B', _hex).tolist() self.root_helper = None self.keymgr = fake.fake_api() self.encryptor = self._create() diff --git a/os_brick/tests/encryptors/test_cryptsetup.py b/os_brick/tests/encryptors/test_cryptsetup.py index d4ec5c87c..c9abdff7f 100644 --- a/os_brick/tests/encryptors/test_cryptsetup.py +++ b/os_brick/tests/encryptors/test_cryptsetup.py @@ -18,17 +18,18 @@ import binascii import copy import mock import six +import uuid from castellan.common.objects import symmetric_key as key from castellan.tests.unit.key_manager import fake from os_brick.encryptors import cryptsetup from os_brick import exception from os_brick.tests.encryptors import test_base +from oslo_concurrency import processutils as putils -def fake__get_key(context): - raw = bytes(binascii.unhexlify('0' * 32)) - +def fake__get_key(context, passphrase): + raw = bytes(binascii.unhexlify(passphrase)) symmetric_key = key.SymmetricKey('AES', len(raw) * 8, raw) return symmetric_key @@ -38,8 +39,8 @@ class CryptsetupEncryptorTestCase(test_base.VolumeEncryptorTestCase): @mock.patch('os.path.exists', return_value=False) def _create(self, mock_exists): return cryptsetup.CryptsetupEncryptor( - root_helper=self.root_helper, connection_info=self.connection_info, + root_helper=self.root_helper, keymgr=self.keymgr) def setUp(self): @@ -64,14 +65,15 @@ class CryptsetupEncryptorTestCase(test_base.VolumeEncryptorTestCase): @mock.patch('os_brick.executor.Executor._execute') def test_attach_volume(self, mock_execute): + fake_key = uuid.uuid4().hex self.encryptor._get_key = mock.MagicMock() - self.encryptor._get_key.return_value = fake__get_key(None) + self.encryptor._get_key.return_value = fake__get_key(None, fake_key) self.encryptor.attach_volume(None) mock_execute.assert_has_calls([ mock.call('cryptsetup', 'create', '--key-file=-', self.dev_name, - self.dev_path, process_input='0' * 32, + self.dev_path, process_input=fake_key, root_helper=self.root_helper, run_as_root=True, check_exit_code=True), mock.call('ln', '--symbolic', '--force', @@ -153,3 +155,34 @@ class CryptsetupEncryptorTestCase(test_base.VolumeEncryptorTestCase): mock.call('/dev/mapper/%s' % wwn)]) mock_execute.assert_called_once_with( 'cryptsetup', 'status', wwn, run_as_root=True) + + @mock.patch('os_brick.executor.Executor._execute') + def test_attach_volume_unmangle_passphrase(self, mock_execute): + fake_key = '0725230b' + fake_key_mangled = '72523b' + self.encryptor._get_key = mock.MagicMock() + self.encryptor._get_key.return_value = fake__get_key(None, fake_key) + + mock_execute.side_effect = [ + putils.ProcessExecutionError(exit_code=2), # luksOpen + mock.DEFAULT, + mock.DEFAULT, + ] + + self.encryptor.attach_volume(None) + + mock_execute.assert_has_calls([ + mock.call('cryptsetup', 'create', '--key-file=-', self.dev_name, + self.dev_path, process_input=fake_key, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + mock.call('cryptsetup', 'create', '--key-file=-', self.dev_name, + self.dev_path, process_input=fake_key_mangled, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + mock.call('ln', '--symbolic', '--force', + '/dev/mapper/%s' % self.dev_name, self.symlink_path, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + ]) + self.assertEqual(3, mock_execute.call_count) diff --git a/os_brick/tests/encryptors/test_luks.py b/os_brick/tests/encryptors/test_luks.py index 76b99c397..1c3307797 100644 --- a/os_brick/tests/encryptors/test_luks.py +++ b/os_brick/tests/encryptors/test_luks.py @@ -13,9 +13,12 @@ # License for the specific language governing permissions and limitations # under the License. - +import binascii import mock +import uuid + +from castellan.common.objects import symmetric_key as key from os_brick.encryptors import luks from os_brick.tests.encryptors import test_cryptsetup from oslo_concurrency import processutils as putils @@ -79,15 +82,16 @@ class LuksEncryptorTestCase(test_cryptsetup.CryptsetupEncryptorTestCase): @mock.patch('os_brick.executor.Executor._execute') def test_attach_volume(self, mock_execute): + fake_key = uuid.uuid4().hex self.encryptor._get_key = mock.MagicMock() self.encryptor._get_key.return_value = ( - test_cryptsetup.fake__get_key(None)) + test_cryptsetup.fake__get_key(None, fake_key)) self.encryptor.attach_volume(None) mock_execute.assert_has_calls([ mock.call('cryptsetup', 'luksOpen', '--key-file=-', self.dev_path, - self.dev_name, process_input='0' * 32, + self.dev_name, process_input=fake_key, root_helper=self.root_helper, run_as_root=True, check_exit_code=True), mock.call('ln', '--symbolic', '--force', @@ -98,9 +102,10 @@ class LuksEncryptorTestCase(test_cryptsetup.CryptsetupEncryptorTestCase): @mock.patch('os_brick.executor.Executor._execute') def test_attach_volume_not_formatted(self, mock_execute): + fake_key = uuid.uuid4().hex self.encryptor._get_key = mock.MagicMock() self.encryptor._get_key.return_value = ( - test_cryptsetup.fake__get_key(None)) + test_cryptsetup.fake__get_key(None, fake_key)) mock_execute.side_effect = [ putils.ProcessExecutionError(exit_code=1), # luksOpen @@ -114,18 +119,18 @@ class LuksEncryptorTestCase(test_cryptsetup.CryptsetupEncryptorTestCase): mock_execute.assert_has_calls([ mock.call('cryptsetup', 'luksOpen', '--key-file=-', self.dev_path, - self.dev_name, process_input='0' * 32, + self.dev_name, process_input=fake_key, root_helper=self.root_helper, run_as_root=True, check_exit_code=True), mock.call('cryptsetup', 'isLuks', '--verbose', self.dev_path, root_helper=self.root_helper, run_as_root=True, check_exit_code=True), mock.call('cryptsetup', '--batch-mode', 'luksFormat', - '--key-file=-', self.dev_path, process_input='0' * 32, + '--key-file=-', self.dev_path, process_input=fake_key, root_helper=self.root_helper, run_as_root=True, check_exit_code=True, attempts=3), mock.call('cryptsetup', 'luksOpen', '--key-file=-', self.dev_path, - self.dev_name, process_input='0' * 32, + self.dev_name, process_input=fake_key, root_helper=self.root_helper, run_as_root=True, check_exit_code=True), mock.call('ln', '--symbolic', '--force', @@ -136,9 +141,10 @@ class LuksEncryptorTestCase(test_cryptsetup.CryptsetupEncryptorTestCase): @mock.patch('os_brick.executor.Executor._execute') def test_attach_volume_fail(self, mock_execute): + fake_key = uuid.uuid4().hex self.encryptor._get_key = mock.MagicMock() self.encryptor._get_key.return_value = ( - test_cryptsetup.fake__get_key(None)) + test_cryptsetup.fake__get_key(None, fake_key)) mock_execute.side_effect = [ putils.ProcessExecutionError(exit_code=1), # luksOpen @@ -150,7 +156,7 @@ class LuksEncryptorTestCase(test_cryptsetup.CryptsetupEncryptorTestCase): mock_execute.assert_has_calls([ mock.call('cryptsetup', 'luksOpen', '--key-file=-', self.dev_path, - self.dev_name, process_input='0' * 32, + self.dev_name, process_input=fake_key, root_helper=self.root_helper, run_as_root=True, check_exit_code=True), mock.call('cryptsetup', 'isLuks', '--verbose', self.dev_path, @@ -177,3 +183,74 @@ class LuksEncryptorTestCase(test_cryptsetup.CryptsetupEncryptorTestCase): root_helper=self.root_helper, attempts=3, run_as_root=True, check_exit_code=True), ]) + + def test_get_mangled_passphrase(self): + # Confirm that a mangled passphrase is provided as per bug#1633518 + unmangled_raw_key = bytes(binascii.unhexlify('0725230b')) + symmetric_key = key.SymmetricKey('AES', len(unmangled_raw_key) * 8, + unmangled_raw_key) + unmangled_encoded_key = symmetric_key.get_encoded() + self.assertEqual(self.encryptor._get_mangled_passphrase( + unmangled_encoded_key), '72523b') + + @mock.patch('os_brick.executor.Executor._execute') + def test_attach_volume_unmangle_passphrase(self, mock_execute): + fake_key = '0725230b' + fake_key_mangled = '72523b' + self.encryptor._get_key = mock.MagicMock() + self.encryptor._get_key.return_value = \ + test_cryptsetup.fake__get_key(None, fake_key) + + mock_execute.side_effect = [ + putils.ProcessExecutionError(exit_code=2), # luksOpen + mock.DEFAULT, # luksOpen + mock.DEFAULT, # luksClose + mock.DEFAULT, # luksAddKey + mock.DEFAULT, # luksOpen + mock.DEFAULT, # luksClose + mock.DEFAULT, # luksRemoveKey + mock.DEFAULT, # luksOpen + mock.DEFAULT, # ln + ] + + self.encryptor.attach_volume(None) + + mock_execute.assert_has_calls([ + mock.call('cryptsetup', 'luksOpen', '--key-file=-', self.dev_path, + self.dev_name, process_input=fake_key, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + mock.call('cryptsetup', 'luksOpen', '--key-file=-', self.dev_path, + self.dev_name, process_input=fake_key_mangled, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + mock.call('cryptsetup', 'luksClose', self.dev_name, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True, attempts=3), + mock.call('cryptsetup', 'luksAddKey', self.dev_path, + process_input=''.join([fake_key_mangled, + '\n', fake_key, + '\n', fake_key]), + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + mock.call('cryptsetup', 'luksOpen', '--key-file=-', self.dev_path, + self.dev_name, process_input=fake_key, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + mock.call('cryptsetup', 'luksClose', self.dev_name, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True, attempts=3), + mock.call('cryptsetup', 'luksRemoveKey', self.dev_path, + process_input=fake_key_mangled, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + mock.call('cryptsetup', 'luksOpen', '--key-file=-', self.dev_path, + self.dev_name, process_input=fake_key, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + mock.call('ln', '--symbolic', '--force', + '/dev/mapper/%s' % self.dev_name, self.symlink_path, + root_helper=self.root_helper, run_as_root=True, + check_exit_code=True), + ], any_order=False) + self.assertEqual(9, mock_execute.call_count)