9:48 a.m.
On 6/2/20 7:27 AM, Richard W.M. Jones wrote:
See this thread:
https://www.redhat.com/archives/libguestfs/2020-June/thread.html#00012
This commit also adds a regression test of vddk password=- and
password=-FD.
---
tests/Makefile.am | 4 ++
plugins/vddk/vddk.h | 1 +
plugins/vddk/reexec.c | 43 ++++++++++++-
plugins/vddk/vddk.c | 2 +-
tests/test-vddk-password-fd.sh | 86 +++++++++++++++++++++++++
tests/test-vddk-password-interactive.sh | 77 ++++++++++++++++++++++
tests/dummy-vddk.c | 6 ++
7 files changed, 217 insertions(+), 2 deletions(-)
@@ -122,7 +123,47 @@ perform_reexec (const char *env, const char
*prepend)
/* Split cmdline into argv, then append one more arg. */
for (len = 0; len < buf.size; len += strlen (buf.ptr + len) + 1) {
- if (string_vector_append (&argv, buf.ptr + len) == -1) {
+ char *arg = buf.ptr + len; /* Next \0-terminated argument. */
+
+ /* password parameter requires special handling for reexec. For
+ * password=- and password=-FD, after reexec we might try to
+ * reread these, but stdin has gone away and FD has been consumed
+ * already so that won't work. Even password=+FILE is a little
+ * problematic since the file will be read twice, which may break
+ * for special files.
+ *
+ * However we may write the password to a temporary file and
+ * substitute password=-<FD> of the opened temporary file here.
+ * The trick is described by Eric Blake here:
+ * https://www.redhat.com/archives/libguestfs/2020-June/msg00021.html
+ *
+ * (RHBZ#1842440)
+ */
Absolutely essential comment :) I think it accurately captures what the
reader needs to know.
+ if (strncmp (arg, "password=", 9) == 0) {
+ char tmpfile[] = "/tmp/XXXXXX";
+ char *password_fd;
+
+ /* These operations should never fail, so exit on error. */
+ fd = mkstemp (tmpfile);
+ if (fd == -1) {
+ nbdkit_error ("mkstemp: %m");
+ exit (EXIT_FAILURE);
+ }
+ unlink (tmpfile);
+ if (write (fd, password, strlen (password)) != strlen (password)) {
Is it worth assert(password) prior to this point? (We are guaranteed
that if we found a "password=..." parameter, the caller already parsed
it into the password variable).
Slight inefficiency: right now, .config allows 'password=' more than
once, and the last one wins. Our re-exec opens more than one fd in that
case, and we still end up with last one wins, but it might be nicer if
we only passed through the last password= rather than all of them.
Perhaps you could do that by a tweak to the logic - the loop through the
argv blindly skips all password= arguments, then after the loop, if
password is non-NULL, you open the temp file and append a password=FD
argument just once. Doing this re-arrangement would also help address
the fd leak...
+ nbdkit_error ("write: %m");
+ exit (EXIT_FAILURE);
+ }
+ lseek (fd, 0, SEEK_SET);
+ if (asprintf (&password_fd, "password=-%d", fd) == -1) {
+ nbdkit_error ("asprintf: %m");
+ exit (EXIT_FAILURE);
+ }
+ /* Leaked here but cleaned up when we exec below. */
+ arg = password_fd;
+ }
+
+ if (string_vector_append (&argv, arg) == -1) {
argv_realloc_fail:
nbdkit_debug ("argv: realloc: %m");
return;
... if execvp fails, we now leak password fd; we should close it before
returning.
+++ b/tests/test-vddk-password-fd.sh
+# Get dummy-vddk to print the password to stderr.
+export DUMMY_VDDK_PRINT_PASSWORD=1
+
+# Password -FD.
+echo 123 > $f
+exec 3< $f
+nbdkit -fv -U - vddk \
+ libdir=.libs \
+ server=noserver.example.com thumbprint=ab \
+ vm=novm /nofile \
+ user=root password=-3 \
+ --run 'qemu-img info $nbd' \
+ >&$out ||:
+exec 3<&-
+cat $out
Slick.
+++ b/tests/test-vddk-password-interactive.sh
+# password=- was broken in the VDDK plugin in nbdkit 1.18 through
+# 1.20.2. This was because the reexec code caused
+# nbdkit_read_password to be called twice, second time with stdin
+# reopened on /dev/null. Since we have now fixed this bug, this is a
+# regression test.
+
+requires qemu-img --version
+requires expect -v
As far as I can tell, this is our first use of expect. README should be
updated to mention the optional dependency.
+
+out=test-vddk-password-interactive.out
+cleanup_fn rm -f $out
+
+# Get dummy-vddk to print the password to stderr.
+export DUMMY_VDDK_PRINT_PASSWORD=1
+
+export out
+expect -f - <<'EOF'
+ spawn sh -c "
+ nbdkit -fv -U - \
+ vddk \
+ libdir=.libs \
+ server=noserver.example.com thumbprint=ab \
+ vm=novm /nofile \
+ user=root password=- \
+ --run 'qemu-img info \$nbd' \
+ 2>$env(out)"
+ expect "ssword:"
+ send "abc\r"
+ wait
+EOF
+cat $out
I'm not seeing an obvious race, I'll have to see if I can encounter it
in person. In the meantime, from what I know about using expect (which
isn't much), this looks like a reasonable way to get nbdkit to see a tty
and perform an interactive password request.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org