From 2c6269c2326ce9604ef6ff84151fc10ac44af19c Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 22 Sep 2021 11:14:37 -0500 Subject: [PATCH 01/18] in progress... --- cloudinit/config/cc_growpart.py | 81 ++++++++++++++++++++++++++++++--- cloudinit/subp.py | 21 +++------ 2 files changed, 82 insertions(+), 20 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index b254a133080..7ad5c1864bf 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -293,12 +293,69 @@ def devent2dev(devent): return dev +def is_mapped_device(blockdev) -> bool: + """ + Check if a device is a mapped device. + + blockdev should look something like '/dev/mapper/disk1' if True, + otherwise something like /dev/vdb1. + """ + is_mapped = blockdev.startswith('/dev/mapper/') + LOG.debug("%s is %s a mapped device", blockdev, "" if is_mapped else "not") + return is_mapped + + +def is_encrypted(blockdev) -> bool: + """ + Check if a device is an encrypted device. + + Will work for both mapped and raw devices. + """ + is_encrypted = False + if not subp.which('cryptsetup'): + LOG.debug('cryptsetup not found. Assuming no encrypted partitions') + return False + with suppress(subp.ProcessExecutionError): + subp.subp(['cryptsetup', 'status', blockdev]) + is_encrypted = True + if not is_encrypted: + with suppress(subp.ProcessExecutionError): + subp.subp(['cryptsetup', 'isLuks', blockdev]) + is_encrypted = True + LOG.debug( + "Determined that %s is %s encrypted", + blockdev, + "" if is_encrypted else "not" + ) + return is_encrypted + + +def get_underlying_partition(blockdev): + command = 'dmsetup deps -o devname {}'.format(blockdev) + dep = subp.subp(command.split())[0] + try: + # Returned result should look something like: + # 1 dependencies : (vdb1) + return '/dev/{}'.format(dep.split(': (')[1].split(')')[0]) + except IndexError: + raise Exception( + "Ran `{}`, but received unexpected stdout: `{}`".format( + command, dep)) + + +def resize_encrypted(blockdev): + subp.subp(['cryptsetup', '--key-file', '', 'resize', blockdev]) + + def resize_devices(resizer, devices): # returns a tuple of tuples containing (entry-in-devices, action, message) + devices = copy.copy(devices) info = [] - for devent in devices: + + while devices: + devent = devices.pop(0) # /mnt TODO: DELETE ME try: - blockdev = devent2dev(devent) + blockdev = devent2dev(devent) # /dev/mapper/disk2 TODO: DELETE ME except ValueError as e: info.append( ( @@ -336,13 +393,25 @@ def resize_devices(resizer, devices): try: (disk, ptnum) = device_part_info(blockdev) except (TypeError, ValueError) as e: - info.append( - ( + if is_mapped_device(blockdev) and is_encrypted(blockdev): + # We need to resize the underlying partition first + partition = get_underlying_partition(blockdev) + if partition not in [x[0] for x in info]: + # We shouldn't attempt to resize this mapped partition + # until the underlying partition is resized, so re-add + # our device to the beginning of the list we're iterating + # over, then add our underlying partition so it can + # get processed first + devices.insert(0, devent) + devices.insert(0, partition) + continue + resize_encrypted(blockdev) + else: + info.append(( devent, RESIZE.SKIPPED, "device_part_info(%s) failed: %s" % (blockdev, e), - ) - ) + )) continue try: diff --git a/cloudinit/subp.py b/cloudinit/subp.py index 7693601dd8b..b3c6c9884c6 100644 --- a/cloudinit/subp.py +++ b/cloudinit/subp.py @@ -4,6 +4,8 @@ import logging import os import subprocess +from typing import Tuple, Any + from errno import ENOEXEC LOG = logging.getLogger(__name__) @@ -154,20 +156,11 @@ def _indent_text(self, text, indent_level=8): def subp( - args, - data=None, - rcs=None, - env=None, - capture=True, - combine_capture=False, - shell=False, - logstring=False, - decode="replace", - target=None, - update_env=None, - status_cb=None, - cwd=None, -): + args, data=None, rcs=None, env=None, capture=True, + combine_capture=False, shell=False, + logstring=False, decode="replace", target=None, update_env=None, + status_cb=None, cwd=None +) -> Tuple[Any, Any]: """Run a subprocess. :param args: command to run in a list. [cmd, arg1, arg2...] From 67304485b099f5da6440db3674d6cb3ba1ce3e0d Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 22 Sep 2021 11:34:44 -0500 Subject: [PATCH 02/18] Remove keyfile argument from cryptsetup --- cloudinit/config/cc_growpart.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index 7ad5c1864bf..df939407074 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -344,7 +344,7 @@ def get_underlying_partition(blockdev): def resize_encrypted(blockdev): - subp.subp(['cryptsetup', '--key-file', '', 'resize', blockdev]) + subp.subp(['cryptsetup', 'resize', blockdev]) def resize_devices(resizer, devices): From 2250d76b8b9dbe8f2dfc57a137c3fd0443925e53 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Thu, 4 Nov 2021 11:59:04 -0500 Subject: [PATCH 03/18] Update based on review comments --- cloudinit/config/cc_growpart.py | 92 +++++++++++++++++++++------------ 1 file changed, 58 insertions(+), 34 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index df939407074..2d77c294f27 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -85,6 +85,8 @@ "ignore_growroot_disabled": False, } +KEYDATA_PATH = "/cc_growpart_keydata" + class RESIZE(object): SKIPPED = "SKIPPED" @@ -293,35 +295,31 @@ def devent2dev(devent): return dev -def is_mapped_device(blockdev) -> bool: - """ - Check if a device is a mapped device. +def get_mapped_device(blockdev): + """Returns underlying block device for a mapped device. - blockdev should look something like '/dev/mapper/disk1' if True, - otherwise something like /dev/vdb1. + If blockdev is a symlink pointing to a /dev/dm-* device, return + the device pointed to. Otherwise, return None. """ - is_mapped = blockdev.startswith('/dev/mapper/') - LOG.debug("%s is %s a mapped device", blockdev, "" if is_mapped else "not") - return is_mapped + realpath = os.path.realpath(blockdev) + if realpath.startswith("/dev/dm-"): + LOG.debug("%s is a mapped device pointing to %s", blockdev, realpath) + return realpath + return None def is_encrypted(blockdev) -> bool: """ - Check if a device is an encrypted device. - - Will work for both mapped and raw devices. + Check if a device is an encrypted device. blockdev should have + a /dev/dm-* path. """ - is_encrypted = False - if not subp.which('cryptsetup'): - LOG.debug('cryptsetup not found. Assuming no encrypted partitions') + if not subp.which("cryptsetup"): + LOG.debug("cryptsetup not found. Assuming no encrypted partitions") return False + is_encrypted = False with suppress(subp.ProcessExecutionError): - subp.subp(['cryptsetup', 'status', blockdev]) + subp.subp(["cryptsetup", "status", blockdev]) is_encrypted = True - if not is_encrypted: - with suppress(subp.ProcessExecutionError): - subp.subp(['cryptsetup', 'isLuks', blockdev]) - is_encrypted = True LOG.debug( "Determined that %s is %s encrypted", blockdev, @@ -331,20 +329,40 @@ def is_encrypted(blockdev) -> bool: def get_underlying_partition(blockdev): - command = 'dmsetup deps -o devname {}'.format(blockdev) - dep = subp.subp(command.split())[0] + command = ["dmsetup", "deps", "--options=devname", blockdev] + dep = subp.subp(command)[0] try: # Returned result should look something like: # 1 dependencies : (vdb1) - return '/dev/{}'.format(dep.split(': (')[1].split(')')[0]) + return "/dev/{}".format(dep.split(": (")[1].split(")")[0]) except IndexError: raise Exception( "Ran `{}`, but received unexpected stdout: `{}`".format( command, dep)) -def resize_encrypted(blockdev): - subp.subp(['cryptsetup', 'resize', blockdev]) +def resize_encrypted(blockdev, partition): + """Use 'cryptsetup resize' to resize LUKS volume. + + The loaded keyfile is json formatted with 'key' and 'slot' keys. + key is base64 encoded. Example: + {"key":"XFmCwX2FHIQp0LBWaLEMiHIyfxt1SGm16VvUAVledlY=","slot":5} + """ + try: + with open(KEYDATA_PATH) as f: + keydata = json.load(f) + key = keydata["key"] + decoded_key = base64.b64decode(key) + slot = keydata["slot"] + except Exception as e: + raise Exception("Could not load encryption key") from e + subp.subp( + ["cryptsetup", "--key-file", "-", "resize", blockdev], + data=decoded_key, + ) + subp.subp([ + "cryptsetup", "luksKillSlot", "--batch-mode", partition, str(slot) + ]) def resize_devices(resizer, devices): @@ -353,9 +371,9 @@ def resize_devices(resizer, devices): info = [] while devices: - devent = devices.pop(0) # /mnt TODO: DELETE ME + devent = devices.pop(0) try: - blockdev = devent2dev(devent) # /dev/mapper/disk2 TODO: DELETE ME + blockdev = devent2dev(devent) except ValueError as e: info.append( ( @@ -390,10 +408,9 @@ def resize_devices(resizer, devices): ) continue - try: - (disk, ptnum) = device_part_info(blockdev) - except (TypeError, ValueError) as e: - if is_mapped_device(blockdev) and is_encrypted(blockdev): + underlying_blockdev = get_mapped_device(blockdev) + if underlying_blockdev and is_encrypted(underlying_blockdev): + try: # We need to resize the underlying partition first partition = get_underlying_partition(blockdev) if partition not in [x[0] for x in info]: @@ -405,13 +422,20 @@ def resize_devices(resizer, devices): devices.insert(0, devent) devices.insert(0, partition) continue - resize_encrypted(blockdev) - else: + resize_encrypted(blockdev, partition) + except Exception as e: info.append(( devent, - RESIZE.SKIPPED, - "device_part_info(%s) failed: %s" % (blockdev, e), + RESIZE.FAILED, + "Resizing encrypted device ({}) failed: {}".format( + blockdev, e + ) )) + try: + (disk, ptnum) = device_part_info(blockdev) + except (TypeError, ValueError) as e: + info.append((devent, RESIZE.SKIPPED, + "device_part_info(%s) failed: %s" % (blockdev, e),)) continue try: From 93ce9507fce17d7b8ff0b3e867e841e370e1ea9c Mon Sep 17 00:00:00 2001 From: James Falcon Date: Thu, 3 Feb 2022 16:40:36 -0600 Subject: [PATCH 04/18] Fix some rebase formatting --- cloudinit/config/cc_growpart.py | 35 +++++++++++++++++++++------------ cloudinit/subp.py | 20 +++++++++++++------ 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index 2d77c294f27..f9bb47f36b6 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -323,7 +323,7 @@ def is_encrypted(blockdev) -> bool: LOG.debug( "Determined that %s is %s encrypted", blockdev, - "" if is_encrypted else "not" + "" if is_encrypted else "not", ) return is_encrypted @@ -338,7 +338,9 @@ def get_underlying_partition(blockdev): except IndexError: raise Exception( "Ran `{}`, but received unexpected stdout: `{}`".format( - command, dep)) + command, dep + ) + ) def resize_encrypted(blockdev, partition): @@ -360,9 +362,9 @@ def resize_encrypted(blockdev, partition): ["cryptsetup", "--key-file", "-", "resize", blockdev], data=decoded_key, ) - subp.subp([ - "cryptsetup", "luksKillSlot", "--batch-mode", partition, str(slot) - ]) + subp.subp( + ["cryptsetup", "luksKillSlot", "--batch-mode", partition, str(slot)] + ) def resize_devices(resizer, devices): @@ -424,18 +426,25 @@ def resize_devices(resizer, devices): continue resize_encrypted(blockdev, partition) except Exception as e: - info.append(( - devent, - RESIZE.FAILED, - "Resizing encrypted device ({}) failed: {}".format( - blockdev, e + info.append( + ( + devent, + RESIZE.FAILED, + "Resizing encrypted device ({}) failed: {}".format( + blockdev, e + ), ) - )) + ) try: (disk, ptnum) = device_part_info(blockdev) except (TypeError, ValueError) as e: - info.append((devent, RESIZE.SKIPPED, - "device_part_info(%s) failed: %s" % (blockdev, e),)) + info.append( + ( + devent, + RESIZE.SKIPPED, + "device_part_info(%s) failed: %s" % (blockdev, e), + ) + ) continue try: diff --git a/cloudinit/subp.py b/cloudinit/subp.py index b3c6c9884c6..08458d2b714 100644 --- a/cloudinit/subp.py +++ b/cloudinit/subp.py @@ -4,9 +4,8 @@ import logging import os import subprocess -from typing import Tuple, Any - from errno import ENOEXEC +from typing import Any, Tuple LOG = logging.getLogger(__name__) @@ -156,10 +155,19 @@ def _indent_text(self, text, indent_level=8): def subp( - args, data=None, rcs=None, env=None, capture=True, - combine_capture=False, shell=False, - logstring=False, decode="replace", target=None, update_env=None, - status_cb=None, cwd=None + args, + data=None, + rcs=None, + env=None, + capture=True, + combine_capture=False, + shell=False, + logstring=False, + decode="replace", + target=None, + update_env=None, + status_cb=None, + cwd=None, ) -> Tuple[Any, Any]: """Run a subprocess. From 0fba7ebdb6a63f3e6efa667cd3a8d68b0a4ad9d3 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Thu, 3 Mar 2022 12:28:03 +0100 Subject: [PATCH 05/18] remove wrong typing stuff I added before --- cloudinit/subp.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cloudinit/subp.py b/cloudinit/subp.py index 08458d2b714..7693601dd8b 100644 --- a/cloudinit/subp.py +++ b/cloudinit/subp.py @@ -5,7 +5,6 @@ import os import subprocess from errno import ENOEXEC -from typing import Any, Tuple LOG = logging.getLogger(__name__) @@ -168,7 +167,7 @@ def subp( update_env=None, status_cb=None, cwd=None, -) -> Tuple[Any, Any]: +): """Run a subprocess. :param args: command to run in a list. [cmd, arg1, arg2...] From 8318bad2098a7386478ef55c5f635a5f9deef452 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 22 Mar 2022 16:43:25 -0500 Subject: [PATCH 06/18] Add additional error handling --- cloudinit/config/cc_growpart.py | 43 +++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 13 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index f9bb47f36b6..3794167c8d9 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -298,6 +298,8 @@ def devent2dev(devent): def get_mapped_device(blockdev): """Returns underlying block device for a mapped device. + blockdev will usually take the form of /dev/mapper/some_name + If blockdev is a symlink pointing to a /dev/dm-* device, return the device pointed to. Otherwise, return None. """ @@ -330,17 +332,19 @@ def is_encrypted(blockdev) -> bool: def get_underlying_partition(blockdev): command = ["dmsetup", "deps", "--options=devname", blockdev] - dep = subp.subp(command)[0] + dep: str = subp.subp(command)[0] # type: ignore + # Returned result should look something like: + # 1 dependencies : (vdb1) + if not dep.startswith("1 depend"): + raise RuntimeError( + f"Expecting 1 dependencies from 'dmsetup'. Received: {dep}" + ) try: - # Returned result should look something like: - # 1 dependencies : (vdb1) - return "/dev/{}".format(dep.split(": (")[1].split(")")[0]) - except IndexError: + return f'/dev/{dep.split(": (")[1].split(")")[0]}' + except IndexError as e: raise Exception( - "Ran `{}`, but received unexpected stdout: `{}`".format( - command, dep - ) - ) + f"Ran `{command}`, but received unexpected stdout: `{dep}`" + ) from e def resize_encrypted(blockdev, partition): @@ -358,10 +362,20 @@ def resize_encrypted(blockdev, partition): slot = keydata["slot"] except Exception as e: raise Exception("Could not load encryption key") from e - subp.subp( - ["cryptsetup", "--key-file", "-", "resize", blockdev], - data=decoded_key, - ) + try: + subp.subp( + ["cryptsetup", "--key-file", "-", "resize", blockdev], + data=decoded_key, + ) + except subp.ProcessExecutionError as e: + if e.exit_code == 2: + raise RuntimeError( + "Resize of encrypted volume failed because no key is " + "available for the provided passphrase. This is expected if " + "the volume has been previously resized." + ) from e + raise + subp.subp( ["cryptsetup", "luksKillSlot", "--batch-mode", partition, str(slot)] ) @@ -435,6 +449,9 @@ def resize_devices(resizer, devices): ), ) ) + # At this point, we WON'T resize a non-encrypted mapped device + # though we should probably grow the ability to + continue try: (disk, ptnum) = device_part_info(blockdev) except (TypeError, ValueError) as e: From dc9ae45a052211d9cffbfb17c3f91ab041291f5c Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 23 Mar 2022 14:37:39 -0500 Subject: [PATCH 07/18] tmp --- cloudinit/config/cc_growpart.py | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index 3794167c8d9..b13e5b3e755 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -85,7 +85,7 @@ "ignore_growroot_disabled": False, } -KEYDATA_PATH = "/cc_growpart_keydata" +KEYDATA_PATH = Path("/cc_growpart_keydata") class RESIZE(object): @@ -355,13 +355,16 @@ def resize_encrypted(blockdev, partition): {"key":"XFmCwX2FHIQp0LBWaLEMiHIyfxt1SGm16VvUAVledlY=","slot":5} """ try: - with open(KEYDATA_PATH) as f: + with KEYDATA_PATH.open() as f: keydata = json.load(f) key = keydata["key"] decoded_key = base64.b64decode(key) slot = keydata["slot"] except Exception as e: - raise Exception("Could not load encryption key") from e + raise RuntimeError( + "Could not load encryption key. This is expected if " + "the volume has been previously resized." + ) from e try: subp.subp( ["cryptsetup", "--key-file", "-", "resize", blockdev], @@ -375,10 +378,22 @@ def resize_encrypted(blockdev, partition): "the volume has been previously resized." ) from e raise - - subp.subp( - ["cryptsetup", "luksKillSlot", "--batch-mode", partition, str(slot)] - ) + else: + try: + subp.subp( + [ + "cryptsetup", + "luksKillSlot", + "--batch-mode", + partition, + str(slot), + ] + ) + KEYDATA_PATH.unlink() + except Exception: + util.logexc( + LOG, "Failed to cleanup after resizing encrypted volume" + ) def resize_devices(resizer, devices): From b0a734f4ea699c7b196b981f57eccdfe6c6130af Mon Sep 17 00:00:00 2001 From: James Falcon Date: Mon, 28 Mar 2022 16:16:06 -0500 Subject: [PATCH 08/18] Add tests and some random fixups --- cloudinit/config/cc_growpart.py | 64 +++--- tests/unittests/config/test_cc_growpart.py | 217 +++++++++++++++++++++ 2 files changed, 251 insertions(+), 30 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index b13e5b3e755..a1a9f5aec66 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -298,7 +298,8 @@ def devent2dev(devent): def get_mapped_device(blockdev): """Returns underlying block device for a mapped device. - blockdev will usually take the form of /dev/mapper/some_name + If it is mapped, blockdev will usually take the form of + /dev/mapper/some_name If blockdev is a symlink pointing to a /dev/dm-* device, return the device pointed to. Otherwise, return None. @@ -323,7 +324,7 @@ def is_encrypted(blockdev) -> bool: subp.subp(["cryptsetup", "status", blockdev]) is_encrypted = True LOG.debug( - "Determined that %s is %s encrypted", + "Determined that %s is %sencrypted", blockdev, "" if is_encrypted else "not", ) @@ -337,12 +338,12 @@ def get_underlying_partition(blockdev): # 1 dependencies : (vdb1) if not dep.startswith("1 depend"): raise RuntimeError( - f"Expecting 1 dependencies from 'dmsetup'. Received: {dep}" + f"Expecting '1 dependencies' from 'dmsetup'. Received: {dep}" ) try: return f'/dev/{dep.split(": (")[1].split(")")[0]}' except IndexError as e: - raise Exception( + raise RuntimeError( f"Ran `{command}`, but received unexpected stdout: `{dep}`" ) from e @@ -365,35 +366,31 @@ def resize_encrypted(blockdev, partition): "Could not load encryption key. This is expected if " "the volume has been previously resized." ) from e + subp.subp( + ["cryptsetup", "--key-file", "-", "resize", blockdev], + data=decoded_key, + ) + try: subp.subp( - ["cryptsetup", "--key-file", "-", "resize", blockdev], - data=decoded_key, + [ + "cryptsetup", + "luksKillSlot", + "--batch-mode", + partition, + str(slot), + ] + ) + except Exception: + util.logexc( + LOG, "Failed to kill luks slot after resizing encrypted volume" + ) + try: + KEYDATA_PATH.unlink() + except Exception: + util.logexc( + LOG, "Failed to remove keyfile after resizing encrypted volume" ) - except subp.ProcessExecutionError as e: - if e.exit_code == 2: - raise RuntimeError( - "Resize of encrypted volume failed because no key is " - "available for the provided passphrase. This is expected if " - "the volume has been previously resized." - ) from e - raise - else: - try: - subp.subp( - [ - "cryptsetup", - "luksKillSlot", - "--batch-mode", - partition, - str(slot), - ] - ) - KEYDATA_PATH.unlink() - except Exception: - util.logexc( - LOG, "Failed to cleanup after resizing encrypted volume" - ) def resize_devices(resizer, devices): @@ -454,6 +451,13 @@ def resize_devices(resizer, devices): devices.insert(0, partition) continue resize_encrypted(blockdev, partition) + info.append( + ( + devent, + RESIZE.CHANGED, + f"Successfully resized encrypted volume '{blockdev}'", + ) + ) except Exception as e: info.append( ( diff --git a/tests/unittests/config/test_cc_growpart.py b/tests/unittests/config/test_cc_growpart.py index e514895bcad..00ce511b843 100644 --- a/tests/unittests/config/test_cc_growpart.py +++ b/tests/unittests/config/test_cc_growpart.py @@ -8,6 +8,8 @@ import stat import unittest from contextlib import ExitStack +from copy import deepcopy +from itertools import chain from unittest import mock import pytest @@ -349,6 +351,221 @@ def find(name, res): os.stat = real_stat +class TestEncrypted: + """Attempt end-to-end scenarios using encrypted devices. + + Things are mocked such that: + - "/fake_encrypted" is mounted onto "/dev/mapper/fake" + - "/dev/mapper/fake" is a LUKS device and symlinked to /dev/dm-1 + - The partition backing "/dev/mapper/fake" is "/dev/vdx1" + - "/" is not encrypted and mounted onto "/dev/vdz1" + + Note that we don't (yet) support non-encrypted mapped drives, such + as LVM volumes. If our mount point is /dev/mapper/*, then we will + not resize it if it is not encrypted. + """ + + def _subp_side_effect(self, value, good=True, **kwargs): + if value[0] == "dmsetup": + return ("1 dependencies : (vdx1)",) + return mock.Mock() + + def _device_part_info_side_effect(self, value): + if value.startswith("/dev/mapper/"): + raise TypeError(f"{value} not a partition") + return (1024, 1024) + + def _devent2dev_side_effect(self, value): + if value == "/fake_encrypted": + return "/dev/mapper/fake" + elif value == "/": + return "/dev/vdz" + elif value.startswith("/dev"): + return value + raise Exception(f"unexpected value {value}") + + def _realpath_side_effect(self, value): + return "/dev/dm-1" if value.startswith("/dev/mapper") else value + + def assert_resize_and_cleanup(self): + all_subp_args = list( + chain(*[args[0][0] for args in self.m_subp.call_args_list]) + ) + assert "resize" in all_subp_args + assert "luksKillSlot" in all_subp_args + self.m_unlink.assert_called_once() + + def assert_no_resize_or_cleanup(self): + all_subp_args = list( + chain(*[args[0][0] for args in self.m_subp.call_args_list]) + ) + assert "resize" not in all_subp_args + assert "luksKillSlot" not in all_subp_args + self.m_unlink.assert_not_called() + + @pytest.fixture + def common_mocks(self, mocker): + # These are all "happy path" mocks which will get overridden + # when needed + mocker.patch( + "cloudinit.config.cc_growpart.device_part_info", + side_effect=self._device_part_info_side_effect, + ) + mocker.patch("os.stat") + mocker.patch("stat.S_ISBLK") + mocker.patch("stat.S_ISCHR") + mocker.patch( + "cloudinit.config.cc_growpart.devent2dev", + side_effect=self._devent2dev_side_effect, + ) + mocker.patch( + "os.path.realpath", side_effect=self._realpath_side_effect + ) + # Only place subp.which is used in cc_growpart is for cryptsetup + mocker.patch( + "cloudinit.config.cc_growpart.subp.which", + return_value="/usr/sbin/cryptsetup", + ) + self.m_subp = mocker.patch( + "cloudinit.config.cc_growpart.subp.subp", + side_effect=self._subp_side_effect, + ) + mocker.patch( + "pathlib.Path.open", + new_callable=mock.mock_open, + read_data=( + '{"key":"XFmCwX2FHIQp0LBWaLEMiHIyfxt1SGm16VvUAVledlY=",' + '"slot":5}' + ), + ) + self.m_unlink = mocker.patch("pathlib.Path.unlink", autospec=True) + + self.resizer = mock.Mock() + self.resizer.resize = mock.Mock(return_value=(1024, 1024)) + + def test_resize_when_encrypted(self, common_mocks, caplog): + info = cc_growpart.resize_devices(self.resizer, ["/fake_encrypted"]) + assert len(info) == 2 + assert info[0][0] == "/dev/vdx1" + assert info[0][2].startswith("no change necessary") + assert info[1][0] == "/fake_encrypted" + assert ( + info[1][2] + == "Successfully resized encrypted volume '/dev/mapper/fake'" + ) + assert ( + "/dev/mapper/fake is a mapped device pointing to /dev/dm-1" + in caplog.text + ) + assert "Determined that /dev/dm-1 is encrypted" in caplog.text + + self.assert_resize_and_cleanup() + + def test_resize_when_unencrypted(self, common_mocks): + info = cc_growpart.resize_devices(self.resizer, ["/"]) + assert len(info) == 1 + assert info[0][0] == "/" + assert "encrypted" not in info[0][2] + self.assert_no_resize_or_cleanup() + + def test_encrypted_but_cryptsetup_not_found( + self, common_mocks, mocker, caplog + ): + mocker.patch( + "cloudinit.config.cc_growpart.subp.which", + return_value=None, + ) + info = cc_growpart.resize_devices(self.resizer, ["/fake_encrypted"]) + assert len(info) == 1 + assert "encrypted" not in info[0][2] + assert "cryptsetup not found" in caplog.text + self.assert_no_resize_or_cleanup() + + def test_dmsetup_not_found(self, common_mocks, mocker, caplog): + def _subp_side_effect(value, **kwargs): + if value[0] == "dmsetup": + raise subp.ProcessExecutionError() + + mocker.patch( + "cloudinit.config.cc_growpart.subp.subp", + side_effect=_subp_side_effect, + ) + info = cc_growpart.resize_devices(self.resizer, ["/fake_encrypted"]) + assert len(info) == 1 + assert info[0][0] == "/fake_encrypted" + assert info[0][1] == "FAILED" + assert ( + "Resizing encrypted device (/dev/mapper/fake) failed" in info[0][2] + ) + self.assert_no_resize_or_cleanup() + + def test_unparsable_dmsetup(self, common_mocks, mocker, caplog): + def _subp_side_effect(value, **kwargs): + if value[0] == "dmsetup": + return ("2 dependencies",) + return mock.Mock() + + mocker.patch( + "cloudinit.config.cc_growpart.subp.subp", + side_effect=_subp_side_effect, + ) + info = cc_growpart.resize_devices(self.resizer, ["/fake_encrypted"]) + assert len(info) == 1 + assert info[0][0] == "/fake_encrypted" + assert info[0][1] == "FAILED" + assert ( + "Resizing encrypted device (/dev/mapper/fake) failed" in info[0][2] + ) + self.assert_no_resize_or_cleanup() + + def test_missing_keydata(self, common_mocks, mocker, caplog): + # Note that this will be standard behavior after first boot + # on a system with an encrypted root partition + mocker.patch("pathlib.Path.open", side_effect=FileNotFoundError()) + info = cc_growpart.resize_devices(self.resizer, ["/fake_encrypted"]) + assert len(info) == 2 + assert info[0][0] == "/dev/vdx1" + assert info[0][2].startswith("no change necessary") + assert info[1][0] == "/fake_encrypted" + assert info[1][1] == "FAILED" + assert ( + info[1][2] + == "Resizing encrypted device (/dev/mapper/fake) failed: Could " + "not load encryption key. This is expected if the volume has " + "been previously resized." + ) + self.assert_no_resize_or_cleanup() + + def test_resize_failed(self, common_mocks, mocker, caplog): + def _subp_side_effect(value, **kwargs): + if value[0] == "dmsetup": + return ("1 dependencies : (vdx1)",) + elif value[0] == "cryptsetup" and "resize" in value: + raise subp.ProcessExecutionError() + return mock.Mock() + + mocker.patch( + "cloudinit.config.cc_growpart.subp.subp", + side_effect=_subp_side_effect, + ) + + info = cc_growpart.resize_devices(self.resizer, ["/fake_encrypted"]) + assert len(info) == 2 + assert info[0][0] == "/dev/vdx1" + assert info[0][2].startswith("no change necessary") + assert info[1][0] == "/fake_encrypted" + assert info[1][1] == "FAILED" + assert ( + "Resizing encrypted device (/dev/mapper/fake) failed" in info[1][2] + ) + # Assert no cleanup + all_subp_args = list( + chain(*[args[0][0] for args in self.m_subp.call_args_list]) + ) + assert "luksKillSlot" not in all_subp_args + self.m_unlink.assert_not_called() + + def simple_device_part_info(devpath): # simple stupid return (/dev/vda, 1) for /dev/vda ret = re.search("([^0-9]*)([0-9]*)$", devpath) From 28c5a4011e6fae4360d06608c794511976e7b640 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Mon, 28 Mar 2022 16:44:52 -0500 Subject: [PATCH 09/18] Add the mocker dependency --- test-requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/test-requirements.txt b/test-requirements.txt index 06dfbbec156..7160416a850 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -2,6 +2,7 @@ httpretty>=0.7.1 pytest pytest-cov +pytest-mock # Only really needed on older versions of python setuptools From 049eead882cc8fadfad90269c53ed8d048101aa6 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Mon, 28 Mar 2022 16:52:21 -0500 Subject: [PATCH 10/18] flake8 --- tests/unittests/config/test_cc_growpart.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unittests/config/test_cc_growpart.py b/tests/unittests/config/test_cc_growpart.py index 00ce511b843..5e7cb0afb95 100644 --- a/tests/unittests/config/test_cc_growpart.py +++ b/tests/unittests/config/test_cc_growpart.py @@ -8,7 +8,6 @@ import stat import unittest from contextlib import ExitStack -from copy import deepcopy from itertools import chain from unittest import mock From 7e9fb15ab30721a221d58119b589d9b6e0e52677 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 6 Apr 2022 08:51:41 -0500 Subject: [PATCH 11/18] Add "lowest-supported" dep for pytest-mock --- tox.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/tox.ini b/tox.ini index b9fe5622142..c699debfaa2 100644 --- a/tox.ini +++ b/tox.ini @@ -111,6 +111,7 @@ deps = # test-requirements pytest==3.3.2 pytest-cov==2.5.1 + pytest-mock==1.7.1 # Needed by pytest and default causes failures attrs==17.4.0 From beae1562c0089ca82f2499e0bd2bdb9133ae1519 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Wed, 6 Apr 2022 09:02:42 -0500 Subject: [PATCH 12/18] fix rebase --- cloudinit/config/cc_growpart.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index a1a9f5aec66..7f227ed3648 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -7,10 +7,15 @@ # This file is part of cloud-init. See LICENSE file for license information. """Growpart: Grow partitions""" +import base64 +import copy +import json import os import os.path import re import stat +from contextlib import suppress +from pathlib import Path from textwrap import dedent from cloudinit import log as logging From 7d7de38dec2754f8e95ded0e3ffb6dacb7f47248 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Mon, 25 Apr 2022 19:20:32 -0500 Subject: [PATCH 13/18] LOG spacing --- cloudinit/config/cc_growpart.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index 7f227ed3648..4a28bd2f6e5 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -331,7 +331,7 @@ def is_encrypted(blockdev) -> bool: LOG.debug( "Determined that %s is %sencrypted", blockdev, - "" if is_encrypted else "not", + "" if is_encrypted else "not ", ) return is_encrypted From 6385dc2c4f663916f92c1924f54d57c0ed11e6a0 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Mon, 25 Apr 2022 23:34:00 -0500 Subject: [PATCH 14/18] Bring back isLuks --- cloudinit/config/cc_growpart.py | 78 +++++++++++++--------- tests/unittests/config/test_cc_growpart.py | 3 +- 2 files changed, 49 insertions(+), 32 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index 4a28bd2f6e5..97d14411e73 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -316,24 +316,31 @@ def get_mapped_device(blockdev): return None -def is_encrypted(blockdev) -> bool: +def is_encrypted(blockdev, partition) -> bool: """ Check if a device is an encrypted device. blockdev should have - a /dev/dm-* path. + a /dev/dm-* path whereas partition is something like /dev/sda1. """ if not subp.which("cryptsetup"): LOG.debug("cryptsetup not found. Assuming no encrypted partitions") return False - is_encrypted = False - with suppress(subp.ProcessExecutionError): + try: subp.subp(["cryptsetup", "status", blockdev]) - is_encrypted = True - LOG.debug( - "Determined that %s is %sencrypted", - blockdev, - "" if is_encrypted else "not ", - ) - return is_encrypted + except subp.ProcessExecutionError as e: + if e.exit_code == 4: + LOG.debug("Determined that %s is not encrypted", blockdev) + else: + LOG.warning( + "Received unexpected exit code %s from " + "cryptsetup status. Assuming no encrypted partitions.", + e.exit_code, + ) + return False + with suppress(subp.ProcessExecutionError): + subp.subp(["cryptsetup", "isLuks", partition]) + LOG.debug("Determined that %s is encrypted", blockdev) + return True + return False def get_underlying_partition(blockdev): @@ -442,35 +449,44 @@ def resize_devices(resizer, devices): continue underlying_blockdev = get_mapped_device(blockdev) - if underlying_blockdev and is_encrypted(underlying_blockdev): + if underlying_blockdev: try: # We need to resize the underlying partition first partition = get_underlying_partition(blockdev) - if partition not in [x[0] for x in info]: - # We shouldn't attempt to resize this mapped partition - # until the underlying partition is resized, so re-add - # our device to the beginning of the list we're iterating - # over, then add our underlying partition so it can - # get processed first - devices.insert(0, devent) - devices.insert(0, partition) - continue - resize_encrypted(blockdev, partition) - info.append( - ( - devent, - RESIZE.CHANGED, - f"Successfully resized encrypted volume '{blockdev}'", + if is_encrypted(underlying_blockdev, partition): + if partition not in [x[0] for x in info]: + # We shouldn't attempt to resize this mapped partition + # until the underlying partition is resized, so re-add + # our device to the beginning of the list we're + # iterating over, then add our underlying partition + # so it can get processed first + devices.insert(0, devent) + devices.insert(0, partition) + continue + resize_encrypted(blockdev, partition) + info.append( + ( + devent, + RESIZE.CHANGED, + "Successfully resized encrypted volume " + f"'{blockdev}'", + ) + ) + else: + info.append( + ( + devent, + RESIZE.SKIPPED, + f"Resizing mapped device ({blockdev}) skipped " + "as it is not encrypted.", + ) ) - ) except Exception as e: info.append( ( devent, RESIZE.FAILED, - "Resizing encrypted device ({}) failed: {}".format( - blockdev, e - ), + f"Resizing encrypted device ({blockdev}) failed: {e}", ) ) # At this point, we WON'T resize a non-encrypted mapped device diff --git a/tests/unittests/config/test_cc_growpart.py b/tests/unittests/config/test_cc_growpart.py index 5e7cb0afb95..d6f06252c66 100644 --- a/tests/unittests/config/test_cc_growpart.py +++ b/tests/unittests/config/test_cc_growpart.py @@ -475,8 +475,9 @@ def test_encrypted_but_cryptsetup_not_found( return_value=None, ) info = cc_growpart.resize_devices(self.resizer, ["/fake_encrypted"]) + assert len(info) == 1 - assert "encrypted" not in info[0][2] + assert "skipped as it is not encrypted" in info[0][2] assert "cryptsetup not found" in caplog.text self.assert_no_resize_or_cleanup() From b00eb02e3d7984d7425bc77b9595e03d77d83f6d Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 26 Apr 2022 00:12:53 -0500 Subject: [PATCH 15/18] review comments --- cloudinit/config/cc_growpart.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index 97d14411e73..6cc3ff6dfdd 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -17,6 +17,7 @@ from contextlib import suppress from pathlib import Path from textwrap import dedent +from typing import Tuple from cloudinit import log as logging from cloudinit import subp, temp_utils, util @@ -360,13 +361,15 @@ def get_underlying_partition(blockdev): ) from e -def resize_encrypted(blockdev, partition): +def resize_encrypted(blockdev, partition) -> Tuple[str, str]: """Use 'cryptsetup resize' to resize LUKS volume. The loaded keyfile is json formatted with 'key' and 'slot' keys. key is base64 encoded. Example: {"key":"XFmCwX2FHIQp0LBWaLEMiHIyfxt1SGm16VvUAVledlY=","slot":5} """ + if not KEYDATA_PATH.exists(): + return (RESIZE.SKIPPED, "No encryption keyfile found") try: with KEYDATA_PATH.open() as f: keydata = json.load(f) @@ -393,9 +396,9 @@ def resize_encrypted(blockdev, partition): str(slot), ] ) - except Exception: - util.logexc( - LOG, "Failed to kill luks slot after resizing encrypted volume" + except subp.ProcessExecutionError as e: + LOG.warning( + "Failed to kill luks slot after resizing encrypted volume: %s", e ) try: KEYDATA_PATH.unlink() @@ -404,6 +407,8 @@ def resize_encrypted(blockdev, partition): LOG, "Failed to remove keyfile after resizing encrypted volume" ) + return (RESIZE.CHANGED, f"Resized encrypted volume {blockdev}") + def resize_devices(resizer, devices): # returns a tuple of tuples containing (entry-in-devices, action, message) @@ -463,13 +468,12 @@ def resize_devices(resizer, devices): devices.insert(0, devent) devices.insert(0, partition) continue - resize_encrypted(blockdev, partition) + status, message = resize_encrypted(blockdev, partition) info.append( ( devent, - RESIZE.CHANGED, - "Successfully resized encrypted volume " - f"'{blockdev}'", + status, + message, ) ) else: From 91761407cf44112056ce2529f83f42535dd82e5d Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 26 Apr 2022 08:54:07 -0500 Subject: [PATCH 16/18] update tests --- cloudinit/config/cc_growpart.py | 5 ++++- tests/unittests/config/test_cc_growpart.py | 11 +++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index 6cc3ff6dfdd..026912ed919 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -407,7 +407,10 @@ def resize_encrypted(blockdev, partition) -> Tuple[str, str]: LOG, "Failed to remove keyfile after resizing encrypted volume" ) - return (RESIZE.CHANGED, f"Resized encrypted volume {blockdev}") + return ( + RESIZE.CHANGED, + f"Successfully resized encrypted volume '{blockdev}'", + ) def resize_devices(resizer, devices): diff --git a/tests/unittests/config/test_cc_growpart.py b/tests/unittests/config/test_cc_growpart.py index d6f06252c66..4c48d2c1135 100644 --- a/tests/unittests/config/test_cc_growpart.py +++ b/tests/unittests/config/test_cc_growpart.py @@ -437,6 +437,7 @@ def common_mocks(self, mocker): '"slot":5}' ), ) + mocker.patch("pathlib.Path.exists", return_value=True) self.m_unlink = mocker.patch("pathlib.Path.unlink", autospec=True) self.resizer = mock.Mock() @@ -565,6 +566,16 @@ def _subp_side_effect(value, **kwargs): assert "luksKillSlot" not in all_subp_args self.m_unlink.assert_not_called() + def test_resize_skipped(self, common_mocks, mocker, caplog): + mocker.patch("pathlib.Path.exists", return_value=False) + info = cc_growpart.resize_devices(self.resizer, ["/fake_encrypted"]) + assert len(info) == 2 + assert info[1] == ( + "/fake_encrypted", + "SKIPPED", + "No encryption keyfile found", + ) + def simple_device_part_info(devpath): # simple stupid return (/dev/vda, 1) for /dev/vda From ac754f1187e9871bde5cb03d85df642a8a45c01a Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 26 Apr 2022 16:31:02 -0500 Subject: [PATCH 17/18] Remove luks slot and key even if cryptsetup fails --- cloudinit/config/cc_growpart.py | 45 ++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/cloudinit/config/cc_growpart.py b/cloudinit/config/cc_growpart.py index 026912ed919..14a2c0b88b6 100644 --- a/cloudinit/config/cc_growpart.py +++ b/cloudinit/config/cc_growpart.py @@ -381,31 +381,34 @@ def resize_encrypted(blockdev, partition) -> Tuple[str, str]: "Could not load encryption key. This is expected if " "the volume has been previously resized." ) from e - subp.subp( - ["cryptsetup", "--key-file", "-", "resize", blockdev], - data=decoded_key, - ) try: subp.subp( - [ - "cryptsetup", - "luksKillSlot", - "--batch-mode", - partition, - str(slot), - ] - ) - except subp.ProcessExecutionError as e: - LOG.warning( - "Failed to kill luks slot after resizing encrypted volume: %s", e - ) - try: - KEYDATA_PATH.unlink() - except Exception: - util.logexc( - LOG, "Failed to remove keyfile after resizing encrypted volume" + ["cryptsetup", "--key-file", "-", "resize", blockdev], + data=decoded_key, ) + finally: + try: + subp.subp( + [ + "cryptsetup", + "luksKillSlot", + "--batch-mode", + partition, + str(slot), + ] + ) + except subp.ProcessExecutionError as e: + LOG.warning( + "Failed to kill luks slot after resizing encrypted volume: %s", + e, + ) + try: + KEYDATA_PATH.unlink() + except Exception: + util.logexc( + LOG, "Failed to remove keyfile after resizing encrypted volume" + ) return ( RESIZE.CHANGED, From 3ce1e06ac6c8b1c0bb09f437695d9bf1024ed412 Mon Sep 17 00:00:00 2001 From: James Falcon Date: Tue, 26 Apr 2022 18:31:35 -0500 Subject: [PATCH 18/18] fix test my last change broke --- tests/unittests/config/test_cc_growpart.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unittests/config/test_cc_growpart.py b/tests/unittests/config/test_cc_growpart.py index 4c48d2c1135..2852fbfdeb4 100644 --- a/tests/unittests/config/test_cc_growpart.py +++ b/tests/unittests/config/test_cc_growpart.py @@ -545,7 +545,7 @@ def _subp_side_effect(value, **kwargs): raise subp.ProcessExecutionError() return mock.Mock() - mocker.patch( + self.m_subp = mocker.patch( "cloudinit.config.cc_growpart.subp.subp", side_effect=_subp_side_effect, ) @@ -559,12 +559,12 @@ def _subp_side_effect(value, **kwargs): assert ( "Resizing encrypted device (/dev/mapper/fake) failed" in info[1][2] ) - # Assert no cleanup + # Assert we still cleanup all_subp_args = list( chain(*[args[0][0] for args in self.m_subp.call_args_list]) ) - assert "luksKillSlot" not in all_subp_args - self.m_unlink.assert_not_called() + assert "luksKillSlot" in all_subp_args + self.m_unlink.assert_called_once() def test_resize_skipped(self, common_mocks, mocker, caplog): mocker.patch("pathlib.Path.exists", return_value=False)