curtin-dev team mailing list archive

[Olivier Gayot <mp+458460@xxxxxxxxxxxxxxxxxx>]

Thanks! I only have minor suggestions. Nothing blocking really.

Diff comments:

> diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst
> index 45b9bbe..30e52aa 100644
> --- a/doc/topics/storage.rst
> +++ b/doc/topics/storage.rst
> @@ -886,6 +886,15 @@ system will prompt for this password in order to mount the disk.
>  The ``keyfile`` contains the password of the encryption key.  The target
>  system will prompt for this password in order to mount the disk.
>  
> +A special case of ``keyfile`` are the values ``/dev/urandom`` and
> +``/dev/random``, which indicate that this ``keyfile`` value will be used in
> +enterity to decrypt the device.  In this case, the keyfile is passed along to

minor: s/enterity/entirety/ ?

> +the crypttab, as the third field.
> +
> +.. note::
> +  ``/dev/urandom`` and ``/dev/random`` are functionally equivalent on modern
> +  systems.
> +
>  Exactly one of **key** and **keyfile** must be supplied.
>  
>  **preserve**: *true, false*
> @@ -893,13 +902,16 @@ Exactly one of **key** and **keyfile** must be supplied.
>  If the ``preserve`` option is True, curtin will verify the dm-crypt device
>  specified is composed of the device specified in ``volume``.
>  
> -
>  **wipe**: *superblock, superblock-recursive, pvremove, zero, random*
>  
>  If ``wipe`` option is set, and ``preserve`` is False, curtin will wipe the
>  contents of the dm-crypt device.  Curtin skips wipe settings if it creates
>  the dm-crypt volume.
>  
> +**options**: *<list of man 5 options strings>*

I think you meant

**options**: *<list of man 5 crypttab options strings>*

> +
> +Options to pass to crypttab, as the fourth field.  See man 5 crypttab for
> +details. The default is ``luks``.

I think there is a possible misinterpretation of what a list means in this context (i.e., an actual YAML list vs comma separated values (which is what we expect for mount options))

I'd suggest:
```
-The default is ``luks``
+The default is ``[luks]``
```

>  
>  .. note::
>  
> diff --git a/tests/integration/test_block_meta.py b/tests/integration/test_block_meta.py
> index acd12e1..d7dcc87 100644
> --- a/tests/integration/test_block_meta.py
> +++ b/tests/integration/test_block_meta.py
> @@ -1285,6 +1297,38 @@ table-length: 256'''.encode()
>                       partition_type='82'))
>  
>      @parameterized.expand(((1,), (2,)))
> +    def test_cryptoswap(self, sv=2):
> +        self.img = self.tmp_path('image.img')
> +        config = StorageConfigBuilder(version=sv)
> +        config.add_image(path=self.img, create=True, size='200M',
> +                         ptable='msdos')
> +        p1 = config.add_part(
> +            number=1, offset=1 << 20, size=19 << 20, flag='swap'
> +        )
> +        cryptoswap = f"cryptoswap-{self.random_string(length=6)}"
> +        dmc1 = config.add_dmcrypt(
> +            volume=p1,
> +            dm_name=cryptoswap,
> +            keyfile="/dev/urandom",
> +            options=["swap", "initramfs"],
> +        )
> +        config.add_format(part=dmc1, fstype="swap")
> +        self.run_bm(config.render())
> +
> +        self.assertPartitions(
> +            PartData(number=1, offset=1 << 20, size=19 << 20, boot=False,
> +                     partition_type='82'))
> +
> +        crypttab_path = Path(self.fstab_dir) / "crypttab"
> +        with open(crypttab_path) as fp:
> +            crypttab = fp.read()
> +        tokens = re.split(r'\s', crypttab)

I assume it doesn't make a difference in your test case, but I'd expect r'\s+' to do a better split if columns are separated by multiple whitespaces.

> +        self.assertEqual(cryptoswap, tokens[0])
> +        self.assertTrue(tokens[1].startswith("UUID="))
> +        self.assertEqual("/dev/urandom", tokens[2])
> +        self.assertEqual("swap,initramfs", tokens[3])
> +
> +    @parameterized.expand(((1,), (2,)))
>      def test_msftres(self, sv):
>          self.img = self.tmp_path('image.img')
>          config = StorageConfigBuilder(version=sv)


-- 
https://code.launchpad.net/~dbungert/curtin/+git/curtin/+merge/458460
Your team curtin developers is subscribed to branch curtin:master.