On Monday, 7 September 2020 11:43:54 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.
+ (* 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