[libvirt PATCH v2 15/16] qemu: pass sensitive data to nbdkit via pipe

[Peter Krempa]

On Wed, Aug 31, 2022 at 13:41:00 -0500, Jonathon Jongsma wrote:
> Rather than passing passwords and cookies (which could contain
> passwords) to nbdkit via commandline arguments, use the alternate format
> that nbdkit supports where we can specify a file descriptor which nbdkit
> will read to get the password or cookies.
> 
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---

[...]

> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index 0ecf6c6537..2b8e203d16 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c
> @@ -55,6 +55,76 @@ VIR_ENUM_IMPL(qemuNbdkitCaps,
>      "filter-readahead", /* QEMU_NBDKIT_CAPS_FILTER_READAHEAD */
>  );
>  
> +
> +static void
> +nbdkitPipeItemFree(NbdkitPipeItem *item)

Please use consistent function naming, both for the function and for the
data.

> +{
> +    if (item->buf) {
> +        virSecureErase(item->buf, item->buflen);
> +        g_free(item->buf);
> +    }
> +
> +    if (item->fd > 0)
> +        VIR_FORCE_CLOSE(item->fd);
> +
> +    g_free(item);
> +}
> +
> +
> +void nbdkitPipeDataFree(NbdkitPipeData *self)

And consistent header formatting.

> +{
> +    size_t i;
> +
> +    if (!self)
> +        return;
> +
> +    for (i = 0; i < self->nitems; i++) {
> +        nbdkitPipeItemFree(self->items[i]);
> +    }
> +
> +    g_free(self->items);
> +    g_free(self);
> +}
> +
> +
> +static NbdkitPipeItem*
> +nbdkitPipeItemNew(int fd, void *data, int datalen)
> +{
> +    NbdkitPipeItem *d;
> +
> +    if (!data || datalen == 0)
> +        return NULL;
> +
> +    d = g_new0(NbdkitPipeItem, 1);
> +    d->fd = fd;
> +
> +    if (datalen < 0) {
> +        /* -1 indicates a null-terminated string */
> +        d->buf = g_strdup(data);
> +        d->buflen = strlen(data);
> +    } else {
> +        d->buf = g_malloc(datalen);
> +        memcpy(d->buf, data, datalen);
> +        d->buflen = datalen;
> +    }
> +
> +    return d;
> +}
> +
> +
> +static int
> +nbdkitPipeDataWrite(NbdkitPipeItem *pipe)
> +{
> +    if (safewrite(pipe->fd, pipe->buf, pipe->buflen) < 0) {

Note that the pipes created by virPipe are not non-blocking. This call
will block if you write into the pipe more than 64kiB on linux.

Normally the size should be enough but you at least need to have
something preventing the process getting stuck.

I think the best case would be if virCommand allowed simply being passed
a buffer and serving the pipes automatically.

> +        virReportSystemError(errno,
> +                             _("failed to write data to pipe %i for nbdkit"),
> +                             pipe->fd);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +
>  struct _qemuNbdkitCaps {
>      GObject parent;
>  


[...]

> @@ -685,12 +761,36 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps,
>  }
>  
>  
> +static NbdkitPipeItem*
> +commandPassDataByPipe(virCommand *cmd,

Again inconsistent naming.

> +                      const char *argName,
> +                      char *buf,
> +                      size_t buflen)
> +{
> +    int fds[2] = { -1, -1 };
> +    g_autofree char *fdfmt = NULL;
> +
> +    if (virPipe(fds) < 0)
> +        return NULL;
> +
> +    /* some nbdkit arguments accept a variation where nbdkit will read the data
> +     * from a file descriptor, e.g. password=-FD */
> +    fdfmt = g_strdup_printf("-%i", fds[PIPE_FD_READ]);
> +    virCommandAddArgPair(cmd, argName, fdfmt);
> +    virCommandPassFD(cmd, fds[PIPE_FD_READ], VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> +
> +    return nbdkitPipeItemNew(fds[PIPE_FD_WRITE], (char*)buf, buflen);
> +}
> +
>  static int
>  qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
> -                                  virCommand *cmd)
> +                                  virCommand *cmd,
> +                                  NbdkitPipeData **pipeData)
>  {
>      g_autoptr(virURI) uri = qemuBlockStorageSourceGetURI(proc->source);
>      g_autofree char *uristring = virURIFormat(uri);
> +    g_autoptr(GPtrArray) pipes =
> +        g_ptr_array_new_with_free_func((GDestroyNotify)nbdkitPipeDataFree);

A GSlist or GList would also do well here since you never actually
randomly access the elements thus don't need them to be numbered.

>  
>      /* nbdkit plugin name */
>      virCommandAddArg(cmd, "curl");

[...]