This is a straight refactor of the existing code that handles ssh/scp into a new module. In this commit I just copy the code around without doing any cleanup; cleanup will follow in subsequent commits. --- input/Makefile.am | 6 ++- input/parse_domain_from_vmx.mli | 6 --- input/ssh.mli | 34 ++++++++++++++ input/input_vmx.ml | 8 ++-- input/parse_domain_from_vmx.ml | 60 ++----------------------- input/ssh.ml | 78 +++++++++++++++++++++++++++++++++ 6 files changed, 123 insertions(+), 69 deletions(-) diff --git a/input/Makefile.am b/input/Makefile.am index de016d6076..4153f87885 100644 --- a/input/Makefile.am +++ b/input/Makefile.am @@ -39,6 +39,7 @@ SOURCES_MLI = \ parse_domain_from_vmx.mli \ parse_libvirt_xml.mli \ parse_vmx.mli \ + ssh.mli \ vCenter.mli SOURCES_ML = \ @@ -46,11 +47,12 @@ SOURCES_ML = \ parse_libvirt_xml.ml \ OVF.ml \ OVA.ml \ - parse_vmx.ml \ - parse_domain_from_vmx.ml \ nbdkit_curl.ml \ nbdkit_ssh.ml \ nbdkit_vddk.ml \ + ssh.ml \ + parse_vmx.ml \ + parse_domain_from_vmx.ml \ vCenter.ml \ input.ml \ input_disk.ml \ diff --git a/input/parse_domain_from_vmx.mli b/input/parse_domain_from_vmx.mli index e354b32e44..42f8100ee7 100644 --- a/input/parse_domain_from_vmx.mli +++ b/input/parse_domain_from_vmx.mli @@ -22,9 +22,3 @@ type vmx_source = val vmx_source_of_arg : [`SSH] option -> string -> vmx_source val parse_domain_from_vmx : vmx_source -> Types.source * string list - -(* XXX Exporting these is a hack. *) -val path_of_uri : Xml.uri -> string -val server_of_uri : Xml.uri -> string -val port_of_uri : Xml.uri -> int option -val remote_file_exists : Xml.uri -> string -> bool diff --git a/input/ssh.mli b/input/ssh.mli new file mode 100644 index 0000000000..e9a1a6a8c7 --- /dev/null +++ b/input/ssh.mli @@ -0,0 +1,34 @@ +(* virt-v2v + * Copyright (C) 2009-2024 Red Hat Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + *) + +(** Wrappers for finding and downloading remote files over ssh. *) + +val path_of_uri : Xml.uri -> string +val server_of_uri : Xml.uri -> string +val port_of_uri : Xml.uri -> int option + +(** [remote_file_exists ssh_uri path] checks that [path] exists + on the remote server [ssh_uri] (note any path inside [ssh_uri] + is ignored). *) +val remote_file_exists : Xml.uri -> string -> bool + +(** [scp_from_remote_to_temporary ssh_uri tmpdir filename] + uses scp to copy the single remote file at [ssh_uri] to + the local file called [tmpdir/filename]. It returns the + final path [tmpdir/filename]. *) +val scp_from_remote_to_temporary : Xml.uri -> string -> string -> string diff --git a/input/input_vmx.ml b/input/input_vmx.ml index eed8a43356..bd20420ca0 100644 --- a/input/input_vmx.ml +++ b/input/input_vmx.ml @@ -79,21 +79,21 @@ module VMX = struct let socket = sprintf "%s/in%d" dir i in On_exit.unlink socket; - let vmx_path = path_of_uri uri in + let vmx_path = Ssh.path_of_uri uri in let abs_path = absolute_path_from_other_file vmx_path filename in let flat_vmdk = PCRE.replace (PCRE.compile "\\.vmdk$") "-flat.vmdk" abs_path in (* RHBZ#1774386 *) - if not (remote_file_exists uri flat_vmdk) then + if not (Ssh.remote_file_exists uri flat_vmdk) then error (f_"This transport does not support guests with snapshots. \ Either collapse the snapshots for this guest and try \ the conversion again, or use one of the alternate \ conversion methods described in \ virt-v2v-input-vmware(1) section \"NOTES\"."); - let server = server_of_uri uri in - let port = Option.map string_of_int (port_of_uri uri) in + 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 diff --git a/input/parse_domain_from_vmx.ml b/input/parse_domain_from_vmx.ml index f24990f8aa..8cf5893c35 100644 --- a/input/parse_domain_from_vmx.ml +++ b/input/parse_domain_from_vmx.ml @@ -51,61 +51,6 @@ let vmx_source_of_arg input_transport arg = error (f_"vmx URI path component looks incorrect"); SSH uri -(* Return various fields from the URI. The checks in vmx_source_of_arg - * should ensure that none of these assertions fail. - *) -let port_of_uri { Xml.uri_port } = - match uri_port with i when i <= 0 -> None | i -> Some i -let server_of_uri { Xml.uri_server } = - match uri_server with None -> assert false | Some s -> s -let path_of_uri { Xml.uri_path } = - match uri_path with None -> assert false | Some p -> p - -(* 'scp' a remote file into a temporary local file, returning the path - * of the temporary local file. - *) -let scp_from_remote_to_temporary uri tmpdir filename = - let localfile = tmpdir // filename in - - let cmd = - sprintf "scp%s%s %s%s:%s %s" - (if verbose () then "" else " -q") - (match port_of_uri uri with - | None -> "" - | Some port -> sprintf " -P %d" port) - (match uri.Xml.uri_user with - | None -> "" - | Some user -> quote user ^ "@") - (quote (server_of_uri uri)) - (quote (path_of_uri uri)) - (quote localfile) in - if verbose () then - eprintf "%s\n%!" cmd; - if Sys.command cmd <> 0 then - error (f_"could not copy the VMX file from the remote server, \ - see earlier error messages"); - localfile - -(* Test if [path] exists on the remote server. *) -let remote_file_exists uri path = - let cmd = - sprintf "ssh%s %s%s test -f %s" - (match port_of_uri uri with - | None -> "" - | Some port -> sprintf " -p %d" port) - (match uri.Xml.uri_user with - | None -> "" - | Some user -> quote user ^ "@") - (quote (server_of_uri uri)) - (* Double quoting is necessary for 'ssh', first to protect - * from the local shell, second to protect from the remote - * shell. https://github.com/libguestfs/virt-v2v/issues/35#issuecomment-1741730963 - *) - (quote (quote path)) in - if verbose () then - eprintf "%s\n%!" cmd; - Sys.command cmd = 0 - let rec find_disks vmx vmx_source = (* Set the s_disk_id field to an incrementing number. *) List.mapi @@ -390,7 +335,8 @@ let parse_domain_from_vmx vmx_source = match vmx_source with | File filename -> Parse_vmx.parse_file filename | SSH uri -> - let filename = scp_from_remote_to_temporary uri tmpdir "source.vmx" in + let filename = Ssh.scp_from_remote_to_temporary uri tmpdir + "source.vmx" in Parse_vmx.parse_file filename in let name = @@ -400,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 (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 *) diff --git a/input/ssh.ml b/input/ssh.ml new file mode 100644 index 0000000000..8e12899a8e --- /dev/null +++ b/input/ssh.ml @@ -0,0 +1,78 @@ +(* virt-v2v + * Copyright (C) 2009-2020 Red Hat Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + *) + +open Std_utils +open Tools_utils +open Common_gettext.Gettext + +open Printf + +(* Return various fields from the URI. The checks in vmx_source_of_arg + * should ensure that none of these assertions fail. + *) +let port_of_uri { Xml.uri_port } = + match uri_port with i when i <= 0 -> None | i -> Some i +let server_of_uri { Xml.uri_server } = + match uri_server with None -> assert false | Some s -> s +let path_of_uri { Xml.uri_path } = + match uri_path with None -> assert false | Some p -> p + +(* 'scp' a remote file into a temporary local file, returning the path + * of the temporary local file. + *) +let scp_from_remote_to_temporary uri tmpdir filename = + let localfile = tmpdir // filename in + + let cmd = + sprintf "scp%s%s %s%s:%s %s" + (if verbose () then "" else " -q") + (match port_of_uri uri with + | None -> "" + | Some port -> sprintf " -P %d" port) + (match uri.Xml.uri_user with + | None -> "" + | Some user -> quote user ^ "@") + (quote (server_of_uri uri)) + (quote (path_of_uri uri)) + (quote localfile) in + if verbose () then + eprintf "%s\n%!" cmd; + if Sys.command cmd <> 0 then + error (f_"could not copy the VMX file from the remote server, \ + see earlier error messages"); + localfile + +(* Test if [path] exists on the remote server. *) +let remote_file_exists uri path = + let cmd = + sprintf "ssh%s %s%s test -f %s" + (match port_of_uri uri with + | None -> "" + | Some port -> sprintf " -p %d" port) + (match uri.Xml.uri_user with + | None -> "" + | Some user -> quote user ^ "@") + (quote (server_of_uri uri)) + (* Double quoting is necessary for 'ssh', first to protect + * from the local shell, second to protect from the remote + * shell. https://github.com/libguestfs/virt-v2v/issues/35#issuecomment-1741730963 + *) + (quote (quote path)) in + if verbose () then + eprintf "%s\n%!" cmd; + Sys.command cmd = 0 -- 2.43.0

On Wed, Jan 17, 2024 at 05:21:53AM +0100, Laszlo Ersek wrote: Learning something new! [...] Yes I do. I was going to also either update all the (C) dates or remove them, as you did in nbdkit: https://gitlab.com/nbdkit/nbdkit/-/commit/712fe24c18df21567e566a013d92f91... Thanks! Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

On 1/15/24 19:24, Richard W.M. Jones wrote: Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt;

On 1/15/24 19:24, Richard W.M. Jones wrote: [`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? (This is not a comment on the patch, just me trying to understand the pre-patch code.) Huh, seems like a very comfortable fit! Ah, so this "comfortable fit" is already there, we're just moving the computation. OK. (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: 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. (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. (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? 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; Laszlo

On Wed, Jan 17, 2024 at 06:23:33AM +0100, Laszlo Ersek wrote: 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. OK. 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? Sure. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

On 1/15/24 19:24, Richard W.M. Jones wrote: Complicated patch; I'll quote it out of (formatting) order: OK OK (1) so here's where I suggest that (if syntactically possible) we don't bind "password", just use "_". OK (sneaky change: port changes from integer to string; but OK) Same two comments: (2) suggest binding the password with just "_" for now, and same "sneaky" (but otherwise harmless / intended) remark about the port type change. OK (complicated, with *_of_uri being flattened in one go, but seems OK) (3) So this is the pattern where -- corresponding to my other suggestion -- we'd have to bind "password" for real, because now we are passing it to "download_file". OK (although not a fan of the code duplication) Hm. I consider this a bit of unwelcome fallout, especially with the same match on "uri.uri_path" as above (I do realize we evaluate different expressions on both "Some path" branches -- "path" versus "name_from_disk path"). (4) Can we introduce a helper function that takes a URI and returns a tuple (server, port, user, path)? Then it could be called from both "input_vmx.ml" and "parse_domain_from_vmx.ml", plus in the latter, we could use the path component of the returned tuple for both the "Ssh.download_file" call and for the "name_from_disk" call. ... In effect I'm suggesting to *replace* the *_of_uri helpers in the Ssh module with a new (unified) helper function in -- say -- Utils. If you agree with that idea, then it might be best to first perform that conversion / extraction in a separate patch, prepended to this one. Laszlo

On Wed, Jan 17, 2024 at 07:17:39AM +0100, Laszlo Ersek wrote: Since the parameter is labelled you can't replace the parameter with '_', since that means the function type changes. However (I discovered today) you can use ~password:_ which doesn't bind password inside the function: # let f ~password a = a ;; val f : password:'a -> 'b -> 'b = <fun> # let f _ a = a ;; val f : 'a -> 'b -> 'b = <fun> # let f ~password a = password ;; val f : password:'a -> 'b -> 'a = <fun> # let f ~password:_ a = password ;; Error: Unbound value password So I used ~password:_ in the updated patch. I fixed this in the updated patch, so now patch #3 uses SSH (_, uri) and patch #4 (this patch) replaces the _ with password. I'm lukewarm on this one. I think just showing what the code does is easier to read than adding a rarely used library function elsewhere. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

On 1/15/24 19:24, Richard W.M. Jones wrote: Reviewed-by: Laszlo Ersek <lersek(a)redhat.com&gt;

On Mon, Jan 15, 2024 at 06:24:16PM +0000, Richard W.M. Jones wrote: This explanation above wasn't great. In my revised version of the commit I changed this to: If you use a -i vmx ssh filename containing '*' then it will expand the glob at the remote side. New scp is weird and silently creates a directory on the local side. For example suppose there's a remote file literally called "/tmp/test*" (ie. the '*' is part of the filename) and you thought you could copy that to local using: scp 'remote:/tmp/test*' '/tmp/test*' scp treats the first parameter (only) as a wildcard and if there are multiple files matching the wildcard /tmp/test*, will create a local directory literally called "/tmp/test*/" and put the files into it. Who expected any of that? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

On Wed, Jan 17, 2024 at 09:24:59AM +0100, Laszlo Ersek wrote: I'm going to inspect the code to see if there are other possible places where we might be running ssh / scp directly first. It'll be in another patch. So my guess here is no, because ... Mkdtemp.temp_dir is a wrapper around the POSIX mkdtemp function, which creates the directory with 0700 (ignoring umask). The socket is opened by our own subprocess, nbdinfo or nbdcopy. This is different from the previous case where we ran qemu from libvirt and libvirt (when running as root) runs qemu as qemu:qemu, and so we had to take special steps with the socket. AFAIK there's no case where a subprocess wouldn't be able to immediately open the socket. (Well there is: SELinux. But that would be a policy error.) Yup, updated. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v