curtin-dev team mailing list archive

[Olivier Gayot <mp+451722@xxxxxxxxxxxxxxxxxx>]

Olivier Gayot has proposed merging ~ogayot/curtin:zkey-recovery into curtin:master.

Commit message:
block_meta: also add recovery key if zkey is used

When zkey is properly setup, we do not invoke cryptsetup luksFormat
ourselves. Instead we lean on zkey to invoke cryptsetup luksFormat for
us.

zkey seems to have no native support for invoking cryptsetup luksAddKey,
so we need to manually call it if we want to add a recovery key in a
second slot.

Signed-off-by: Olivier Gayot <olivier.gayot@xxxxxxxxxxxxx>


Requested reviews:
  curtin developers (curtin-dev)

For more details, see:
https://code.launchpad.net/~ogayot/curtin/+git/curtin/+merge/451722

Add support for recovery key when zkey is used on s390x.

On s390x, if the hardware is present, we lean on zkey (from src:s390-tools) instead of calling cryptsetup commands manually. There seem to be no native support for adding a recovery key using zkey so, let's call cryptsetup luksAddKey manually.

In my test, it shows cryptsetup luksDump shows the following after adding a recovery key.

LUKS header information
Version:       	2
Epoch:         	5
Metadata area: 	16384 [bytes]
Keyslots area: 	16744448 [bytes]
UUID:          	f44a90ff-730b-44e6-9194-78da37e46775
Label:         	(no label)
Subsystem:     	(no subsystem)
Flags:       	(no flags)

Data segments:
  0: crypt
	offset: 16777216 [bytes]
	length: (whole device)
	cipher: paes-xts-plain64
	sector: 4096 [bytes]

Keyslots:
  0: luks2
	Key:        1024 bits
	Priority:   normal
	Cipher:     aes-xts-plain64
	Cipher key: 512 bits
	PBKDF:      argon2id
	Time cost:  4
	Memory:     584806
	Threads:    4
	Salt:       ab a0 b3 24 85 63 54 f2 cb 9b ac 8c 32 a3 ff 97 
	            f3 6d ad 81 76 9d 01 95 dd 4b 07 c5 75 11 45 4f 
	AF stripes: 4000
	AF hash:    sha256
	Area offset:32768 [bytes]
	Area length:512000 [bytes]
	Digest ID:  0
  1: luks2
	Key:        1024 bits
	Priority:   normal
	Cipher:     aes-xts-plain64
	Cipher key: 512 bits
	PBKDF:      argon2id
	Time cost:  4
	Memory:     578882
	Threads:    4
	Salt:       86 22 de ad 25 11 4f 36 5a 16 f0 f9 07 41 0e 34 
	            48 5a 43 65 6d 32 c7 15 c6 11 9b 44 6e 48 9a b8 
	AF stripes: 4000
	AF hash:    sha256
	Area offset:544768 [bytes]
	Area length:512000 [bytes]
	Digest ID:  0
Tokens:
  0: paes-verification-pattern
Digests:
  0: pbkdf2
	Hash:       sha256
	Iterations: 50567
	Salt:       10 f3 68 d2 36 ce c7 91 3d b3 b7 77 8c 3a cb 5b 
	            e9 1a 29 bd 2c c3 0e 99 85 7d 42 f1 b2 02 a3 c2 
	Digest:     2b 56 0e b6 67 a5 c1 c7 66 8f 22 be a5 f1 1f 00 
	            ba 3b 16 83 0a 97 e7 d0 ee 79 c2 f4 38 f0 53 dd 

This looks correct from the functional standpoint, but I can hardly judge the security aspect.
-- 
Your team curtin developers is requested to review the proposed merge of ~ogayot/curtin:zkey-recovery into curtin:master.

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index 64bf393..ebae27c 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -1693,16 +1693,15 @@ def dm_crypt_handler(info, storage_config, context):
 
             util.subp(cmd)
 
-            # Should this be done as well if we are using zkey?
-            if recovery_keyfile is not None:
-                LOG.debug("Adding recovery key to %s", volume_path)
+        if recovery_keyfile is not None:
+            LOG.debug("Adding recovery key to %s", volume_path)
 
-                cmd = [
-                    "cryptsetup", "luksAddKey",
-                    "--key-file", keyfile,
-                    volume_path, recovery_keyfile]
+            cmd = [
+                "cryptsetup", "luksAddKey",
+                "--key-file", keyfile,
+                volume_path, recovery_keyfile]
 
-                util.subp(cmd)
+            util.subp(cmd)
 
         cmd = ["cryptsetup", "open", "--type", luks_type, volume_path, dm_name,
                "--key-file", keyfile]
diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py
index 1a1f65d..9d7d0f3 100644
--- a/tests/unittests/test_commands_block_meta.py
+++ b/tests/unittests/test_commands_block_meta.py
@@ -2198,6 +2198,70 @@ class TestDmCryptHandler(CiTestCase):
         self.m_subp.assert_has_calls(expected_calls)
         self.assertEqual(len(util.load_file(self.crypttab).splitlines()), 1)
 
+    def test_dm_crypt_zkey_cryptsetup_with_recovery_key(self):
+        """ verify dm_crypt zkey calls generates and run before crypt open."""
+
+        # zkey binary is present
+        self.m_block.zkey_supported.return_value = True
+        self.m_which.return_value = "/my/path/to/zkey"
+        volume_path = self.random_string()
+        self.m_getpath.return_value = volume_path
+        volume_byid = "/dev/disk/by-id/ccw-%s" % volume_path
+        self.m_block.disk_to_byid_path.return_value = volume_byid
+
+        recovery_keyfile = self.random_string()
+
+        config = {
+            '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',
+                     'cipher': self.cipher,
+                     'keysize': self.keysize,
+                     'keyfile': self.keyfile,
+                     'recovery_keyfile': recovery_keyfile},
+                ],
+            }
+        }
+        storage_config = block_meta.extract_storage_ordered_dict(config)
+
+        info = storage_config['dmcrypt0']
+
+        volume_name = "%s:%s" % (volume_byid, info['dm_name'])
+        block_meta.dm_crypt_handler(info, storage_config, empty_context)
+        expected_calls = [
+            call(['zkey', 'generate', '--xts', '--volume-type', 'luks2',
+                  '--sector-size', '4096', '--name', info['dm_name'],
+                  '--description',
+                  'curtin generated zkey for %s' % volume_name,
+                  '--volumes', volume_name], capture=True),
+            call(['zkey', 'cryptsetup', '--run', '--volumes', volume_byid,
+                  '--batch-mode', '--key-file', self.keyfile], capture=True),
+            call(['cryptsetup', 'luksAddKey',
+                  '--key-file', self.keyfile,
+                  volume_path, recovery_keyfile]),
+            call(['cryptsetup', 'open', '--type', 'luks2', volume_path,
+                  info['dm_name'], '--key-file', self.keyfile]),
+        ]
+        self.m_subp.assert_has_calls(expected_calls)
+        self.assertEqual(len(util.load_file(self.crypttab).splitlines()), 1)
+
     def test_dm_crypt_zkey_gen_failure_fallback_to_cryptsetup(self):
         """ verify dm_cyrpt zkey generate err falls back cryptsetup format. """