Libguestfs - Jun 2020 - [PATCH v2v 0/2] v2v: nbdkit: Don't use password=- parameter.

Part 2 fix for:
https://bugzilla.redhat.com/show_bug.cgi?id=1842440

Actually this fix on its own should be sufficient, but probably we
want the nbdkit fixes too.

Note this uses actual OCaml 4.05 features!  ("let open" and the
Unix.tcgetattr functions).  I checked that both features are available
on RHEL 7's OCaml.

Rich.

Just refactoring.
---
 v2v/nbdkit_sources.ml | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/v2v/nbdkit_sources.ml b/v2v/nbdkit_sources.ml
index bfda91a79..47832011d 100644
--- a/v2v/nbdkit_sources.ml
+++ b/v2v/nbdkit_sources.ml
@@ -58,7 +58,8 @@ let error_unless_nbdkit_compiled_with_selinux config       
error (f_"nbdkit was compiled without SELinux support.  You will have to
recompile nbdkit with libselinux-devel installed, or else set SELinux to
Permissive mode while doing the conversion.")
   )
 
-let common_create ?bandwidth ?extra_debug ?extra_env plugin_name plugin_args
+let common_create ?bandwidth ?extra_debug ?extra_env password
+      plugin_name plugin_args    error_unless_nbdkit_working ();
   let config = Nbdkit.config () in
   error_unless_nbdkit_min_version config;
@@ -136,6 +137,15 @@ let common_create ?bandwidth ?extra_debug ?extra_env
plugin_name plugin_args      List.fold_left (fun cmd (k, v) -> Nbdkit.add_arg
cmd k v)
       cmd (plugin_args @ rate_args) in
 
+  (* Handle the password parameter specially. *)
+  let cmd +    match password with
+    | NoPassword -> cmd
+    | AskForPassword ->
+       Nbdkit.add_arg cmd "password" "-"
+    | PasswordFile password_file ->
+       Nbdkit.add_arg cmd "password" ("+" ^ password_file)
in
+
   cmd
 
 (* VDDK libraries are located under lib32/ or lib64/ relative to the
@@ -223,20 +233,16 @@ See also the virt-v2v-input-vmware(1) manual.") libNN
     let get_args () = List.rev !args in
     add_arg, get_args in
 
-  let password_param -    match password_file with
-    | None ->
-       (* nbdkit asks for the password interactively *)
-       "password", "-"
-    | Some password_file ->
-       (* nbdkit reads the password from the file *)
-       "password", "+" ^ password_file in
   add_arg ("server", server);
   add_arg ("user", user);
-  add_arg password_param;
   add_arg ("vm", sprintf "moref=%s" moref);
   add_arg ("file", path);
 
+  let password +    match password_file with
+    | None -> AskForPassword
+    | Some password_file -> PasswordFile password_file in
+
   (* The passthrough parameters. *)
   Option.may (fun s -> add_arg ("config", s)) config;
   Option.may (fun s -> add_arg ("cookie", s)) cookie;
@@ -251,7 +257,7 @@ See also the virt-v2v-input-vmware(1) manual.") libNN
   let debug_flag      if version >= (1, 17, 10) then Some
("vddk.datapath", "0") else None in
 
-  common_create ?bandwidth ?extra_debug:debug_flag ?extra_env:env
+  common_create ?bandwidth ?extra_debug:debug_flag ?extra_env:env password
     "vddk" (get_args ())
 
 (* Create an nbdkit module specialized for reading from SSH sources. *)
@@ -267,14 +273,9 @@ let create_ssh ?bandwidth ~password ?port ~server ?user
path    add_arg ("host", server);
   Option.may (fun s -> add_arg ("port", s)) port;
   Option.may (fun s -> add_arg ("user", s)) user;
-  (match password with
-   | NoPassword -> ()
-   | AskForPassword -> add_arg ("password", "-")
-   | PasswordFile password_file -> add_arg ("password",
"+" ^ password_file)
-  );
   add_arg ("path", path);
 
-  common_create ?bandwidth "ssh" (get_args ())
+  common_create ?bandwidth password "ssh" (get_args ())
 
 (* Create an nbdkit module specialized for reading from Curl sources. *)
 let create_curl ?bandwidth ?cookie ~password ?(sslverify=true) ?user url @@
-287,18 +288,13 @@ let create_curl ?bandwidth ?cookie ~password
?(sslverify=true) ?user url      add_arg, get_args in
 
   Option.may (fun s -> add_arg ("user", s)) user;
-  (match password with
-   | NoPassword -> ()
-   | AskForPassword -> add_arg ("password", "-")
-   | PasswordFile password_file -> add_arg ("password",
"+" ^ password_file)
-  );
   (* https://bugzilla.redhat.com/show_bug.cgi?id=1146007#c10 *)
   add_arg ("timeout", "2000");
   Option.may (fun s -> add_arg ("cookie", s)) cookie;
   if not sslverify then add_arg ("sslverify", "false");
   add_arg ("url", url);
 
-  common_create ?bandwidth "curl" (get_args ())
+  common_create ?bandwidth password "curl" (get_args ())
 
 let run cmd    let sock, _ = Nbdkit.run_unix cmd in
-- 
2.25.0

This was broken with all nbdkit plugins, some in more ways than others.

Because we start nbdkit in the background and wait 30 seconds for it
to start running, the user had only 30 seconds to type in a password
before we timed out the process.  In addition with the VDDK plugin
password=- had been broken ever since we changed the plugin to use a
reexec
(https://www.redhat.com/archives/libguestfs/2020-June/msg00012.html).

The solution is to read the password ourselves and pass it to nbdkit
as a private file.
---
 v2v/nbdkit_sources.ml | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/v2v/nbdkit_sources.ml b/v2v/nbdkit_sources.ml
index 47832011d..f5e919116 100644
--- a/v2v/nbdkit_sources.ml
+++ b/v2v/nbdkit_sources.ml
@@ -142,7 +142,26 @@ let common_create ?bandwidth ?extra_debug ?extra_env
password
     match password with
     | NoPassword -> cmd
     | AskForPassword ->
-       Nbdkit.add_arg cmd "password" "-"
+       (* Because we will start nbdkit in the background and then wait
+        * for 30 seconds for it to start up, we cannot use the
+        * password=- feature of nbdkit to read the password
+        * interactively (since in the words of the movie the user has
+        * only "30 seconds to comply").  In any case this feature
broke
+        * in the VDDK plugin in nbdkit 1.18 and 1.20.  So in the
+        * AskForPassword case we read the password here.
+        *)
+       printf "password: ";
+       let open Unix in
+       let orig = tcgetattr stdin in
+       let tios = { orig with c_echo = false } in
+       tcsetattr stdin TCSAFLUSH tios; (* Disable echo. *)
+       let password = read_line () in
+       tcsetattr stdin TCSAFLUSH orig; (* Restore echo. *)
+       printf "\n";
+       let password_file = Filename.temp_file "v2vnbdkit"
".txt" in
+       unlink_on_exit password_file;
+       with_open_out password_file (fun chan -> output_string chan
password);
+       Nbdkit.add_arg cmd "password" ("+" ^ password_file)
     | PasswordFile password_file ->
        Nbdkit.add_arg cmd "password" ("+" ^ password_file)
in
 
-- 
2.25.0

On Monday, 1 June 2020 18:43:36 CEST Richard W.M. Jones
wrote:
> Part 2 fix for:
> https://bugzilla.redhat.com/show_bug.cgi?id=1842440
> 
> Actually this fix on its own should be sufficient, but probably we
> want the nbdkit fixes too.
> 
> Note this uses actual OCaml 4.05 features!  ("let open" and the
> Unix.tcgetattr functions).
BTW Unix.tcgetattr is very old, even old 3.11 had it (found in some old
tests of mine).

LGTM; IMHO printing user/host in the password prompt would make it more
clear as to for which host the password is asked.

-- 
Pino Toscano