curtin-dev team mailing list archive

[Dan Bungert <mp+461402@xxxxxxxxxxxxxxxxxx>]

@ogayot: All feedback should be addressed, thanks!

Diff comments:

> diff --git a/curtin/block/zfs.py b/curtin/block/zfs.py
> index 01ba9b9..99973d5 100644
> --- a/curtin/block/zfs.py
> +++ b/curtin/block/zfs.py
> @@ -26,6 +29,97 @@ ZFS_UNSUPPORTED_ARCHES = ['i386']
>  ZFS_UNSUPPORTED_RELEASES = ['precise', 'trusty']
>  
>  
> +class ZPoolEncryption:
> +    def __init__(self, poolname, style, keyfile):
> +        self.poolname = poolname
> +        self.style = style
> +        self.keyfile = keyfile
> +        self.system_key = None
> +
> +    def get_system_key(self):
> +        if self.system_key is None:
> +            fd, self.system_key = tempfile.mkstemp()
> +            with open(fd, "wb") as writer:
> +                with open("/dev/urandom", "rb") as reader:

Done, thank you!  With a bonus that the unit test is less ugly.

> +                    writer.write(reader.read(32))
> +        return self.system_key
> +
> +    def validate(self):
> +        if self.style is None:
> +            return
> +
> +        if not self.poolname:
> +            raise ValueError("valid pool name required")
> +
> +        if self.style != "luks_keystore":
> +            raise ValueError(f"unrecognized encryption style {self.style}")
> +
> +        if not self.keyfile:
> +            raise ValueError(f"keyfile required when using {self.style}")
> +
> +        if not Path(self.keyfile).is_file():
> +            raise ValueError(f"invalid keyfile path {self.keyfile}")
> +
> +    def in_use(self):
> +        return self.style is not None
> +
> +    def dataset_properties(self):
> +        if not self.in_use():
> +            return {}
> +
> +        ks_system_key = self.get_system_key()
> +        return {
> +            "encryption": "on",
> +            "keylocation": f"file://{ks_system_key}",
> +            "keyformat": "raw",
> +            # as we'll be formatting this as another fs, ext4,
> +            # mounting before the ext4 is no help
> +            "canmount": "off",
> +        }
> +
> +    def setup(self, storage_config, context):
> +        if not self.in_use():
> +            return
> +
> +        # Create the dataset for the keystore.  This is a bit special as it
> +        # won't be ZFS despite being on the zpool.
> +        zfs_create(
> +            self.poolname, "keystore", {"encryption": "off"}, size=20 << 20,

The other curtin convention is util.human2bytes.  I can get behind moving away from the bitshifts.  I'm implementing a human2bytes call, what do you think?

I'm concerned about different tools having inconsistent implementations on defaults of power-of-2 and power-of-10.

Then I saw that human2bytes can actually return a float but has confirmed it is equal to the int but still returns it as a float anyhow, so I've fixed that.

> +        )
> +
> +        # cryptsetup format and open this keystore
> +        keystore_volume = f"/dev/zvol/{self.poolname}/keystore"
> +        cmd = ["cryptsetup", "luksFormat", keystore_volume, self.keyfile]
> +        util.subp(cmd)
> +        dm_name = f"keystore-{self.poolname}"
> +        cmd = [
> +            "cryptsetup", "open", "--type", "luks", keystore_volume, dm_name,
> +            "--key-file", self.keyfile,
> +        ]
> +        util.subp(cmd)
> +
> +        # format as ext4, mount it, move the previously-generated system key
> +        dmpath = f"/dev/mapper/{dm_name}"
> +        cmd = ["mke2fs", "-t", "ext4", dmpath, "-L", dm_name]
> +        util.subp(cmd, capture=True)
> +
> +        keystore_root = f"/run/keystore/{self.poolname}"
> +        with util.mount(dmpath, keystore_root):
> +            ks_system_key = f"{keystore_root}/system.key"
> +            shutil.move(self.system_key, ks_system_key)
> +            Path(ks_system_key).chmod(0o400)
> +
> +            # update the pool with the real keylocation
> +            keylocation = f"keylocation=file://{ks_system_key}"
> +            cmd = ["zfs", "set", keylocation, self.poolname]
> +            util.subp(cmd, capture=True)
> +
> +        # The keystore crypsetup device needs to be closed so we can (at the
> +        # end of the install) allow the zpool to complete the export step.
> +        # Once we have moved the key over we can close the keystore.
> +        util.subp(["cryptsetup", "close", dmpath], capture=True)
> +
> +
>  def _join_flags(optflag, params):
>      """
>      Insert optflag for each param in params and return combined list.
> diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst
> index 7650c4d..60bedb1 100644
> --- a/doc/topics/storage.rst
> +++ b/doc/topics/storage.rst
> @@ -1162,6 +1162,27 @@ set is used.
>      - sda1
>     mountpoint: /
>  
> +**encryption_style**: *luks_keystore, null*
> +
> +If set to **luks_keystore**, an encrypted pool is created using a LUKS backed
> +keystore system.  When **encryption_style** is not null, **keyfile** is
> +required.
> +
> +This works as follows:
> +* A LUKS device is created as a ZFS dataset in the ZPool.
> +* The supplied passphase (see **keyfile**) is used to encrypt the LUKS device.

Thanks!

> +* The real key for the ZFS dataset is contained in the LUKS device as a simple
> +  file.
> +* The zpool is decrypted using this simple file inside the encrypted LUKS
> +  device.
> +
> +Default value is **null**, which means the resulting zpool is unencrypted.
> +
> +**keyfile**: *<keyfile>*
> +
> +The ``keyfile`` contains the password of the encryption key.  The target

Done.  Also fixed the related comment in dm_crypt.  Also added a bit more text about how the keystore works.

> +system will prompt for this password in order to mount the zpool.
> +
>  ZFS Command
>  ~~~~~~~~~~~~~~
>  ZFS Support is **experimental**.


-- 
https://code.launchpad.net/~dbungert/curtin/+git/curtin/+merge/461402
Your team curtin developers is requested to review the proposed merge of ~dbungert/curtin:zpool-keystore into curtin:master.