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@redhat.com>
---
 src/qemu/qemu_nbdkit.c | 55 ++++++++++++++++++++++++++++++------------
 1 file changed, 40 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
index 2b26e9bc08..ba84958e8d 100644
--- a/src/qemu/qemu_nbdkit.c
+++ b/src/qemu/qemu_nbdkit.c
@@ -67,6 +67,12 @@ struct _qemuNbdkitCaps {
 G_DEFINE_TYPE(qemuNbdkitCaps, qemu_nbdkit_caps, G_TYPE_OBJECT);
 
 
+enum {
+    PIPE_FD_READ = 0,
+    PIPE_FD_WRITE = 1
+};
+
+
 static void
 qemuNbdkitCheckCommandCap(qemuNbdkitCaps *nbdkit,
                           virCommand *cmd,
@@ -759,6 +765,29 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps,
 }
 
 
+static int
+qemuNbdkitCommandPassDataByPipe(virCommand *cmd,
+                                const char *argName,
+                                unsigned char **buf,
+                                size_t buflen)
+{
+    g_autofree char *fdfmt = NULL;
+    int fd = virCommandSetSendBuffer(cmd, buf, buflen);
+
+    if (fd < 0)
+        return -1;
+
+    /* 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", fd);
+    virCommandAddArgPair(cmd, argName, fdfmt);
+
+    virCommandDoAsyncIO(cmd);
+
+    return 0;
+}
+
+
 static int
 qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
                                   virCommand *cmd)
@@ -784,7 +813,6 @@ qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
         g_autoptr(virConnect) conn = virGetConnectSecret();
         g_autofree uint8_t *secret = NULL;
         size_t secretlen = 0;
-        g_autofree char *password = NULL;
         int secrettype;
         virStorageAuthDef *authdef = proc->source->auth;
 
@@ -808,22 +836,19 @@ qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
             return -1;
         }
 
-        /* ensure that the secret is a NULL-terminated string */
-        password = g_strndup((char*)secret, secretlen);
-
-        /* for now, just report an error rather than passing the password in
-         * cleartext on the commandline */
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("Password not yet supported for nbdkit sources"));
-        return -1;
+        if (qemuNbdkitCommandPassDataByPipe(cmd, "password",
+                                            &secret, secretlen) < 0)
+            return -1;
     }
 
-    if (proc->source->ncookies > 0) {
-        /* for now, just report an error rather than passing cookies in
-         * cleartext on the commandline */
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("Cookies not yet supported for nbdkit sources"));
-        return -1;
+    /* Create a pipe to send the cookies to the nbdkit process. */
+    if (proc->source->ncookies) {
+        g_autofree char *cookies = qemuBlockStorageSourceGetCookieString(proc->source);
+
+        if (qemuNbdkitCommandPassDataByPipe(cmd, "cookie",
+                                            (unsigned char**)&cookies,
+                                            strlen(cookies)) < 0)
+            return -1;
     }
 
     if (proc->source->sslverify == VIR_TRISTATE_BOOL_NO) {
-- 
2.39.0

On Fri, Jan 20, 2023 at 16:03:13 -0600, 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@redhat.com>
> ---
>  src/qemu/qemu_nbdkit.c | 55 ++++++++++++++++++++++++++++++------------
>  1 file changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index 2b26e9bc08..ba84958e8d 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c
> @@ -67,6 +67,12 @@ struct _qemuNbdkitCaps {
>  G_DEFINE_TYPE(qemuNbdkitCaps, qemu_nbdkit_caps, G_TYPE_OBJECT);
>  
>  
> +enum {
> +    PIPE_FD_READ = 0,
> +    PIPE_FD_WRITE = 1
> +};

The values are not used.

> +
> +
>  static void
>  qemuNbdkitCheckCommandCap(qemuNbdkitCaps *nbdkit,
>                            virCommand *cmd,
> @@ -759,6 +765,29 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps,
>  }
>  
>  
> +static int
> +qemuNbdkitCommandPassDataByPipe(virCommand *cmd,
> +                                const char *argName,
> +                                unsigned char **buf,
> +                                size_t buflen)
> +{
> +    g_autofree char *fdfmt = NULL;
> +    int fd = virCommandSetSendBuffer(cmd, buf, buflen);
> +
> +    if (fd < 0)
> +        return -1;
> +
> +    /* 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", fd);
> +    virCommandAddArgPair(cmd, argName, fdfmt);
> +
> +    virCommandDoAsyncIO(cmd);
> +
> +    return 0;
> +}
> +
> +
>  static int
>  qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
>                                    virCommand *cmd)
> @@ -808,22 +836,19 @@ qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
>              return -1;
>          }
>  
> -        /* ensure that the secret is a NULL-terminated string */
> -        password = g_strndup((char*)secret, secretlen);
> -
> -        /* for now, just report an error rather than passing the password in
> -         * cleartext on the commandline */
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("Password not yet supported for nbdkit sources"));
> -        return -1;
> +        if (qemuNbdkitCommandPassDataByPipe(cmd, "password",
> +                                            &secret, secretlen) < 0)
> +            return -1a

Don't forget to destroy 'secret' using virSecureErase.

>      }
>  
> -    if (proc->source->ncookies > 0) {
> -        /* for now, just report an error rather than passing cookies in
> -         * cleartext on the commandline */
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("Cookies not yet supported for nbdkit sources"));

Reviewed-by: Peter Krempa <pkrempa@redhat.com>

On 2/7/23 10:51 AM, Peter Krempa wrote:
> On Fri, Jan 20, 2023 at 16:03:13 -0600, Jonathon Jongsma wrote:

...

>> @@ -759,6 +765,29 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps,
>>   }
>>   
>>   
>> +static int
>> +qemuNbdkitCommandPassDataByPipe(virCommand *cmd,
>> +                                const char *argName,
>> +                                unsigned char **buf,
>> +                                size_t buflen)
>> +{
>> +    g_autofree char *fdfmt = NULL;
>> +    int fd = virCommandSetSendBuffer(cmd, buf, buflen);
>> +
>> +    if (fd < 0)
>> +        return -1;
>> +
>> +    /* 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", fd);
>> +    virCommandAddArgPair(cmd, argName, fdfmt);
>> +
>> +    virCommandDoAsyncIO(cmd);
>> +
>> +    return 0;
>> +}
>> +
>> +
>>   static int
>>   qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
>>                                     virCommand *cmd)
>> @@ -808,22 +836,19 @@ qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
>>               return -1;
>>           }
>>   
>> -        /* ensure that the secret is a NULL-terminated string */
>> -        password = g_strndup((char*)secret, secretlen);
>> -
>> -        /* for now, just report an error rather than passing the password in
>> -         * cleartext on the commandline */
>> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> -                       _("Password not yet supported for nbdkit sources"));
>> -        return -1;
>> +        if (qemuNbdkitCommandPassDataByPipe(cmd, "password",
>> +                                            &secret, secretlen) < 0)
>> +            return -1a
> 
> Don't forget to destroy 'secret' using virSecureErase.

hmm. 'secret' is passed to virCommandSetSendBuffer() (see above), which 
takes ownership of the buffer, so I can't actually free it. It looks 
like all users of virCommandSetSendBuffer() are doing so in order to 
pass sensitive data to a child process securely (For example, 
qemuTPMSetupEncryption() has this same issue). I can add a new commit 
changing virCommand to free all send buffers with virSecureErase().

Jonathon