Richard W.M. Jones
2019-Oct-17 21:38 UTC
[Libguestfs] [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
Eric Blake
2019-Oct-17 21:50 UTC
Re: [Libguestfs] [PATCH nbdkit] server: Allow file descriptors to be passed to nbdkit_read_password.
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
Richard W.M. Jones
2019-Oct-18 07:41 UTC
Re: [Libguestfs] [PATCH nbdkit] server: Allow file descriptors to be passed to nbdkit_read_password.
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/
Possibly Parallel Threads
- Re: [PATCH nbdkit] server: Allow file descriptors to be passed to nbdkit_read_password.
- [nbdkit PATCH v2 1/3] server: Add nbdkit_stdio_safe
- [nbdkit PATCH 1/2] server: Add nbdkit_stdio_safe
- [PATCH nbdkit 5/9 patch split 2/5] lib: Move code for parsing, passwords and paths into libnbdkit.so.
- [PATCH nbdkit 1/3] server: Disallow password=- from non-tty and fix error message (RHBZ#1842440).