Libguestfs - Jun 2020 - Re: [PATCH nbdkit] vddk: Disallow password=-

This has been broken since we added the reexec code
(commit 155af3107292c351d54ed42c732f4a67bb9aa910) because it
tried to read the password twice (before and after the reexec) failing
the second time because stdin had already been reopened on /dev/null.

Virt-v2v used this feature, but I will change virt-v2v instead.
---
 plugins/vddk/nbdkit-vddk-plugin.pod | 7 +------
 plugins/vddk/vddk.c                 | 4 ++++
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod
b/plugins/vddk/nbdkit-vddk-plugin.pod
index 73316dd1..96a665b6 100644
--- a/plugins/vddk/nbdkit-vddk-plugin.pod
+++ b/plugins/vddk/nbdkit-vddk-plugin.pod
@@ -7,8 +7,7 @@ nbdkit-vddk-plugin - nbdkit VMware VDDK plugin
  nbdkit vddk [file=]FILENAME
              [config=FILENAME] [cookie=COOKIE] [libdir=LIBRARY]
              [nfchostport=PORT] [single-link=true]
-             [password=PASSWORD | password=- | password=+FILENAME
-             | password=-FD]
+             [password=PASSWORD | password=+FILENAME | password=-FD]
              [port=PORT] [server=HOSTNAME] [snapshot=MOREF]
              [thumbprint=THUMBPRINT] [transports=MODE:MODE:...]
              [unbuffered=true] [user=USERNAME] [vm=moref=ID]
@@ -159,10 +158,6 @@ Set the password to use when connecting to the remote
server.
 Note that passing this on the command line is not secure on shared
 machines.
 
-=item B<password=->
-
-Ask for the password (interactively) when nbdkit starts up.
-
 =item B<password=+>FILENAME
 
 Read the password from the named file.  This is a secure method
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 4815a43e..54a6e019 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -240,6 +240,10 @@ vddk_config (const char *key, const char *value)
       return -1;
   }
   else if (strcmp (key, "password") == 0) {
+    if (strcmp (value, "-") == 0) {
+      nbdkit_error ("password=- is not supported with the VDDK
plugin");
+      return -1;
+    }
     free (password);
     if (nbdkit_read_password (value, &password) == -1)
       return -1;
-- 
2.25.0

On 6/1/20 11:07 AM, Richard W.M. Jones wrote:
> This has been broken since we added the reexec code
> (commit 155af3107292c351d54ed42c732f4a67bb9aa910) because it
> tried to read the password twice (before and after the reexec) failing
> the second time because stdin had already been reopened on /dev/null.
> 
> Virt-v2v used this feature, but I will change virt-v2v instead.
> ---
>   plugins/vddk/nbdkit-vddk-plugin.pod | 7 +------
>   plugins/vddk/vddk.c                 | 4 ++++
>   2 files changed, 5 insertions(+), 6 deletions(-)
This is the simplest "fix", by avoiding the problem. It may still be 
possible to come up with something more robust, where (since we are 
already special-casing for "-" as the password value) we read the 
password prior to re-exec, and rewrite the command line of the re-exec 
to instead consume a temporary file.  But that's more complex, and 
doesn't stop us from pushing this now; if we ever do implement the more 
complex patch, we can roll back the documentation change.

And I already see that you have the counterpart v2v patch that hoists 
the reading of the password into v2v instead of worrying about vddk; the 
v2v hack of using a temporary file "works" in spite of our re-exec 
reading it twice, even if we want to eventually reach the point where a 
more complex patch in the vddk plugin would only read the password once 
before re-exec.

ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Jun 01, 2020 at 12:13:13PM -0500, Eric Blake
wrote:
> On 6/1/20 11:07 AM, Richard W.M. Jones wrote:
> >This has been broken since we added the reexec code
> >(commit 155af3107292c351d54ed42c732f4a67bb9aa910) because it
> >tried to read the password twice (before and after the reexec) failing
> >the second time because stdin had already been reopened on /dev/null.
> >
> >Virt-v2v used this feature, but I will change virt-v2v instead.
> >---
> >  plugins/vddk/nbdkit-vddk-plugin.pod | 7 +------
> >  plugins/vddk/vddk.c                 | 4 ++++
> >  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> This is the simplest "fix", by avoiding the problem. It may still
be
> possible to come up with something more robust, where (since we are
> already special-casing for "-" as the password value) we read the
> password prior to re-exec, and rewrite the command line of the
> re-exec to instead consume a temporary file.  But that's more
> complex, and doesn't stop us from pushing this now; if we ever do
> implement the more complex patch, we can roll back the documentation
> change.
> 
> And I already see that you have the counterpart v2v patch that
> hoists the reading of the password into v2v instead of worrying
> about vddk; the v2v hack of using a temporary file "works" in
spite
> of our re-exec reading it twice, even if we want to eventually reach
> the point where a more complex patch in the vddk plugin would only
> read the password once before re-exec.
The other advantage with doing this in virt-v2v is that there is a
place to clean up this temporary file.  There was as far as I could
tell no easy way for nbdkit to reexec itself with
password=+/tmp/<generated file> and have that file get cleaned up.
Not a problem since the file is not large, but nicer not to leave
passwords around if we can help it.

Thanks,

Rich.
> ACK.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
-- 
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