From c71d13c1bae44de51923537bcea3eac2e6647a2a Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 7 Nov 2021 21:33:02 +0100 Subject: [PATCH 1/8] Use 'cryptsetup erase' to kill LUKS signature. --- changelogs/fragments/327-luks_device-wipe.yml | 2 ++ plugins/modules/luks_device.py | 6 ++++++ 2 files changed, 8 insertions(+) create mode 100644 changelogs/fragments/327-luks_device-wipe.yml diff --git a/changelogs/fragments/327-luks_device-wipe.yml b/changelogs/fragments/327-luks_device-wipe.yml new file mode 100644 index 000000000..c2884358c --- /dev/null +++ b/changelogs/fragments/327-luks_device-wipe.yml @@ -0,0 +1,2 @@ +bugfixes: + - "luks_device - on ``state=absent`` now also runs ``cryptsetup erase`` to make sure the device is no longer accepted as a LUKS device (https://github.com/ansible-collections/community.crypto/issues/326, https://github.com/ansible-collections/community.crypto/pull/327)." diff --git a/plugins/modules/luks_device.py b/plugins/modules/luks_device.py index 9b687b402..f981ac811 100644 --- a/plugins/modules/luks_device.py +++ b/plugins/modules/luks_device.py @@ -507,12 +507,18 @@ def run_luks_close(self, name): if result[RETURN_CODE] != 0: raise ValueError('Error while closing LUKS container %s' % (name)) + def run_luks_erase(self, device, ignore_errors=False): + result = self._run_command([self._cryptsetup_bin, 'erase', device]) + if result[RETURN_CODE] != 0 and not ignore_errors: + raise ValueError('Error while erasing LUKS container %s' % (device)) + def run_luks_remove(self, device): wipefs_bin = self._module.get_bin_path('wipefs', True) name = self.get_container_name_by_device(device) if name is not None: self.run_luks_close(name) + self.run_luks_erase(device, ignore_errors=True) result = self._run_command([wipefs_bin, '--all', device]) if result[RETURN_CODE] != 0: raise ValueError('Error while wiping luks container %s: %s' From 044a3e7e5150459d561be8a7bf2061d97858dcad Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Sun, 7 Nov 2021 21:40:36 +0100 Subject: [PATCH 2/8] Adjust unit test. --- tests/unit/plugins/modules/test_luks_device.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/unit/plugins/modules/test_luks_device.py b/tests/unit/plugins/modules/test_luks_device.py index d798b4474..3d142a71e 100644 --- a/tests/unit/plugins/modules/test_luks_device.py +++ b/tests/unit/plugins/modules/test_luks_device.py @@ -46,9 +46,16 @@ def test_get_container_device_by_name(monkeypatch): def test_run_luks_remove(monkeypatch): + counter = [0] + def run_command_check(self, command): - # check that wipefs command is actually called - assert command[0] == "wipefs" + # check that 'cryptsetup erase' and 'wipefs' commands are actually called + if counter[0] == 0: + assert command[0] == "cryptsetup" + assert command[1] == "erase" + if counter[0] == 1: + assert command[0] == "wipefs" + counter[0] += 1 return [0, "", ""] module = DummyModule() From e5ad7fbf359da727b1c14f89156c94b5a1bbf4be Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Tue, 9 Nov 2021 22:41:47 +0100 Subject: [PATCH 3/8] Use own wiper for LUKS headers. --- plugins/modules/luks_device.py | 36 +++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/plugins/modules/luks_device.py b/plugins/modules/luks_device.py index f981ac811..a3c76009e 100644 --- a/plugins/modules/luks_device.py +++ b/plugins/modules/luks_device.py @@ -356,6 +356,32 @@ LUKS_DEVICE_REGEX = re.compile(r'\s*device:\s+([^\s]*)\s*') +LUKS_HEADER = b'LUKS\xba\xbe' +LUKS_HEADER_L = 6 +LUKS2_HEADER_OFFSETS = [0x4000, 0x8000, 0x10000, 0x20000, 0x40000, 0x80000, 0x100000, 0x200000, 0x400000] +LUKS2_HEADER2 = b'SKUL\xba\xbe' + + +def wipe_luks_headers(device): + wipe_offsets = [] + with open(device, 'rb') as f: + # f.seek(0) + data = f.read(LUKS_HEADER_L) + if data == LUKS_HEADER: + wipe_offsets.append(0) + for offset in LUKS2_HEADER_OFFSETS: + f.seek(offset) + data = f.read(LUKS_HEADER_L) + if data == LUKS2_HEADER2: + wipe_offsets.append(offset) + + if wipe_offsets: + with open(device, 'wb') as f: + for offset in wipe_offsets: + f.seek(offset) + f.write(b'\x00\x00\x00\x00\x00\x00') + + class Handler(object): def __init__(self, module): @@ -508,7 +534,7 @@ def run_luks_close(self, name): raise ValueError('Error while closing LUKS container %s' % (name)) def run_luks_erase(self, device, ignore_errors=False): - result = self._run_command([self._cryptsetup_bin, 'erase', device]) + result = self._run_command([self._cryptsetup_bin, 'erase', '-q', device]) if result[RETURN_CODE] != 0 and not ignore_errors: raise ValueError('Error while erasing LUKS container %s' % (device)) @@ -524,6 +550,14 @@ def run_luks_remove(self, device): raise ValueError('Error while wiping luks container %s: %s' % (device, result[STDERR])) + # For LUKS2, sometimes both `cryptsetup erase` and `wipefs` do **not** + # erase all LUKS signatures (they seem to miss the second header). That's + # why we do it ourselves here. + try: + wipe_luks_headers(device) + except Exception as exc: + raise ValueError('Error while wiping luks container %s: %s' % (device, exc)) + def run_luks_add_key(self, device, keyfile, passphrase, new_keyfile, new_passphrase, pbkdf): ''' Add new key from a keyfile or passphrase to given 'device'; From 599cde27fef4890e8a7aa9be4b775214787b554b Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Tue, 9 Nov 2021 22:42:33 +0100 Subject: [PATCH 4/8] Add comments. --- plugins/modules/luks_device.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/modules/luks_device.py b/plugins/modules/luks_device.py index a3c76009e..9c11bb1d4 100644 --- a/plugins/modules/luks_device.py +++ b/plugins/modules/luks_device.py @@ -356,8 +356,10 @@ LUKS_DEVICE_REGEX = re.compile(r'\s*device:\s+([^\s]*)\s*') +# See https://gitlab.com/cryptsetup/cryptsetup/-/wikis/LUKS-standard/on-disk-format.pdf LUKS_HEADER = b'LUKS\xba\xbe' LUKS_HEADER_L = 6 +# See https://gitlab.com/cryptsetup/LUKS2-docs/-/blob/master/luks2_doc_wip.pdf LUKS2_HEADER_OFFSETS = [0x4000, 0x8000, 0x10000, 0x20000, 0x40000, 0x80000, 0x100000, 0x200000, 0x400000] LUKS2_HEADER2 = b'SKUL\xba\xbe' From ca5d5ff84fb4f01ace13a7ade41026b976c88b0e Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Tue, 9 Nov 2021 22:49:50 +0100 Subject: [PATCH 5/8] Fix tests. --- tests/unit/plugins/modules/test_luks_device.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit/plugins/modules/test_luks_device.py b/tests/unit/plugins/modules/test_luks_device.py index 3d142a71e..3872ab29a 100644 --- a/tests/unit/plugins/modules/test_luks_device.py +++ b/tests/unit/plugins/modules/test_luks_device.py @@ -65,6 +65,9 @@ def run_command_check(self, command): monkeypatch.setattr(luks_device.Handler, "_run_command", run_command_check) + monkeypatch.setattr(luks_device, + "wipe_luks_headers", + lambda device: True) crypt = luks_device.CryptHandler(module) crypt.run_luks_remove("dummy") From 457070a61a82c875613a2302bb372b07669be585 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Tue, 9 Nov 2021 22:58:22 +0100 Subject: [PATCH 6/8] Update changelog. --- changelogs/fragments/327-luks_device-wipe.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/fragments/327-luks_device-wipe.yml b/changelogs/fragments/327-luks_device-wipe.yml index c2884358c..a51db9847 100644 --- a/changelogs/fragments/327-luks_device-wipe.yml +++ b/changelogs/fragments/327-luks_device-wipe.yml @@ -1,2 +1,2 @@ bugfixes: - - "luks_device - on ``state=absent`` now also runs ``cryptsetup erase`` to make sure the device is no longer accepted as a LUKS device (https://github.com/ansible-collections/community.crypto/issues/326, https://github.com/ansible-collections/community.crypto/pull/327)." + - "luks_device - on ``state=absent`` now also runs ``cryptsetup erase`` as well as a built-in LUKS signature cleaner to make sure the device is no longer accepted as a LUKS device (https://github.com/ansible-collections/community.crypto/issues/326, https://github.com/ansible-collections/community.crypto/pull/327)." From b05c89801716da4dfb7b5ac408921451509c5edd Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Wed, 10 Nov 2021 07:14:59 +0100 Subject: [PATCH 7/8] Remove 'cryptsetup erase'. --- changelogs/fragments/327-luks_device-wipe.yml | 2 +- plugins/modules/luks_device.py | 6 ------ tests/unit/plugins/modules/test_luks_device.py | 11 ++--------- 3 files changed, 3 insertions(+), 16 deletions(-) diff --git a/changelogs/fragments/327-luks_device-wipe.yml b/changelogs/fragments/327-luks_device-wipe.yml index a51db9847..5929e4116 100644 --- a/changelogs/fragments/327-luks_device-wipe.yml +++ b/changelogs/fragments/327-luks_device-wipe.yml @@ -1,2 +1,2 @@ bugfixes: - - "luks_device - on ``state=absent`` now also runs ``cryptsetup erase`` as well as a built-in LUKS signature cleaner to make sure the device is no longer accepted as a LUKS device (https://github.com/ansible-collections/community.crypto/issues/326, https://github.com/ansible-collections/community.crypto/pull/327)." + - "luks_device - now also runs a built-in LUKS signature cleaner on ``state=absent`` to make sure that also the secondary LUKS2 header is wiped when older versions of wipefs are used (https://github.com/ansible-collections/community.crypto/issues/326, https://github.com/ansible-collections/community.crypto/pull/327)." diff --git a/plugins/modules/luks_device.py b/plugins/modules/luks_device.py index 9c11bb1d4..e9bff73ff 100644 --- a/plugins/modules/luks_device.py +++ b/plugins/modules/luks_device.py @@ -535,18 +535,12 @@ def run_luks_close(self, name): if result[RETURN_CODE] != 0: raise ValueError('Error while closing LUKS container %s' % (name)) - def run_luks_erase(self, device, ignore_errors=False): - result = self._run_command([self._cryptsetup_bin, 'erase', '-q', device]) - if result[RETURN_CODE] != 0 and not ignore_errors: - raise ValueError('Error while erasing LUKS container %s' % (device)) - def run_luks_remove(self, device): wipefs_bin = self._module.get_bin_path('wipefs', True) name = self.get_container_name_by_device(device) if name is not None: self.run_luks_close(name) - self.run_luks_erase(device, ignore_errors=True) result = self._run_command([wipefs_bin, '--all', device]) if result[RETURN_CODE] != 0: raise ValueError('Error while wiping luks container %s: %s' diff --git a/tests/unit/plugins/modules/test_luks_device.py b/tests/unit/plugins/modules/test_luks_device.py index 3872ab29a..10eec80ee 100644 --- a/tests/unit/plugins/modules/test_luks_device.py +++ b/tests/unit/plugins/modules/test_luks_device.py @@ -46,16 +46,9 @@ def test_get_container_device_by_name(monkeypatch): def test_run_luks_remove(monkeypatch): - counter = [0] - def run_command_check(self, command): - # check that 'cryptsetup erase' and 'wipefs' commands are actually called - if counter[0] == 0: - assert command[0] == "cryptsetup" - assert command[1] == "erase" - if counter[0] == 1: - assert command[0] == "wipefs" - counter[0] += 1 + # check that wipefs command is actually called + assert command[0] == "wipefs" return [0, "", ""] module = DummyModule() From c892d3ca6da95f83978be6c21be4dde04499f813 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Wed, 10 Nov 2021 07:16:32 +0100 Subject: [PATCH 8/8] Improve error messages. --- plugins/modules/luks_device.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/modules/luks_device.py b/plugins/modules/luks_device.py index e9bff73ff..ddaedd24c 100644 --- a/plugins/modules/luks_device.py +++ b/plugins/modules/luks_device.py @@ -543,7 +543,7 @@ def run_luks_remove(self, device): self.run_luks_close(name) result = self._run_command([wipefs_bin, '--all', device]) if result[RETURN_CODE] != 0: - raise ValueError('Error while wiping luks container %s: %s' + raise ValueError('Error while wiping LUKS container signatures for %s: %s' % (device, result[STDERR])) # For LUKS2, sometimes both `cryptsetup erase` and `wipefs` do **not** @@ -552,7 +552,7 @@ def run_luks_remove(self, device): try: wipe_luks_headers(device) except Exception as exc: - raise ValueError('Error while wiping luks container %s: %s' % (device, exc)) + raise ValueError('Error while wiping LUKS container signatures for %s: %s' % (device, exc)) def run_luks_add_key(self, device, keyfile, passphrase, new_keyfile, new_passphrase, pbkdf):