Libguestfs - Oct 2019 - Re: [PATCH nbdkit] server: Allow file descriptors to be passed to nbdkit_read_password.

Allow password parameters such as ‘password=-FD’ where FD is a file
descriptor number inherited by nbdkit from the parent process.  This
is another way to allow programs to hand passwords to nbdkit in a very
secure way, for example over a pipe so they never touch the
filesystem.

Previously nbdkit allowed you to use literal passwords on the command
line if they began with a ‘-’ (but were not just that single
character).  However that was contrary to the documentation, and this
commit now prevents that.
---
 docs/nbdkit-plugin.pod              |  4 ++
 plugins/curl/nbdkit-curl-plugin.pod | 10 ++++-
 plugins/ssh/nbdkit-ssh-plugin.pod   |  8 +++-
 plugins/vddk/nbdkit-vddk-plugin.pod | 11 +++++-
 server/public.c                     | 58 +++++++++++++++++++++--------
 server/test-public.c                | 31 +++++++++++++++
 6 files changed, 102 insertions(+), 20 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index e34ffd1..0afd5e1 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -1120,6 +1120,10 @@ or from a file:
 
  nbdkit myplugin password=+/tmp/secret
 
+or from a file descriptor inherited by nbdkit:
+
+ nbdkit myplugin password=-99
+
 (If the password begins with a C<-> or C<+> character then it must
be
 passed in a file).
 
diff --git a/plugins/curl/nbdkit-curl-plugin.pod
b/plugins/curl/nbdkit-curl-plugin.pod
index 63500a4..827e0bd 100644
--- a/plugins/curl/nbdkit-curl-plugin.pod
+++ b/plugins/curl/nbdkit-curl-plugin.pod
@@ -70,10 +70,16 @@ Ask for the password (interactively) when nbdkit starts up.
 
 =item B<password=+>FILENAME
 
-Read the password from the named file.  This is the most secure method
+Read the password from the named file.  This is a secure method
 to supply a password, as long as you set the permissions on the file
 appropriately.
 
+=item B<password=->FD
+
+Read the password from file descriptor number C<FD>, inherited from
+the parent process when nbdkit starts up.  This is also a secure
+method to supply a password.
+
 =item B<protocols=>PROTO,PROTO,...
 
 Limit the protocols that are allowed in the URL.  Use this option for
@@ -100,6 +106,8 @@ The default is to allow any protocol.
 
 =item B<proxy-password=+>FILENAME
 
+=item B<proxy-password=->FD
+
 =item B<proxy-user=>USERNAME
 
 Set the proxy username and password.
diff --git a/plugins/ssh/nbdkit-ssh-plugin.pod
b/plugins/ssh/nbdkit-ssh-plugin.pod
index 0d0bc2b..687c08c 100644
--- a/plugins/ssh/nbdkit-ssh-plugin.pod
+++ b/plugins/ssh/nbdkit-ssh-plugin.pod
@@ -96,10 +96,16 @@ Ask for the password (interactively) when nbdkit starts up.
 
 =item B<password=+>FILENAME
 
-Read the password from the named file.  This is the most secure method
+Read the password from the named file.  This is a secure method
 to supply a password, as long as you set the permissions on the file
 appropriately.
 
+=item B<password=->FD
+
+Read the password from file descriptor number C<FD>, inherited from
+the parent process when nbdkit starts up.  This is also a secure
+method to supply a password.
+
 =item [B<path=>]PATH
 
 Specify the path to the remote file.  This can be a relative path in
diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod
b/plugins/vddk/nbdkit-vddk-plugin.pod
index bf6f7e7..4ae647c 100644
--- a/plugins/vddk/nbdkit-vddk-plugin.pod
+++ b/plugins/vddk/nbdkit-vddk-plugin.pod
@@ -6,7 +6,8 @@ nbdkit-vddk-plugin - nbdkit VMware VDDK plugin
 
  nbdkit vddk file=FILENAME [config=FILENAME] [libdir=LIBRARY]
              [vm=moref=ID] [server=HOSTNAME] [user=USERNAME]
-             [password=PASSWORD | password=- | password=+FILENAME]
+             [password=PASSWORD | password=- | password=+FILENAME |
+              password=-FD]
              [cookie=COOKIE] [thumbprint=THUMBPRINT]
              [port=PORT] [nfchostport=PORT] [single-link=true]
              [snapshot=MOREF] [transports=MODE:MODE:...]
@@ -140,10 +141,16 @@ Ask for the password (interactively) when nbdkit starts
up.
 
 =item B<password=+>FILENAME
 
-Read the password from the named file.  This is the most secure method
+Read the password from the named file.  This is a secure method
 to supply a password, as long as you set the permissions on the file
 appropriately.
 
+=item B<password=->FD
+
+Read the password from file descriptor number C<FD>, inherited from
+the parent process when nbdkit starts up.  This is also a secure
+method to supply a password.
+
 =item B<port=>PORT
 
 The port on the VCenter/ESXi host.  Defaults to 443.
diff --git a/server/public.c b/server/public.c
index 9a3aa31..418945f 100644
--- a/server/public.c
+++ b/server/public.c
@@ -405,6 +405,8 @@ nbdkit_parse_bool (const char *str)
 }
 
 /* Read a password from configuration value. */
+static int read_password_from_fd (const char *what, int fd, char **password);
+
 int
 nbdkit_read_password (const char *value, char **password)
 {
@@ -412,7 +414,6 @@ nbdkit_read_password (const char *value, char **password)
   struct termios orig, temp;
   ssize_t r;
   size_t n;
-  FILE *fp;
 
   *password = NULL;
 
@@ -448,6 +449,16 @@ nbdkit_read_password (const char *value, char **password)
       (*password)[r-1] = '\0';
   }
 
+  /* Read from numbered file descriptor. */
+  else if (value[0] == '-') {
+    int fd;
+
+    if (nbdkit_parse_int ("password file descriptor", &value[1],
&fd) == -1)
+      return -1;
+    if (read_password_from_fd (&value[1], fd, password) == -1)
+      return -1;
+  }
+
   /* Read password from a file. */
   else if (value[0] == '+') {
     int fd;
@@ -457,22 +468,8 @@ nbdkit_read_password (const char *value, char **password)
       nbdkit_error ("open %s: %m", &value[1]);
       return -1;
     }
-    fp = fdopen (fd, "r");
-    if (fp == NULL) {
-      nbdkit_error ("fdopen %s: %m", &value[1]);
-      close (fd);
+    if (read_password_from_fd (&value[1], fd, password) == -1)
       return -1;
-    }
-    r = getline (password, &n, fp);
-    err = errno;
-    fclose (fp);
-    if (r == -1) {
-      errno = err;
-      nbdkit_error ("could not read password from file %s: %m",
&value[1]);
-      return -1;
-    }
-    if (*password && r > 0 && (*password)[r-1] ==
'\n')
-      (*password)[r-1] = '\0';
   }
 
   /* Parameter is the password. */
@@ -487,6 +484,35 @@ nbdkit_read_password (const char *value, char **password)
   return 0;
 }
 
+static int
+read_password_from_fd (const char *what, int fd, char **password)
+{
+  FILE *fp;
+  size_t n;
+  ssize_t r;
+  int err;
+
+  fp = fdopen (fd, "r");
+  if (fp == NULL) {
+    nbdkit_error ("fdopen %s: %m", what);
+    close (fd);
+    return -1;
+  }
+  r = getline (password, &n, fp);
+  err = errno;
+  fclose (fp);
+  if (r == -1) {
+    errno = err;
+    nbdkit_error ("could not read password from %s: %m", what);
+    return -1;
+  }
+
+  if (*password && r > 0 && (*password)[r-1] ==
'\n')
+    (*password)[r-1] = '\0';
+
+  return 0;
+}
+
 int
 nbdkit_nanosleep (unsigned sec, unsigned nsec)
 {
diff --git a/server/test-public.c b/server/test-public.c
index ea10189..4a7eb17 100644
--- a/server/test-public.c
+++ b/server/test-public.c
@@ -335,6 +335,8 @@ test_nbdkit_read_password (void)
 {
   bool pass = true;
   char template[] = "+/tmp/nbdkit_testpw_XXXXXX";
+  char template2[] = "/tmp/nbdkit_testpw2_XXXXXX";
+  char fdbuf[16];
   char *pw = template;
   int fd;
 
@@ -391,6 +393,35 @@ test_nbdkit_read_password (void)
     unlink (&template[1]);
   }
 
+  /* Test reading password from file descriptor. */
+  fd = mkstemp (template2);
+  if (fd < 0) {
+    perror ("mkstemp");
+    pass = false;
+  }
+  else if (write (fd, "abc\n", 4) != 4) {
+    fprintf (stderr, "Failed to write to file %s\n", template2);
+    pass = false;
+  }
+  else {
+    snprintf (fdbuf, sizeof fdbuf, "-%d", fd);
+    lseek (fd, 0, 0);
+    if (nbdkit_read_password (fdbuf, &pw) == -1) {
+      fprintf (stderr, "Failed to read password from fd %s\n",
fdbuf);
+      pass = false;
+    }
+    else if (strcmp (pw, "abc") != 0) {
+      fprintf (stderr, "Wrong file password, expected 'abc' got
'%s'\n", pw);
+      pass = false;
+    }
+    free (pw);
+  }
+
+  if (fd >= 0) {
+    /* Don't close fd, it is closed by nbdkit_read_password. */
+    unlink (template2);
+  }
+
   if (error_flagged) {
     fprintf (stderr, "Wrong error message handling\n");
     pass = false;
-- 
2.23.0

On 10/17/19 4:38 PM, Richard W.M. Jones wrote:
> Allow password parameters such as ‘password=-FD’ where FD is a file
> descriptor number inherited by nbdkit from the parent process.  This
> is another way to allow programs to hand passwords to nbdkit in a very
> secure way, for example over a pipe so they never touch the
> filesystem.
> 
> Previously nbdkit allowed you to use literal passwords on the command
> line if they began with a ‘-’ (but were not just that single
> character).  However that was contrary to the documentation, and this
> commit now prevents that.
> ---
>   
> +static int
> +read_password_from_fd (const char *what, int fd, char **password)
> +{
> +  FILE *fp;
> +  size_t n;
> +  ssize_t r;
> +  int err;
> +
> +  fp = fdopen (fd, "r");
> +  if (fp == NULL) {
> +    nbdkit_error ("fdopen %s: %m", what);
> +    close (fd);
> +    return -1;
> +  }
> +  r = getline (password, &n, fp);
This prevents a password from containing a newline.  Is that a problem? 
Can a password contain a literal newline when passed literally through 
the command line?  If so, that feels inconsistent.
> +  err = errno;
> +  fclose (fp);
> +  if (r == -1) {
> +    errno = err;
> +    nbdkit_error ("could not read password from %s: %m", what);
> +    return -1;
> +  }
> +
> +  if (*password && r > 0 && (*password)[r-1] ==
'\n')
> +    (*password)[r-1] = '\0';
> +
> +  return 0;
> +}
> +
> +++ b/server/test-public.c
> @@ -335,6 +335,8 @@ test_nbdkit_read_password (void)
>   {
>     bool pass = true;
>     char template[] = "+/tmp/nbdkit_testpw_XXXXXX";
> +  char template2[] = "/tmp/nbdkit_testpw2_XXXXXX";
> +  char fdbuf[16];
>     char *pw = template;
>     int fd;
>   
> @@ -391,6 +393,35 @@ test_nbdkit_read_password (void)
>       unlink (&template[1]);
>     }
>   
> +  /* Test reading password from file descriptor. */
> +  fd = mkstemp (template2);
> +  if (fd < 0) {
> +    perror ("mkstemp");
> +    pass = false;
> +  }
> +  else if (write (fd, "abc\n", 4) != 4) {
> +    fprintf (stderr, "Failed to write to file %s\n", template2);
> +    pass = false;
> +  }
But at least you test that newlines are stripped when using a file 
descriptor, so the addition seems self-consistent (even if we need more 
documentation on how newlines in passwords are handled).

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

On Thu, Oct 17, 2019 at 04:50:20PM -0500, Eric Blake
wrote:
> On 10/17/19 4:38 PM, Richard W.M. Jones wrote:
> >Allow password parameters such as ‘password=-FD’ where FD is a file
> >descriptor number inherited by nbdkit from the parent process.  This
> >is another way to allow programs to hand passwords to nbdkit in a very
> >secure way, for example over a pipe so they never touch the
> >filesystem.
> >
> >Previously nbdkit allowed you to use literal passwords on the command
> >line if they began with a ‘-’ (but were not just that single
> >character).  However that was contrary to the documentation, and this
> >commit now prevents that.
> >---
> 
> >+static int
> >+read_password_from_fd (const char *what, int fd, char **password)
> >+{
> >+  FILE *fp;
> >+  size_t n;
> >+  ssize_t r;
> >+  int err;
> >+
> >+  fp = fdopen (fd, "r");
> >+  if (fp == NULL) {
> >+    nbdkit_error ("fdopen %s: %m", what);
> >+    close (fd);
> >+    return -1;
> >+  }
> >+  r = getline (password, &n, fp);
> 
> This prevents a password from containing a newline.  Is that a
> problem? Can a password contain a literal newline when passed
> literally through the command line?  If so, that feels inconsistent.
I believe that's also a problem with the current code, as I simply
factored out this function from the existing code for "+file".  Anyone
who has a password containing a newline presumably also has a problem
logging in (to any reasonable server)?

Rich.
> >+  err = errno;
> >+  fclose (fp);
> >+  if (r == -1) {
> >+    errno = err;
> >+    nbdkit_error ("could not read password from %s: %m",
what);
> >+    return -1;
> >+  }
> >+
> >+  if (*password && r > 0 && (*password)[r-1] ==
'\n')
> >+    (*password)[r-1] = '\0';
> >+
> >+  return 0;
> >+}
> >+
> 
> >+++ b/server/test-public.c
> >@@ -335,6 +335,8 @@ test_nbdkit_read_password (void)
> >  {
> >    bool pass = true;
> >    char template[] = "+/tmp/nbdkit_testpw_XXXXXX";
> >+  char template2[] = "/tmp/nbdkit_testpw2_XXXXXX";
> >+  char fdbuf[16];
> >    char *pw = template;
> >    int fd;
> >@@ -391,6 +393,35 @@ test_nbdkit_read_password (void)
> >      unlink (&template[1]);
> >    }
> >+  /* Test reading password from file descriptor. */
> >+  fd = mkstemp (template2);
> >+  if (fd < 0) {
> >+    perror ("mkstemp");
> >+    pass = false;
> >+  }
> >+  else if (write (fd, "abc\n", 4) != 4) {
> >+    fprintf (stderr, "Failed to write to file %s\n",
template2);
> >+    pass = false;
> >+  }
> 
> But at least you test that newlines are stripped when using a file
> descriptor, so the addition seems self-consistent (even if we need
> more documentation on how newlines in passwords are handled).
> 
> -- 
> 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
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/