[Libguestfs] [PATCH 1/7] New APIs: cryptsetup-open and cryptsetup-close.

[Pino Toscano]

On Monday, 30 March 2020 17:03:36 CEST Richard W.M. Jones wrote:
> This commit deprecates luks-open/luks-open-ro/luks-close for the more
> generic sounding names cryptsetup-open/cryptsetup-close, which also
> correspond directly to the cryptsetup commands.
> 
> The optional cryptsetup-open readonly flag is used to replace the
> functionality of luks-open-ro.
> 
> The optional cryptsetup-open crypttype parameter can be used to select
> the type (corresponding to cryptsetup open --type), which allows us to
> open BitLocker-encrypted disks with no extra effort.  As a convenience
> the crypttype parameter may be omitted, and libguestfs will use a
> heuristic (based on vfs-type output) to try to determine the correct
> type to use.

At least in my (non extensive) tests with cryptsetup, it seems it can
detect the right format even without --type=format or the luksOpen/etc
aliases.

What I'd do is:
- drop the autodetection: it is a mild duplication of what cryptsetup
  already does, and adding it in the API now means that it will be
  stuck there...
- maybe (need to think about it) make the crypttype parameter mandatory,
  so the user has to select the right format

> The deprecated functions and the new functions are all (re-)written in
> OCaml.

Regarding the deprecated functions: wouldn't it be better to have them
in the library, rather than in the daemon? The machinery needed in the
library is more or less than same anyway, and this way we can remove
two RPCs in the daemon.

Also, please bump the versions of the new APIs.

> +let cryptsetup_open ?(readonly = false) ?crypttype device key mapname =
> +  (* /dev/mapper/mapname must not exist already. *)

  /* Sanity check: /dev/mapper/mapname must not exist already.  Note
   * that the device-mapper control device (/dev/mapper/control) is
   * always there, so you can't ever have mapname == "control".
   */

This whole comment seems useful, it would be a pity to lose it in the
conversion. Also, unrelated to this patch: IMHO it would be a good idea
to explicitly note this limitation in the API docs.

> +  (* Write the key to a temporary file. *)
> +  let keyfile, chan = Filename.open_temp_file "crypt" ".key" in
> +  fchmod (descr_of_out_channel chan) 0o600;

This fchmod is not needed, as temporary files are already 600 by
default.

> +  output_string chan key;
> +  close_out chan;
> +
> +  (* Make sure we always remove the temporary file. *)
> +  protect ~f:(
> +    fun () ->
> +      let args = ref [] in
> +      List.push_back_list args ["-d"; keyfile];
> +      if readonly then List.push_back args "--readonly";
> +      List.push_back_list args ["open"; device; mapname; "--type"; crypttype];

Minor nit: even if this way makes sense more from a scope POV, I'd
instead build the args list out of the protect. Mostly for ease of
reading.

> +(* Deprecated APIs for backwards compatibility.
> + *
> + * For convenient for existing callers we do not specify the
> + * crypttype parameter.  This means that callers of these APIs
> + * will be able to open BitLocker drives transparently.  (If
> + * we wanted to be strict we would pass ~crypttype:"luks"
> + * here, but that seems to have only downsides).

I disagree here: if an user uses the luks_open API, IMHO they expect it
to be LUKS; if they want to use other types, then they want to switch
to the new cryptsetup_open. So I'd pass ~crypttype:"luks" in luks_open.

> +  { defaults with
> +    name = "cryptsetup_open"; added = (1, 43, 1);
> +    style = RErr, [String (Device, "device"); String (Key, "key"); String (PlainString, "mapname")], [OBool "readonly"; OString "crypttype"];
> +    impl = OCaml "Cryptsetup.cryptsetup_open";
> +    optional = Some "luks";
> +    test_excuse = "no way to format BitLocker, and smallest device is huge";
> +    shortdesc = "open an encrypted block device";
> +    longdesc = "\
> +This command opens a block device which has been encrypted
> +according to the Linux Unified Key Setup (LUKS) standard,
> +Windows BitLocker, or some other types.
> +
> +C<device> is the encrypted block device or partition.
> +
> +The caller must supply one of the keys associated with the
> +encrypted block device, in the C<key> parameter.
> +
> +This creates a new block device called F</dev/mapper/mapname>.
> +Reads and writes to this block device are decrypted from and
> +encrypted to the underlying C<device> respectively.
> +
> +If the optional C<crypttype> parameter is not present then
> +libguestfs tries to guess the correct type (for example
> +LUKS or BitLocker).  However you can override this by
> +specifying one of the following types:
> +
> +=over 4
> +
> +=item C<luks>
> +
> +A Linux LUKS device.
> +
> +=item C<bitlk>
> +
> +A Windows BitLocker device.
> +
> +=back

Maybe add:

  Please refer to the L<cryptsetup(8)> documentation for all the
  supported types.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20200727/91da06aa/attachment.sig>