Merge ~dbungert/curtin:cryptoswap-dmc-type into curtin:master

[Dan Bungert]

dm_crypt: create and use cryptoswap as PLAIN

Previously, cryptoswap was acting strangely, in that at install time it
was considered to be a LUKS2 device and durign actual system use it
would be considered a PLAIN device, in the cryptsetup(8) meanings of
these terms.

This caused problems when attempting to reference the device, as we
would do get_volume_spec(), the returned /dev/disk/by-id/dm-uuid-...
value would be wrong - it would change from dm-uuid-CRYPT-LUKS2-... to
dm-uuid-CRYPT-PLAIN-...

As the real runtime behavior is to be a PLAIN device for cryptoswap,
stop creating it as a LUKS2 device and just use it as a PLAIN device,
which doesn't involve a "create" step at all, you just open it as PLAIN
directly.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:b237c99786eb5d7f65363de84e540ced2a3d1691
https://jenkins.canonical.com/server-team/job/curtin-ci/226/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/226/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/226/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/226/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/226/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/226//rebuild

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

Can you expand the commit message a whole bunch please? This is about the case where /dev/random is used as a keyfile, in which case the dm type of the device would be created as luks in the live system but then be plain in the installed system and changing to a world where it will be plain in both?

Code changes look good but I have a worry about temporary file handling.

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:02b801325244176a8e128d6a5a76ed84cb44f4c9
https://jenkins.canonical.com/server-team/job/curtin-ci/231/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/231/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/231/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/231/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/231/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/231//rebuild

review: Approve (continuous-integration)

[Dan Bungert (dbungert)]

> Can you expand the commit message a whole bunch please?

Done, or let me know if you would prefer more.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:0bbd69f9725db580c666802a1c60683d11371ba0
https://jenkins.canonical.com/server-team/job/curtin-ci/232/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/232/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/232/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/232/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/232/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/232//rebuild

review: Approve (continuous-integration)

[Michael Hudson-Doyle (mwhudson)]

Thanks for the changes. Please remember to update the commit message in the MP if you are going to let jenkins squash merge this!

review: Approve

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:e01fe404124d6e03be3960d190dc0278334fb23a
https://jenkins.canonical.com/server-team/job/curtin-ci/234/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-amd64/234/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-arm64/234/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-ppc64el/234/
    SUCCESS: https://jenkins.canonical.com/server-team/job/curtin-ci/nodes=metal-s390x/234/

Click here to trigger a rebuild:
https://jenkins.canonical.com/server-team/job/curtin-ci/234//rebuild

review: Approve (continuous-integration)

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index c7bd0dd..ff72193 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -1633,28 +1633,29 @@ def dm_crypt_handler(info, storage_config, context):
     volume_path = get_path_to_storage_volume(volume, storage_config)
     volume_byid_path = block.disk_to_byid_path(volume_path)
 
+    create_dmcrypt = True
+    open_dmcrypt = False
     if 'keyfile' in info:
         if 'key' in info:
             raise ValueError("cannot specify both key and keyfile")
         keyfile = info['keyfile']
         if keyfile in ("/dev/random", "/dev/urandom"):
             crypttab_keyfile = keyfile
-            keyfile = tempfile.mkstemp()[1]
-            keyfile_is_tmp = True
-        else:
-            keyfile_is_tmp = False
+            luks_type = "plain"
+            open_dmcrypt = True
+            create_dmcrypt = False
+        remove_keyfile = False
     elif 'key' in info:
         # TODO: this is insecure, find better way to do this
         key = info.get('key')
         keyfile = tempfile.mkstemp()[1]
-        keyfile_is_tmp = True
+        remove_keyfile = True
         util.write_file(keyfile, key, mode=0o600)
     else:
         raise ValueError("encryption key or keyfile must be specified")
 
     recovery_keyfile = info.get('recovery_keyfile')
 
-    create_dmcrypt = True
     if preserve:
         dm_crypt_verify(dmcrypt_dev, volume_path)
         LOG.debug('dm_crypt %s already present, skipping create', dmcrypt_dev)
@@ -1664,6 +1665,7 @@ def dm_crypt_handler(info, storage_config, context):
         # if zkey is available, attempt to generate and use it; if it's not
         # available or fails to setup properly, fallback to normal cryptsetup
         # passing strict=False downgrades log messages to warnings
+        open_dmcrypt = True
         zkey_used = None
         if block.zkey_supported(strict=False):
             volume_name = "%s:%s" % (volume_byid_path, dm_name)
@@ -1710,12 +1712,13 @@ def dm_crypt_handler(info, storage_config, context):
 
             util.subp(cmd)
 
+    if open_dmcrypt:
         cmd = ["cryptsetup", "open", "--type", luks_type, volume_path, dm_name,
                "--key-file", keyfile]
 
         util.subp(cmd)
 
-        if keyfile_is_tmp:
+        if remove_keyfile:
             os.remove(keyfile)
 
     wipe_mode = info.get('wipe')
diff --git a/tests/integration/test_block_meta.py b/tests/integration/test_block_meta.py
index 568f466..2fc7c7d 100644
--- a/tests/integration/test_block_meta.py
+++ b/tests/integration/test_block_meta.py
@@ -1327,6 +1327,13 @@ table-length: 256'''.encode()
         self.assertEqual("/dev/urandom", tokens[2])
         self.assertEqual("swap,initramfs", tokens[3])
 
+        cmd = ["cryptsetup", "status", cryptoswap]
+        status = util.subp(cmd, capture=True)[0]
+        for line in status.splitlines():
+            key, _, value = line.strip().partition(':')
+            if key == "type":
+                self.assertEqual("PLAIN", value.strip())
+
     @parameterized.expand(((1,), (2,)))
     def test_msftres(self, sv):
         self.img = self.tmp_path('image.img')
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index c2d592d..727fba5 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -2413,6 +2413,108 @@ class TestDmCryptHandler(DmCryptCommon):
                 info, self.storage_config, empty_context)
 
 
+class TestDmCryptKeyfileRemoval(DmCryptCommon):
+    def setUp(self):
+        super().setUp()
+        self.tempkey = self.tmp_path('test_dm_crypt_key')
+        basepath = 'curtin.commands.block_meta.'
+        self.add_patch(basepath + 'os.remove', 'm_os_remove')
+
+    @patch('curtin.commands.block_meta.tempfile.mkstemp')
+    def test_dm_crypt_removes_tmpfile_if_key(self, m_mkstemp):
+        m_mkstemp.return_value = (None, self.tempkey)
+        self.setUpStorageConfig({
+            'storage': {
+                'version': 1,
+                'config': [
+                    {'grub_device': True,
+                     'id': 'sda',
+                     'name': 'sda',
+                     'path': '/wark/xxx',
+                     'ptable': 'msdos',
+                     'type': 'disk',
+                     'wipe': 'superblock'},
+                    {'device': 'sda',
+                     'id': 'sda-part1',
+                     'name': 'sda-part1',
+                     'number': 1,
+                     'size': '511705088B',
+                     'type': 'partition'},
+                    {'id': 'dmcrypt0',
+                     'type': 'dm_crypt',
+                     'dm_name': 'cryptroot',
+                     'volume': 'sda-part1',
+                     'key': 'passw0rd'},
+                ],
+            }
+        })
+        info = self.storage_config['dmcrypt0']
+        block_meta.dm_crypt_handler(info, self.storage_config, empty_context)
+        self.m_os_remove.assert_called_once_with(self.tempkey)
+
+    @patch('curtin.commands.block_meta.os.remove')
+    def test_dm_crypt_not_remove_keyfile(self, m_os_remove):
+        self.setUpStorageConfig({
+            'storage': {
+                'version': 1,
+                'config': [
+                    {'grub_device': True,
+                     'id': 'sda',
+                     'name': 'sda',
+                     'path': '/wark/xxx',
+                     'ptable': 'msdos',
+                     'type': 'disk',
+                     'wipe': 'superblock'},
+                    {'device': 'sda',
+                     'id': 'sda-part1',
+                     'name': 'sda-part1',
+                     'number': 1,
+                     'size': '511705088B',
+                     'type': 'partition'},
+                    {'id': 'dmcrypt0',
+                     'type': 'dm_crypt',
+                     'dm_name': 'cryptroot',
+                     'volume': 'sda-part1',
+                     'keyfile': self.tempkey},
+                ],
+            }
+        })
+        info = self.storage_config['dmcrypt0']
+        block_meta.dm_crypt_handler(info, self.storage_config, empty_context)
+        self.m_os_remove.assert_not_called()
+
+    @patch('curtin.commands.block_meta.os.remove')
+    def test_dm_crypt_not_remove_dev_random(self, m_os_remove):
+        self.setUpStorageConfig({
+            'storage': {
+                'version': 1,
+                'config': [
+                    {'grub_device': True,
+                     'id': 'sda',
+                     'name': 'sda',
+                     'path': '/wark/xxx',
+                     'ptable': 'msdos',
+                     'type': 'disk',
+                     'wipe': 'superblock'},
+                    {'device': 'sda',
+                     'id': 'sda-part1',
+                     'name': 'sda-part1',
+                     'number': 1,
+                     'size': '511705088B',
+                     'type': 'partition'},
+                    {'id': 'dmcrypt0',
+                     'type': 'dm_crypt',
+                     'dm_name': 'cryptroot',
+                     'volume': 'sda-part1',
+                     'keyfile': '/dev/random'},
+                ],
+            }
+        })
+        info = self.storage_config['dmcrypt0']
+        block_meta.dm_crypt_handler(info, self.storage_config, empty_context)
+        self.m_os_remove.assert_not_called()
+
+
 class TestCrypttab(DmCryptCommon):
 
     def test_multi_dm_crypt(self):