Merge ~dbungert/curtin:cryptoswap into curtin:master
- Git
- lp:~dbungert/curtin
- cryptoswap
- Merge into master
Proposed by
Dan Bungert
Status: | Merged |
---|---|
Merged at revision: | 1997a1614e774cbd152fa33059d53417a641dbd6 |
Proposed branch: | ~dbungert/curtin:cryptoswap |
Merge into: | curtin:master |
Diff against target: |
348 lines (+196/-17) 5 files modified
curtin/commands/block_meta.py (+11/-3) doc/topics/storage.rst (+13/-1) tests/integration/test_block_meta.py (+45/-1) tests/unittests/test_commands_block_meta.py (+126/-12) tools/vmtest-system-setup (+1/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Olivier Gayot | Approve | ||
Server Team CI bot | continuous-integration | Approve | |
Chris Peterson | Approve | ||
Review via email: mp+458460@code.launchpad.net |
Commit message
Add features to the dm_crypt action to support cryptoswap
Description of the change
To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote : | # |
review:
Approve
(continuous-integration)
Revision history for this message
Chris Peterson (cpete) : | # |
review:
Approve
Revision history for this message
Olivier Gayot (ogayot) wrote : | # |
Thanks! I only have minor suggestions. Nothing blocking really.
Revision history for this message
Dan Bungert (dbungert) : | # |
Revision history for this message
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:1997a1614e7
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
review:
Approve
(continuous-integration)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py |
2 | index ebae27c..8ba7a55 100644 |
3 | --- a/curtin/commands/block_meta.py |
4 | +++ b/curtin/commands/block_meta.py |
5 | @@ -1618,6 +1618,8 @@ def dm_crypt_handler(info, storage_config, context): |
6 | keysize = info.get('keysize') |
7 | cipher = info.get('cipher') |
8 | dm_name = info.get('dm_name') |
9 | + options = ','.join(info.get('options', ['luks'])) |
10 | + crypttab_keyfile = 'none' |
11 | if not dm_name: |
12 | dm_name = info.get('id') |
13 | dmcrypt_dev = os.path.join("/dev", "mapper", dm_name) |
14 | @@ -1634,8 +1636,13 @@ def dm_crypt_handler(info, storage_config, context): |
15 | if 'keyfile' in info: |
16 | if 'key' in info: |
17 | raise ValueError("cannot specify both key and keyfile") |
18 | - keyfile_is_tmp = False |
19 | keyfile = info['keyfile'] |
20 | + if keyfile in ("/dev/random", "/dev/urandom"): |
21 | + crypttab_keyfile = keyfile |
22 | + keyfile = tempfile.mkstemp()[1] |
23 | + keyfile_is_tmp = True |
24 | + else: |
25 | + keyfile_is_tmp = False |
26 | elif 'key' in info: |
27 | # TODO: this is insecure, find better way to do this |
28 | key = info.get('key') |
29 | @@ -1727,8 +1734,9 @@ def dm_crypt_handler(info, storage_config, context): |
30 | state_dir = os.path.dirname(state['fstab']) |
31 | crypt_tab_location = os.path.join(state_dir, "crypttab") |
32 | uuid = block.get_volume_uuid(volume_path) |
33 | - util.write_file(crypt_tab_location, |
34 | - "%s UUID=%s none luks\n" % (dm_name, uuid), omode="a") |
35 | + util.write_file( |
36 | + crypt_tab_location, |
37 | + f"{dm_name} UUID={uuid} {crypttab_keyfile} {options}\n", omode="a") |
38 | else: |
39 | LOG.info("fstab configuration is not present in environment, so \ |
40 | cannot locate an appropriate directory to write crypttab in \ |
41 | diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst |
42 | index 45b9bbe..97e900d 100644 |
43 | --- a/doc/topics/storage.rst |
44 | +++ b/doc/topics/storage.rst |
45 | @@ -886,6 +886,15 @@ system will prompt for this password in order to mount the disk. |
46 | The ``keyfile`` contains the password of the encryption key. The target |
47 | system will prompt for this password in order to mount the disk. |
48 | |
49 | +A special case of ``keyfile`` are the values ``/dev/urandom`` and |
50 | +``/dev/random``, which indicate that this ``keyfile`` value will be used in |
51 | +entirety to decrypt the device. In this case, the keyfile is passed along to |
52 | +the crypttab, as the third field. |
53 | + |
54 | +.. note:: |
55 | + ``/dev/urandom`` and ``/dev/random`` are functionally equivalent on modern |
56 | + systems. |
57 | + |
58 | Exactly one of **key** and **keyfile** must be supplied. |
59 | |
60 | **preserve**: *true, false* |
61 | @@ -893,13 +902,16 @@ Exactly one of **key** and **keyfile** must be supplied. |
62 | If the ``preserve`` option is True, curtin will verify the dm-crypt device |
63 | specified is composed of the device specified in ``volume``. |
64 | |
65 | - |
66 | **wipe**: *superblock, superblock-recursive, pvremove, zero, random* |
67 | |
68 | If ``wipe`` option is set, and ``preserve`` is False, curtin will wipe the |
69 | contents of the dm-crypt device. Curtin skips wipe settings if it creates |
70 | the dm-crypt volume. |
71 | |
72 | +**options**: *<list of man 5 crypttab options strings>* |
73 | + |
74 | +Options to pass to crypttab, as the fourth field. See man 5 crypttab for |
75 | +details. The default is ``[luks]``. |
76 | |
77 | .. note:: |
78 | |
79 | diff --git a/tests/integration/test_block_meta.py b/tests/integration/test_block_meta.py |
80 | index acd12e1..ec9b33c 100644 |
81 | --- a/tests/integration/test_block_meta.py |
82 | +++ b/tests/integration/test_block_meta.py |
83 | @@ -6,6 +6,7 @@ import contextlib |
84 | import json |
85 | import os |
86 | from parameterized import parameterized |
87 | +from pathlib import Path |
88 | import re |
89 | import sys |
90 | from typing import Optional |
91 | @@ -192,6 +193,15 @@ class StorageConfigBuilder: |
92 | for action in self.config: |
93 | action['preserve'] = True |
94 | |
95 | + def add_dmcrypt(self, *, volume, dm_name=None, **kw): |
96 | + if dm_name is None: |
97 | + dm_name = CiTestCase.random_string() |
98 | + return self._add( |
99 | + type='dm_crypt', |
100 | + volume=volume['id'], |
101 | + dm_name=dm_name, |
102 | + **kw) |
103 | + |
104 | |
105 | class TestBlockMeta(IntegrationTestCase): |
106 | def setUp(self): |
107 | @@ -239,15 +249,17 @@ class TestBlockMeta(IntegrationTestCase): |
108 | with open(config_path, 'w') as fp: |
109 | yaml.dump(config, fp) |
110 | |
111 | + self.fstab_dir = self.tmp_dir() |
112 | cmd_env = kwargs.pop('env', {}) |
113 | cmd_env.update({ |
114 | 'PATH': os.environ['PATH'], |
115 | 'CONFIG': config_path, |
116 | 'WORKING_DIR': '/tmp', |
117 | - 'OUTPUT_FSTAB': self.tmp_path('fstab'), |
118 | + 'OUTPUT_FSTAB': self.tmp_path('fstab', _dir=self.fstab_dir), |
119 | 'OUTPUT_INTERFACES': '', |
120 | 'OUTPUT_NETWORK_STATE': '', |
121 | 'OUTPUT_NETWORK_CONFIG': '', |
122 | + 'TARGET_MOUNT_POINT': self.tmp_dir(), |
123 | }) |
124 | |
125 | cmd = [ |
126 | @@ -1285,6 +1297,38 @@ table-length: 256'''.encode() |
127 | partition_type='82')) |
128 | |
129 | @parameterized.expand(((1,), (2,))) |
130 | + def test_cryptoswap(self, sv=2): |
131 | + self.img = self.tmp_path('image.img') |
132 | + config = StorageConfigBuilder(version=sv) |
133 | + config.add_image(path=self.img, create=True, size='200M', |
134 | + ptable='msdos') |
135 | + p1 = config.add_part( |
136 | + number=1, offset=1 << 20, size=19 << 20, flag='swap' |
137 | + ) |
138 | + cryptoswap = f"cryptoswap-{self.random_string(length=6)}" |
139 | + dmc1 = config.add_dmcrypt( |
140 | + volume=p1, |
141 | + dm_name=cryptoswap, |
142 | + keyfile="/dev/urandom", |
143 | + options=["swap", "initramfs"], |
144 | + ) |
145 | + config.add_format(part=dmc1, fstype="swap") |
146 | + self.run_bm(config.render()) |
147 | + |
148 | + self.assertPartitions( |
149 | + PartData(number=1, offset=1 << 20, size=19 << 20, boot=False, |
150 | + partition_type='82')) |
151 | + |
152 | + crypttab_path = Path(self.fstab_dir) / "crypttab" |
153 | + with open(crypttab_path) as fp: |
154 | + crypttab = fp.read() |
155 | + tokens = re.split(r'\s+', crypttab) |
156 | + self.assertEqual(cryptoswap, tokens[0]) |
157 | + self.assertTrue(tokens[1].startswith("UUID=")) |
158 | + self.assertEqual("/dev/urandom", tokens[2]) |
159 | + self.assertEqual("swap,initramfs", tokens[3]) |
160 | + |
161 | + @parameterized.expand(((1,), (2,))) |
162 | def test_msftres(self, sv): |
163 | self.img = self.tmp_path('image.img') |
164 | config = StorageConfigBuilder(version=sv) |
165 | diff --git a/tests/unittests/test_commands_block_meta.py b/tests/unittests/test_commands_block_meta.py |
166 | index 9d7d0f3..5adb46c 100644 |
167 | --- a/tests/unittests/test_commands_block_meta.py |
168 | +++ b/tests/unittests/test_commands_block_meta.py |
169 | @@ -2030,10 +2030,10 @@ class TestLvmPartitionHandler(CiTestCase): |
170 | self.assertEqual(0, self.m_subp.call_count) |
171 | |
172 | |
173 | -class TestDmCryptHandler(CiTestCase): |
174 | +class DmCryptCommon(CiTestCase): |
175 | |
176 | def setUp(self): |
177 | - super(TestDmCryptHandler, self).setUp() |
178 | + super().setUp() |
179 | |
180 | basepath = 'curtin.commands.block_meta.' |
181 | self.add_patch(basepath + 'get_path_to_storage_volume', 'm_getpath') |
182 | @@ -2047,7 +2047,26 @@ class TestDmCryptHandler(CiTestCase): |
183 | self.keyfile = self.random_string() |
184 | self.cipher = self.random_string() |
185 | self.keysize = self.random_string() |
186 | - self.config = { |
187 | + self.m_block.zkey_supported.return_value = False |
188 | + self.block_uuids = [random_uuid() for unused in range(2)] |
189 | + self.m_block.get_volume_uuid.side_effect = self.block_uuids |
190 | + |
191 | + self.m_which.return_value = False |
192 | + self.fstab = self.tmp_path('fstab') |
193 | + self.crypttab = os.path.join(os.path.dirname(self.fstab), 'crypttab') |
194 | + self.m_load_env.return_value = {'fstab': self.fstab, |
195 | + 'target': self.target} |
196 | + |
197 | + def setUpStorageConfig(self, config): |
198 | + self.config = config |
199 | + self.storage_config = block_meta.extract_storage_ordered_dict(config) |
200 | + |
201 | + |
202 | +class TestDmCryptHandler(DmCryptCommon): |
203 | + |
204 | + def setUp(self): |
205 | + super().setUp() |
206 | + self.setUpStorageConfig({ |
207 | 'storage': { |
208 | 'version': 1, |
209 | 'config': [ |
210 | @@ -2073,15 +2092,7 @@ class TestDmCryptHandler(CiTestCase): |
211 | 'keyfile': self.keyfile}, |
212 | ], |
213 | } |
214 | - } |
215 | - self.storage_config = ( |
216 | - block_meta.extract_storage_ordered_dict(self.config)) |
217 | - self.m_block.zkey_supported.return_value = False |
218 | - self.m_which.return_value = False |
219 | - self.fstab = self.tmp_path('fstab') |
220 | - self.crypttab = os.path.join(os.path.dirname(self.fstab), 'crypttab') |
221 | - self.m_load_env.return_value = {'fstab': self.fstab, |
222 | - 'target': self.target} |
223 | + }) |
224 | |
225 | def test_dm_crypt_calls_cryptsetup(self): |
226 | """ verify dm_crypt calls (format, open) w/ correct params""" |
227 | @@ -2402,6 +2413,109 @@ class TestDmCryptHandler(CiTestCase): |
228 | info, self.storage_config, empty_context) |
229 | |
230 | |
231 | +class TestCrypttab(DmCryptCommon): |
232 | + |
233 | + def test_multi_dm_crypt(self): |
234 | + """ verify that multiple dm_crypt calls result in the data for both |
235 | + being present in crypttab""" |
236 | + |
237 | + self.setUpStorageConfig({ |
238 | + 'storage': { |
239 | + 'version': 1, |
240 | + 'config': [ |
241 | + {'grub_device': True, |
242 | + 'id': 'sda', |
243 | + 'name': 'sda', |
244 | + 'path': '/wark/xxx', |
245 | + 'ptable': 'msdos', |
246 | + 'type': 'disk', |
247 | + 'wipe': 'superblock'}, |
248 | + {'device': 'sda', |
249 | + 'id': 'sda-part1', |
250 | + 'name': 'sda-part1', |
251 | + 'number': 1, |
252 | + 'size': '511705088B', |
253 | + 'type': 'partition'}, |
254 | + {'id': 'dmcrypt0', |
255 | + 'type': 'dm_crypt', |
256 | + 'dm_name': 'cryptroot', |
257 | + 'volume': 'sda-part1', |
258 | + 'cipher': self.cipher, |
259 | + 'keysize': self.keysize, |
260 | + 'keyfile': self.keyfile}, |
261 | + {'device': 'sda', |
262 | + 'id': 'sda-part2', |
263 | + 'name': 'sda-part2', |
264 | + 'number': 2, |
265 | + 'size': '511705088B', |
266 | + 'type': 'partition'}, |
267 | + {'id': 'dmcrypt1', |
268 | + 'type': 'dm_crypt', |
269 | + 'dm_name': 'cryptfoo', |
270 | + 'volume': 'sda-part2', |
271 | + 'cipher': self.cipher, |
272 | + 'keysize': self.keysize, |
273 | + 'keyfile': self.keyfile}, |
274 | + ], |
275 | + } |
276 | + }) |
277 | + |
278 | + for i in range(2): |
279 | + self.m_getpath.return_value = self.random_string() |
280 | + info = self.storage_config['dmcrypt' + str(i)] |
281 | + block_meta.dm_crypt_handler( |
282 | + info, self.storage_config, empty_context |
283 | + ) |
284 | + |
285 | + data = util.load_file(self.crypttab) |
286 | + self.assertEqual( |
287 | + f"cryptroot UUID={self.block_uuids[0]} none luks\n" |
288 | + f"cryptfoo UUID={self.block_uuids[1]} none luks\n", |
289 | + data |
290 | + ) |
291 | + |
292 | + def test_cryptoswap(self): |
293 | + """ Check the keyfile and options features needed for cryptoswap """ |
294 | + |
295 | + self.setUpStorageConfig({ |
296 | + 'storage': { |
297 | + 'version': 1, |
298 | + 'config': [ |
299 | + {'grub_device': True, |
300 | + 'id': 'sda', |
301 | + 'name': 'sda', |
302 | + 'path': '/wark/xxx', |
303 | + 'ptable': 'msdos', |
304 | + 'type': 'disk', |
305 | + 'wipe': 'superblock'}, |
306 | + {'device': 'sda', |
307 | + 'id': 'sda-part1', |
308 | + 'name': 'sda-part1', |
309 | + 'number': 1, |
310 | + 'size': '511705088B', |
311 | + 'type': 'partition'}, |
312 | + {'id': 'dmcrypt0', |
313 | + 'type': 'dm_crypt', |
314 | + 'dm_name': 'cryptswap', |
315 | + 'volume': 'sda-part1', |
316 | + 'options': ["swap", "initramfs"], |
317 | + 'keyfile': "/dev/urandom"}, |
318 | + ], |
319 | + } |
320 | + }) |
321 | + |
322 | + self.m_getpath.return_value = self.random_string() |
323 | + block_meta.dm_crypt_handler( |
324 | + self.storage_config['dmcrypt0'], self.storage_config, empty_context |
325 | + ) |
326 | + |
327 | + uuid = self.block_uuids[0] |
328 | + self.assertEqual( |
329 | + f"cryptswap UUID={uuid} /dev/urandom swap,initramfs\n", |
330 | + util.load_file(self.crypttab), |
331 | + ) |
332 | + |
333 | + |
334 | class TestRaidHandler(CiTestCase): |
335 | |
336 | def setUp(self): |
337 | diff --git a/tools/vmtest-system-setup b/tools/vmtest-system-setup |
338 | index 5172741..2f82464 100755 |
339 | --- a/tools/vmtest-system-setup |
340 | +++ b/tools/vmtest-system-setup |
341 | @@ -14,6 +14,7 @@ esac |
342 | DEPS=( |
343 | build-essential |
344 | cloud-image-utils |
345 | + cryptsetup |
346 | git |
347 | make |
348 | net-tools |
PASSED: Continuous integration, rev:6afaafefaf4 12a61bda2eb125d 354d0b6daa6aab /jenkins. canonical. com/server- team/job/ curtin- ci/216/ /jenkins. canonical. com/server- team/job/ curtin- ci/nodes= metal-amd64/ 216/ /jenkins. canonical. com/server- team/job/ curtin- ci/nodes= metal-arm64/ 216/ /jenkins. canonical. com/server- team/job/ curtin- ci/nodes= metal-ppc64el/ 216/ /jenkins. canonical. com/server- team/job/ curtin- ci/nodes= metal-s390x/ 216/
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild: /jenkins. canonical. com/server- team/job/ curtin- ci/216/ /rebuild
https:/