thr3ads.net - Libguestfs - [Libguestfs] [v2v PATCH] nbdkit, qemuNBD: run_unix: formally require externally provided socket [Mar 2022]

If this information is useful, please help other people find it:
Share via:

Laszlo Ersek

2022-Mar-23 10:43 UTC

[Libguestfs] [v2v PATCH] nbdkit, qemuNBD: run_unix: formally require externally provided socket

At this point, virt-v2v never relies on the Unix domain sockets created
inside the "run_unix" implementations. Simplify the code by removing
this
option.

Consequently, the internally created temporary directory only holds the
NBD server's PID file, and never its UNIX domain socket. Therefore:

(1) we no longer need the libguestfs socket dir to be our temp dir,

(2) we need not change the file mode bits on the temp dir,

(3) we can rename "tmpdir" to the more specific "piddir".

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2066773
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    This patch depends on
    
      [PATCH v2v v3]
      lib: Improve security of in/out sockets when running virt-v2v as root
    
      https://listman.redhat.com/archives/libguestfs/2022-March/028460.html
    
    with or without the "chmod socket 0o700" suggested in
   
<https://listman.redhat.com/archives/libguestfs/2022-March/028463.html>.

 lib/nbdkit.mli              |  6 +----
 lib/qemuNBD.mli             |  6 +----
 input/input_disk.ml         |  4 ++--
 input/input_libvirt.ml      |  8 +++----
 input/input_ova.ml          |  2 +-
 input/input_vddk.ml         |  2 +-
 input/input_vmx.ml          |  4 ++--
 input/input_xen_ssh.ml      |  2 +-
 input/vCenter.ml            |  2 +-
 lib/nbdkit.ml               | 24 ++++---------------
 lib/qemuNBD.ml              | 25 ++++----------------
 output/output.ml            |  4 ++--
 output/output_null.ml       |  2 +-
 output/output_rhv_upload.ml |  2 +-
 14 files changed, 28 insertions(+), 65 deletions(-)

diff --git a/lib/nbdkit.mli b/lib/nbdkit.mli
index dc2fd04b2984..5ba83ab04540 100644
--- a/lib/nbdkit.mli
+++ b/lib/nbdkit.mli
@@ -92,14 +92,10 @@ val add_args : cmd -> (string * string) list -> unit
 val add_env : cmd -> string -> string -> unit
 (** Add name=value environment variable. *)
 
-val run_unix : ?socket:string -> cmd -> string * int
+val run_unix : string -> cmd -> string * int
 (** Start nbdkit command listening on a Unix domain socket, waiting
     for the process to start up.
 
-    If optional [?socket] parameter is omitted, then a temporary
-    Unix domain socket name is created.  If [?socket] is present
-    then this overrides the temporary name.
-
     Returns the Unix domain socket name and the nbdkit process ID.
 
     The --exit-with-parent, --foreground, --pidfile, --newstyle and
diff --git a/lib/qemuNBD.mli b/lib/qemuNBD.mli
index 83871c5bfb9e..e10d3106c7e7 100644
--- a/lib/qemuNBD.mli
+++ b/lib/qemuNBD.mli
@@ -43,12 +43,8 @@ val set_snapshot : cmd -> bool -> unit
 val set_format : cmd -> string option -> unit
 (** Set the format [--format] parameter. *)
 
-val run_unix : ?socket:string -> cmd -> string * int
+val run_unix : string -> cmd -> string * int
 (** Start qemu-nbd command listening on a Unix domain socket,
     waiting for the process to start up.
 
-    If optional [?socket] parameter is omitted, then a temporary
-    Unix domain socket name is created.  If [?socket] is present
-    then this overrides the temporary name.
-
     Returns the Unix domain socket name and the qemu-nbd process ID. *)
diff --git a/input/input_disk.ml b/input/input_disk.ml
index bdb8a5103b93..508adf9dd634 100644
--- a/input/input_disk.ml
+++ b/input/input_disk.ml
@@ -110,7 +110,7 @@ module Disk = struct
            Nbdkit.add_arg cmd "file" disk;
            if Nbdkit.version nbdkit_config >= (1, 22, 0) then
              Nbdkit.add_arg cmd "cache" "none";
-           let _, pid = Nbdkit.run_unix ~socket cmd in
+           let _, pid = Nbdkit.run_unix socket cmd in
 
            (* --exit-with-parent should ensure nbdkit is cleaned
             * up when we exit, but it's not supported everywhere.
@@ -121,7 +121,7 @@ module Disk = struct
            let cmd = QemuNBD.create disk in
            QemuNBD.set_snapshot cmd options.read_only;
            QemuNBD.set_format cmd (Some format);
-           let _, pid = QemuNBD.run_unix ~socket cmd in
+           let _, pid = QemuNBD.run_unix socket cmd in
            On_exit.kill pid
     ) args;
 
diff --git a/input/input_libvirt.ml b/input/input_libvirt.ml
index 2458d1b0eee7..9311e89a2cba 100644
--- a/input/input_libvirt.ml
+++ b/input/input_libvirt.ml
@@ -88,7 +88,7 @@ and setup_servers options dir disks           Nbdkit.add_arg
cmd "hostname" hostname;
          Nbdkit.add_arg cmd "port" (string_of_int port);
          Nbdkit.add_arg cmd "shared" "true";
-         let _, pid = Nbdkit.run_unix ~socket cmd in
+         let _, pid = Nbdkit.run_unix socket cmd in
 
          (* --exit-with-parent should ensure nbdkit is cleaned
           * up when we exit, but it's not supported everywhere.
@@ -102,7 +102,7 @@ and setup_servers options dir disks  
          let cor = dir // "convert" in
          let cmd = Nbdkit_curl.create_curl ~cor url in
-         let _, pid = Nbdkit.run_unix ~socket cmd in
+         let _, pid = Nbdkit.run_unix socket cmd in
 
          (* --exit-with-parent should ensure nbdkit is cleaned
           * up when we exit, but it's not supported everywhere.
@@ -118,7 +118,7 @@ and setup_servers options dir disks             
Nbdkit.add_arg cmd "file" filename;
             if Nbdkit.version nbdkit_config >= (1, 22, 0) then
               Nbdkit.add_arg cmd "cache" "none";
-            let _, pid = Nbdkit.run_unix ~socket cmd in
+            let _, pid = Nbdkit.run_unix socket cmd in
 
             (* --exit-with-parent should ensure nbdkit is cleaned
              * up when we exit, but it's not supported everywhere.
@@ -130,7 +130,7 @@ and setup_servers options dir disks              let cmd =
QemuNBD.create filename in
             QemuNBD.set_snapshot cmd options.read_only;
             QemuNBD.set_format cmd format;
-            let _, pid = QemuNBD.run_unix ~socket cmd in
+            let _, pid = QemuNBD.run_unix socket cmd in
             On_exit.kill pid
   ) disks
 
diff --git a/input/input_ova.ml b/input/input_ova.ml
index 1b0e136723c3..8ec1f8026b19 100644
--- a/input/input_ova.ml
+++ b/input/input_ova.ml
@@ -192,7 +192,7 @@ module OVA = struct
         let cmd = QemuNBD.create qemu_uri in
         QemuNBD.set_snapshot cmd options.read_only; (* protective overlay *)
         QemuNBD.set_format cmd None; (* auto-detect format *)
-        let _, pid = QemuNBD.run_unix ~socket cmd in
+        let _, pid = QemuNBD.run_unix socket cmd in
         On_exit.kill pid
       ) qemu_uris;
 
diff --git a/input/input_vddk.ml b/input/input_vddk.ml
index 23cfd529a2be..e48495d36d34 100644
--- a/input/input_vddk.ml
+++ b/input/input_vddk.ml
@@ -199,7 +199,7 @@ information on these settings.
                ?nfchostport ?password_file:options.input_password ?port
                ~server ?snapshot ~thumbprint ?transports ?user
                path in
-           let _, pid = Nbdkit.run_unix ~socket nbdkit in
+           let _, pid = Nbdkit.run_unix socket nbdkit in
            On_exit.kill pid
     ) disks;
 
diff --git a/input/input_vmx.ml b/input/input_vmx.ml
index 8ceb0bb0d175..9921419b5a85 100644
--- a/input/input_vmx.ml
+++ b/input/input_vmx.ml
@@ -69,7 +69,7 @@ module VMX = struct
                         (absolute_path_from_other_file vmx_filename filename)
in
             QemuNBD.set_snapshot cmd true; (* protective overlay *)
             QemuNBD.set_format cmd (Some "vmdk");
-            let _, pid = QemuNBD.run_unix ~socket cmd in
+            let _, pid = QemuNBD.run_unix socket cmd in
             On_exit.kill pid
         ) filenames
 
@@ -111,7 +111,7 @@ module VMX = struct
             let bandwidth = options.bandwidth in
             let nbdkit = Nbdkit_ssh.create_ssh ?bandwidth ~cor ~password
                            ~server ?port ?user abs_path in
-            let _, pid = Nbdkit.run_unix ~socket nbdkit in
+            let _, pid = Nbdkit.run_unix socket nbdkit in
             On_exit.kill pid
         ) filenames
     );
diff --git a/input/input_xen_ssh.ml b/input/input_xen_ssh.ml
index b154a1a5b6bd..0aad36a8befc 100644
--- a/input/input_xen_ssh.ml
+++ b/input/input_xen_ssh.ml
@@ -121,7 +121,7 @@ module XenSSH = struct
            let bandwidth = options.bandwidth in
            let nbdkit = Nbdkit_ssh.create_ssh ?bandwidth ~cor ~password
                           ?port ~server ?user path in
-           let _, pid = Nbdkit.run_unix ~socket nbdkit in
+           let _, pid = Nbdkit.run_unix socket nbdkit in
            On_exit.kill pid
     ) disks;
 
diff --git a/input/vCenter.ml b/input/vCenter.ml
index 40d594f0d6ea..8a1a5655ec8a 100644
--- a/input/vCenter.ml
+++ b/input/vCenter.ml
@@ -117,7 +117,7 @@ let rec start_nbdkit_for_path ?bandwidth ?cor ?password_file
     Nbdkit_curl.create_curl ?bandwidth ?cor
       ~cookie_script ~cookie_script_renew
       ~sslverify https_url in
-  let _, pid = Nbdkit.run_unix ~socket nbdkit in
+  let _, pid = Nbdkit.run_unix socket nbdkit in
   pid
 
 and get_https_url dcPath uri server path diff --git a/lib/nbdkit.ml
b/lib/nbdkit.ml
index 9ee6f39ce335..07896684e861 100644
--- a/lib/nbdkit.ml
+++ b/lib/nbdkit.ml
@@ -102,27 +102,13 @@ let add_env cmd name value = cmd.env <- (name, value)
:: cmd.env
 let add_filter_if_available cmd filter    if probe_filter filter then
add_filter cmd filter
 
-let run_unix ?socket cmd -  (* Create a temporary directory where we place the
socket and PID file.
-   * Use the libguestfs socket directory, so it is more likely the full path
-   * of the UNIX sockets will fit in the (limited) socket pathname.
-   *)
-  let tmpdir -    let base_dir = (open_guestfs ())#get_sockdir () in
-    let t = Mkdtemp.temp_dir ~base_dir "v2vnbdkit." in
-    (* tmpdir must be readable (but not writable) by "other" so that
-     * qemu can open the sockets.
-     *)
-    chmod t 0o755;
-    On_exit.rmdir t;
-    t in
+let run_unix socket cmd +  (* Create a temporary directory where we place the
PID file. *)
+  let piddir = Mkdtemp.temp_dir "v2vnbdkit." in
+  On_exit.rmdir piddir;
 
   let id = unique () in
-  let pidfile = tmpdir // sprintf "nbdkit%d.pid" id in
-  let socket -    match socket with
-    | None -> tmpdir // sprintf "nbdkit%d.sock" id
-    | Some socket -> socket in
+  let pidfile = piddir // sprintf "nbdkit%d.pid" id in
 
   (* Construct the final command line. *)
   let add_arg, add_args_reversed, get_args diff --git a/lib/qemuNBD.ml
b/lib/qemuNBD.ml
index 2c999b9f8f8b..ae21b17c7de2 100644
--- a/lib/qemuNBD.ml
+++ b/lib/qemuNBD.ml
@@ -62,30 +62,15 @@ let create disk = { disk; snapshot = false; format = None }
 let set_snapshot cmd snap = cmd.snapshot <- snap
 let set_format cmd format = cmd.format <- format
 
-let run_unix ?socket { disk; snapshot; format } +let run_unix socket { disk;
snapshot; format }    assert (disk <> "");
 
-  (* Create a temporary directory where we place the socket and PID file.
-   * Use the libguestfs socket directory, so it is more likely the full path
-   * of the UNIX sockets will fit in the (limited) socket pathname.
-   *)
-  let tmpdir -    let base_dir = (open_guestfs ())#get_sockdir () in
-    let t = Mkdtemp.temp_dir ~base_dir "v2vqemunbd." in
-    (* tmpdir must be readable (but not writable) by "other" so that
-     * qemu can open the sockets.
-     *)
-    chmod t 0o755;
-    On_exit.rmdir t;
-    t in
+  (* Create a temporary directory where we place the PID file. *)
+  let piddir = Mkdtemp.temp_dir "v2vqemunbd." in
+  On_exit.rmdir piddir;
 
   let id = unique () in
-  let pidfile = tmpdir // sprintf "qemunbd%d.pid" id in
-
-  let socket -    match socket with
-    | Some socket -> socket
-    | None -> tmpdir // sprintf "qemunbd%d.sock" id in
+  let pidfile = piddir // sprintf "qemunbd%d.pid" id in
 
   (* Construct the qemu-nbd command line. *)
   let args = ref [] in
diff --git a/output/output.ml b/output/output.ml
index 7256b547e080..10e685c46926 100644
--- a/output/output.ml
+++ b/output/output.ml
@@ -90,7 +90,7 @@ let output_to_local_file ?(changeuid = fun f -> f ())
        let cmd = Nbdkit.add_arg cmd "cache" "none" in
        cmd
      );
-     let _, pid = Nbdkit.run_unix ~socket cmd in
+     let _, pid = Nbdkit.run_unix socket cmd in
 
      (* --exit-with-parent should ensure nbdkit is cleaned
       * up when we exit, but it's not supported everywhere.
@@ -101,7 +101,7 @@ let output_to_local_file ?(changeuid = fun f -> f ())
      let cmd = QemuNBD.create filename in
      QemuNBD.set_snapshot cmd false;
      QemuNBD.set_format cmd (Some "qcow2");
-     let _, pid = QemuNBD.run_unix ~socket cmd in
+     let _, pid = QemuNBD.run_unix socket cmd in
      On_exit.kill pid
 
   | _ ->
diff --git a/output/output_null.ml b/output/output_null.ml
index 86d81eaa7b64..c8e27c0b0aaf 100644
--- a/output/output_null.ml
+++ b/output/output_null.ml
@@ -70,7 +70,7 @@ module Null = struct
     let ()        let cmd = Nbdkit.create ~quiet:true "null" in
       Nbdkit.add_arg cmd "size" "7E";
-      let _, pid = Nbdkit.run_unix ~socket cmd in
+      let _, pid = Nbdkit.run_unix socket cmd in
 
       (* --exit-with-parent should ensure nbdkit is cleaned
        * up when we exit, but it's not supported everywhere.
diff --git a/output/output_rhv_upload.ml b/output/output_rhv_upload.ml
index 72463e572f6d..828996b36261 100644
--- a/output/output_rhv_upload.ml
+++ b/output/output_rhv_upload.ml
@@ -398,7 +398,7 @@ e command line has to match the number of guest disk images
(for this guest: %d)
           Nbdkit.add_arg cmd "insecure" "true";
         if is_ovirt_host then
           Nbdkit.add_arg cmd "is_ovirt_host" "true";
-        let _, pid = Nbdkit.run_unix ~socket cmd in
+        let _, pid = Nbdkit.run_unix socket cmd in
         List.push_front pid nbdkit_pids
     ) (List.combine disks disk_uuids);
 
-- 
2.19.1.3.g30247aa5d201

Richard W.M. Jones

2022-Mar-23 10:52 UTC

head link

[Libguestfs] [v2v PATCH] nbdkit, qemuNBD: run_unix: formally require externally provided socket

On Wed, Mar 23, 2022 at 11:43:30AM +0100, Laszlo Ersek
wrote:> At this point, virt-v2v never relies on the Unix domain sockets created
> inside the "run_unix" implementations. Simplify the code by
removing this
> option.
> 
> Consequently, the internally created temporary directory only holds the
> NBD server's PID file, and never its UNIX domain socket. Therefore:
> 
> (1) we no longer need the libguestfs socket dir to be our temp dir,
> 
> (2) we need not change the file mode bits on the temp dir,
> 
> (3) we can rename "tmpdir" to the more specific
"piddir".
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2066773
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
ACK.

I pushed my patch -- note it has changes related to the "lazy"
clause relative to what I posted in v3.

I also tested with your patch on top, doing both non-root and root
conversions, and it all seems to work.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Libguestfs - Mar 2022 - [v2v PATCH] nbdkit, qemuNBD: run_unix: formally require externally provided socket

[Libguestfs] [v2v PATCH] nbdkit, qemuNBD: run_unix: formally require externally provided socket

[Libguestfs] [v2v PATCH] nbdkit, qemuNBD: run_unix: formally require externally provided socket