curtin-dev team mailing list archive

[Olivier Gayot <mp+461402@xxxxxxxxxxxxxxxxxx>]

I'm not familiar with ZFS but your implementation of the encryption looks sensible to me.

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:

Consider using secrets.token_bytes(32)? It's supposed to be a more secure alternative when doing cryptography.

https://docs.python.org/3/library/secrets.html#secrets.token_bytes

> +                    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,

I know that it's common in curtin to use bitshift to express sizes but it still does not feel natural to me. Since `zfs create -V` can work with suffixes ; can we use `20M` instead? I assume they use powers of 2 and not powers of 10

> +        )
> +
> +        # 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.

minor: s/passphase/passphrase/

> +* 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

I think it would be useful to emphasize that we expect here the path to a file that already exists in the live installer environment; or something like that.

> +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.