[Libguestfs] [PATCH nbdkit 5/5] vddk: Munge password parameters when we reexec (RHBZ#1842440).

[Eric Blake]

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