[Libguestfs] Re: [PATCH virt-v2v 3/6] virt-v2v: -i vmx: Add the input password to vmx_source

On Wed, Jan 17, 2024 at 06:23:33AM +0100, Laszlo Ersek wrote: > On 1/15/24 19:24, Richard W.M. Jones wrote: >> Since we use the input password in various places in the VMX module, >> store the input password in vmx_source. This neutral refactoring >> makes later changes simpler. >> --- >> input/parse_domain_from_vmx.mli | 7 ++++--- >> input/input_vmx.ml | 13 ++++++------- >> input/parse_domain_from_vmx.ml | 12 ++++++------ >> 3 files changed, 16 insertions(+), 16 deletions(-) >> >> diff --git a/input/parse_domain_from_vmx.mli b/input/parse_domain_from_vmx.mli >> index 42f8100ee7..208797a736 100644 >> --- a/input/parse_domain_from_vmx.mli >> +++ b/input/parse_domain_from_vmx.mli >> @@ -17,8 +17,9 @@ >> *) >> >> type vmx_source = >> - | File of string (** local file or NFS *) >> - | SSH of Xml.uri (** SSH URI *) >> + | File of string (** local file or NFS *) >> + | SSH of Nbdkit_ssh.password * Xml.uri (** SSH URI *) >> >> -val vmx_source_of_arg : [`SSH] option -> string -> vmx_source > > [`SSH] looks unusual. According to > <https://dev.realworldocaml.org/variants.html>;, it is an "exact > polymorphic variant type". > > If I understand correctly, originally we just wanted an empty data > constructor (taking no params) to plug into "option", but without > defining an explicit variant type. > > An alternative to "[`SSH] option" could have been: > > - first step: just define a normal variant type, with a single data > constructor taking no parameter. This would effectively amount to an > enumeration type, with a single (and flat) enumeration constant: > > type ssh_type = Ssh > val vmx_source_of_arg : ssh_type option -> string -> vmx_source > > - second step: realizing that the value set {None, Some Ssh} is not > super useful, we could have gone with > > type ssh_type = Ssh | Nothing > val vmx_source_of_arg : ssh_type -> string -> vmx_source > > effectively squashing "option" into "ssh_type" (the same two-element > value set would be represented directly in the enumeration type). > > Is that right? No I don't think that's right. The actual type of options.input_transport is: input_transport : [`SSH|`VDDK] option; (defined in input/input.mli). What we're saying here is that you can only call vmx_source_of_arg with specifically the subset of this type which is Some `SSH (or None). Some `SSH represents SSH connections (-it ssh), and None (no -it option) represents local files. The caller of vmx_source_of_arg ensures this using: let input_transport = match options.input_transport with | None -> None | Some `SSH -> Some `SSH | Some `VDDK -> error (f_"-i vmx: cannot use -it vddk in this input mode") in vmx_source_of_arg input_password input_transport arg (input/input_vmx.ml) and the type checker verifies this is correct at compile time.

> (This is not a comment on the patch, just me trying to understand the > pre-patch code.) > >> +val vmx_source_of_arg : Nbdkit_ssh.password -> [`SSH] option -> string -> >> + vmx_source >> val parse_domain_from_vmx : vmx_source -> Types.source * string list >> diff --git a/input/input_vmx.ml b/input/input_vmx.ml >> index bd20420ca0..b9cce10fed 100644 >> --- a/input/input_vmx.ml >> +++ b/input/input_vmx.ml >> @@ -45,13 +45,17 @@ module VMX = struct >> let vmx_source = >> match args with >> | [arg] -> >> + let input_password = >> + match options.input_password with >> + | None -> Nbdkit_ssh.NoPassword >> + | Some ip -> Nbdkit_ssh.PasswordFile ip in > > Huh, seems like a very comfortable fit! > >> let input_transport = >> match options.input_transport with >> | None -> None >> | Some `SSH -> Some `SSH >> | Some `VDDK -> >> error (f_"-i vmx: cannot use -it vddk in this input mode") in >> - vmx_source_of_arg input_transport arg >> + vmx_source_of_arg input_password input_transport arg >> | _ -> >> error (f_"-i vmx: expecting a VMX file or ssh:// URI") in >> >> @@ -73,7 +77,7 @@ module VMX = struct >> On_exit.kill pid >> ) filenames >> >> - | SSH uri -> >> + | SSH (password, uri) -> >> List.iteri ( >> fun i filename -> >> let socket = sprintf "%s/in%d" dir i in >> @@ -95,11 +99,6 @@ module VMX = struct >> let server = Ssh.server_of_uri uri in >> let port = Option.map string_of_int (Ssh.port_of_uri uri) in >> let user = uri.Xml.uri_user in >> - let password = >> - match options.input_password with >> - | None -> Nbdkit_ssh.NoPassword >> - | Some ip -> Nbdkit_ssh.PasswordFile ip in >> - >> let cor = dir // "convert" in >> let bandwidth = options.bandwidth in >> let nbdkit = Nbdkit_ssh.create_ssh ?bandwidth ~cor ~password > > Ah, so this "comfortable fit" is already there, we're just moving the > computation. OK. > >> diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml >> index e6500da64a..94ae9957e3 100644 >> --- a/input/parse_domain_from_vmx.ml >> +++ b/input/parse_domain_from_vmx.ml >> @@ -29,13 +29,13 @@ open Utils >> open Name_from_disk >> >> type vmx_source = >> - | File of string (* local file or NFS *) >> - | SSH of Xml.uri (* SSH URI *) >> + | File of string (* local file or NFS *) >> + | SSH of Nbdkit_ssh.password * Xml.uri (* SSH URI *) >> >> (* The single filename on the command line is intepreted either as >> * a local file or a remote SSH URI (only if ‘-it ssh’). >> *) >> -let vmx_source_of_arg input_transport arg = >> +let vmx_source_of_arg input_password input_transport arg = >> match input_transport, arg with >> | None, arg -> File arg >> | Some `SSH, arg -> >> @@ -49,7 +49,7 @@ let vmx_source_of_arg input_transport arg = >> error (f_"vmx URI remote server name omitted"); >> if uri.Xml.uri_path = None || uri.Xml.uri_path = Some "/" then >> error (f_"vmx URI path component looks incorrect"); >> - SSH uri >> + SSH (input_password, uri) >> >> let rec find_disks vmx vmx_source = >> (* Set the s_disk_id field to an incrementing number. *) >> @@ -334,7 +334,7 @@ let parse_domain_from_vmx vmx_source = >> let vmx = >> match vmx_source with >> | File filename -> Parse_vmx.parse_file filename >> - | SSH uri -> >> + | SSH (password, uri) -> >> let filename = tmpdir // "source.vmx" in >> Ssh.download_file uri filename; >> Parse_vmx.parse_file filename in > > (1) Seems like binding the first component of the tuple with "password" > is unnecessary here -- I think we could use the wildcard "_", just like > below: > >> @@ -346,7 +346,7 @@ let parse_domain_from_vmx vmx_source = >> warning (f_"no displayName key found in VMX file"); >> match vmx_source with >> | File filename -> name_from_disk filename >> - | SSH uri -> name_from_disk (Ssh.path_of_uri uri) in >> + | SSH (_, uri) -> name_from_disk (Ssh.path_of_uri uri) in >> >> let genid = >> (* See: https://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg02019.html *) > > Now, I figure that in one of the next patches, the part marked with (1) > will actually grow a use for the password component of the tuple; after > all, "Ssh.download_file" will still need to authenticate. > > ... Right, that seems to be happening in the next patch (#4). > > For slightly improving clarity, do you think you could use the wildcard > "_" at (1) in *this* patch, and move the *real* binding to the next > patch? Because right in this patch, the binding is still unneeded. OK. > (2) A further question: now I wanted to recheck how "Ssh.download_file > uri" actually uses the password file passed in with "-ip", and the > answer, as far as I can tell, is that it doesn't! > > The manual <https://libguestfs.org/virt-v2v-input-vmware.1.html> says, > > Note that support for non-interactive authentication via the -ip > option is incomplete. Some operations remain that still require the > user to enter the password manually. Therefore ssh-agent is > recommended over the -ip option. See > https://bugzilla.redhat.com/1854275. > > which certainly brings back some unpleasant :) memories -- and the BZ is > indeed called 'document that vmx+ssh "-ip" auth doesn't cover ssh / scp > shell commands'. > > OK, so long story short: is it the case that this patch series actually > *extends* "-ip" support to "-i vmx -it ssh"? Because in that case, I > think we should update the documentation as well. This does fix the -ip option (in a later patch). However I'm not sure if it fixes it everywhere. I guess so ... but I'm not too confident! Maybe we should leave the advice there?

> (3) Having written the above, I realize that in patch #4, we don't > actually *consume* the password (file) just yet; we only push it down > one level deeper. The actual consumption only happens in patch#6, where > we finally forward ~password to "start_nbdkit". > > Because of that, I'm going to suggest one more "tightening" here (not > sure if it makes sense; my OCaml is certainly rusty): in patch#4, can we > *implement* (not declare) the "download_file" function such that it > still take (bind) the password with the _ wildcard? Sure. > Quick check: > > # let func _ a b = a + b;; > val func : 'a -> int -> int -> int = <fun> > > # func "hello" 1 2;; > - : int = 3 > > so at least the syntax should exist for this. > > And then bind the password in the "download_file" function for real only > in patch#6, where it gets put to use -- i.e., where it gets passed to > "start_nbdkit". > > "Unused variables / parameters" don't hurt, but at least in OCaml, we > can keep them nameless! :) > > Summary: > > - I think at (1) we can / should use "_" > > - I suggest adding a patch#7 that removes the reference to RHBZ 1854275 > from the docs, because (I think?) we're ending up fixing that BZ now > > - I'll make comment (3) under patch#4 too. > > Only the first item affects this particular patch, and I don't insist on > even that; just consider it. However you decide: > > Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt; Thanks, Rich.