11:07 a.m.
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
12:13 p.m.
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
12:16 p.m.
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
12:31 p.m.
On 6/1/20 12:16 PM, Richard W.M. Jones wrote:
> 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.
Sure there is:
user passes password=-
.config reads the password interactively, into a variable
re-exec code sees that we have a password string, so it calls:
fd=open("/safe/temp/name", O_CREAT|O_EXCL|O_RDWR, 0600)
unlink("/safe/temp/name")
write(fd, password...)
lseek(fd, 0, SEEK_SET)
rewrite command-line to use password="-fd"
re-exec with altered command-line
.config sees password=-N, reads from the temporary file, which closes
the fd
That would also solve the fact that even with password=/path/to/file,
our re-exec code is reading the password twice; or worse, with
password=-4, we are not rewinding the file position of fd 4 (if it is
seekable) and the re-exec is reading from the wrong position (worse if
fd 4 is non-seekable, and we cannot rewind).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
1635
days inactive
1635
days old
3 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones