On Wed, Aug 31, 2022 at 13:40:54 -0500, Jonathon Jongsma wrote:
> Add some helper functions to build a virCommand object and run the
> nbdkit process for a given virStorageSource.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> ---
> src/qemu/qemu_nbdkit.c | 208 +++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_nbdkit.h | 8 ++
> 2 files changed, 216 insertions(+)
>
> diff --git a/src/qemu/qemu_nbdkit.c b/src/qemu/qemu_nbdkit.c
> index 4df8957196..e8f9acc17c 100644
> --- a/src/qemu/qemu_nbdkit.c
> +++ b/src/qemu/qemu_nbdkit.c
> @@ -648,6 +648,156 @@ qemuNbdkitInitStorageSource(qemuNbdkitCaps *caps,
> }
>
>
> +static int
> +qemuNbdkitProcessBuildCommandCurl(qemuNbdkitProcess *proc,
> + virCommand *cmd)
> +{
> + g_autoptr(virURI) uri = qemuBlockStorageSourceGetURI(proc->source);
> + g_autofree char *uristring = virURIFormat(uri);
> +
> + /* nbdkit plugin name */
> + virCommandAddArg(cmd, "curl");
> + virCommandAddArgPair(cmd, "protocols",
> + virStorageNetProtocolTypeToString(proc->source->protocol));
> + virCommandAddArgPair(cmd, "url", uristring);
> +
> + if (proc->source->auth) {
> + g_autoptr(virConnect) conn = virGetConnectSecret();
> + g_autofree uint8_t *secret = NULL;
> + size_t secretlen = 0;
> + g_autofree char *password = NULL;
> + int secrettype;
> +
> + virCommandAddArgPair(cmd, "user",
> + proc->source->auth->username);
> +
> + if ((secrettype = virSecretUsageTypeFromString(proc->source->auth->secrettype)) < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("invalid secret type %s"),
> + proc->source->auth->secrettype);
> + return -1;
> + }
> +
> + if (virSecretGetSecretString(conn,
> + &proc->source->auth->seclookupdef,
> + secrettype,
> + &secret,
> + &secretlen) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to get auth secret for storage"));
> + return -1;
> + }
> +
> + /* ensure that the secret is a NULL-terminated string */
> + password = g_strndup((char*)secret, secretlen);
> +
> + virCommandAddArgPair(cmd, "password", password);
I'd prefer to report error if password is used as dummy implementation
rather than having this insecure one.
> +
> + virSecureErase(secret, secretlen);
> + virSecureErase(password, secretlen);
This security theatre is pointless in this instace as you've just passed
the password into a buffer in the virCommand struct which is not
sanitized.
> + }
> +
> + if (proc->source->ncookies > 0)
> + virCommandAddArgPair(cmd, "cookie",
> + qemuBlockStorageSourceGetCookieString(proc->source));
Same with cookies. Additionally this leaks the buffer returned by
qemuBlockStorageSourceGetCookieString.
> +
> + if (proc->source->sslverify == VIR_TRISTATE_BOOL_NO) {
> + virCommandAddArgPair(cmd, "sslverify", "false");
> + }
> +
> + if (proc->source->timeout > 0) {
> + g_autofree char *timeout = g_strdup_printf("%llu", proc->source->timeout);
> + virCommandAddArgPair(cmd, "timeout", timeout);
> + }
> +
> + return 0;
> +}
> +
> +
> +static int
> +qemuNbdkitProcessBuildCommandSSH(qemuNbdkitProcess *proc,
> + virCommand *cmd)
> +{
> + const char *user = NULL;
> + virStorageNetHostDef *host = &proc->source->hosts[0];
> + g_autofree char *portstr = g_strdup_printf("%u", host->port);
> +
> + /* nbdkit plugin name */
> + virCommandAddArg(cmd, "ssh");
> +
> + virCommandAddArgPair(cmd, "host", host->name);
> + virCommandAddArgPair(cmd, "port", portstr);
> + virCommandAddArgPair(cmd, "path", proc->source->path);
> +
> + if (proc->source->auth)
> + user = proc->source->auth->username;
> + else if (proc->source->ssh_user)
> + user = proc->source->ssh_user;
> +
> + if (user)
> + virCommandAddArgPair(cmd, "user", user);
> +
> + if (proc->source->ssh_host_key_check_disabled)
> + virCommandAddArgPair(cmd, "verify-remote-host", "false");
To make the ssh driver useful you should at least add support for the
'password' and (requiring XML additions) config and identity options of
the ssh plugin of nbdkit.
> +
> + return 0;
> +}
> +
> +
> +static virCommand *
> +qemuNbdkitProcessBuildCommand(qemuNbdkitProcess *proc)
> +{
> + g_autoptr(virCommand) cmd = virCommandNewArgList(proc->caps->path,
> + "--exit-with-parent",
> + "--unix",
> + proc->socketfile,
> + "--foreground",
> + //"--selinux-label",
> + //selinux_label,
> + NULL);
> +
> + if (proc->source->readonly)
> + virCommandAddArg(cmd, "--readonly");
> +
> + if (qemuNbdkitCapsGet(proc->caps, QEMU_NBDKIT_CAPS_FILTER_READAHEAD) &&
> + proc->source->readahead > 0)
> + virCommandAddArgPair(cmd, "--filter", "readahead");
So the readahead filter can't be actually configured?
> +
> + switch (proc->source->protocol) {
> + case VIR_STORAGE_NET_PROTOCOL_HTTP:
> + case VIR_STORAGE_NET_PROTOCOL_HTTPS:
> + case VIR_STORAGE_NET_PROTOCOL_FTP:
> + case VIR_STORAGE_NET_PROTOCOL_FTPS:
> + case VIR_STORAGE_NET_PROTOCOL_TFTP:
> + if (qemuNbdkitProcessBuildCommandCurl(proc, cmd) < 0)
> + return NULL;
> + break;
> + case VIR_STORAGE_NET_PROTOCOL_SSH:
> + if (qemuNbdkitProcessBuildCommandSSH(proc, cmd) < 0)
> + return NULL;
> + break;
> +
> + case VIR_STORAGE_NET_PROTOCOL_NONE:
> + case VIR_STORAGE_NET_PROTOCOL_NBD:
> + case VIR_STORAGE_NET_PROTOCOL_RBD:
> + case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG:
> + case VIR_STORAGE_NET_PROTOCOL_GLUSTER:
> + case VIR_STORAGE_NET_PROTOCOL_ISCSI:
> + case VIR_STORAGE_NET_PROTOCOL_VXHS:
> + case VIR_STORAGE_NET_PROTOCOL_NFS:
> + case VIR_STORAGE_NET_PROTOCOL_LAST:
> + virReportError(VIR_ERR_NO_SUPPORT,
> + _("protocol '%s' is not supported by nbdkit"),
> + virStorageNetProtocolTypeToString(proc->source->protocol));
> + return NULL;
> + }
> +
> + virCommandDaemonize(cmd);
> +
> + return g_steal_pointer(&cmd);
> +}
> +
> +
> void
> qemuNbdkitProcessFree(qemuNbdkitProcess *proc)
> {
> @@ -661,3 +811,61 @@ qemuNbdkitProcessFree(qemuNbdkitProcess *proc)
> g_clear_object(&proc->caps);
> g_free(proc);
> }
> +
> +
> +int
> +qemuNbdkitProcessStart(qemuNbdkitProcess *proc,
> + virDomainObj *vm,
> + virQEMUDriver *driver)
> +{
> + g_autoptr(virCommand) cmd = NULL;
> + int rc;
> + int exitstatus = 0;
> + int cmdret = 0;
> + int errfd = -1;
> +
> + if (!(cmd = qemuNbdkitProcessBuildCommand(proc)))
> + return -1;
> +
> + virCommandSetErrorFD(cmd, &errfd);
> + virCommandSetPidFile(cmd, proc->pidfile);
> +
> + if (qemuExtDeviceLogCommand(driver, vm, cmd, "nbdkit") < 0)
> + goto error;
> +
> + if (qemuSecurityCommandRun(driver, vm, cmd, proc->user, proc->group, &exitstatus, &cmdret) < 0)
> + goto error;
> +
> + if (cmdret < 0 || exitstatus != 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not start 'nbdkit'. exitstatus: %d"), exitstatus);
> + goto error;
> + }
> +
> + if ((rc = virPidFileReadPath(proc->pidfile, &proc->pid)) < 0) {
> + virReportSystemError(-rc,
> + _("Failed to read pidfile %s"),
> + proc->pidfile);
> + goto error;
> + }
> +
> + return 0;
> +
> + error:
> + if (errfd > 0) {
> + char errbuf[1024] = { 0 };
Please don't stack-allocate huge buffers.
> + if (read(errfd, errbuf, sizeof(errbuf) - 1) > 0)
> + VIR_WARN("nbdkit failed to start: %s", errbuf);
Why is this just a VIR_WARN, when the rest of the code uses
virReportError?
Additionally if I'm not mistaken by cleanup of virCommand this function
leaks errfd.
Also if something is written by nbdkit to stderr during runtime and the
pipe is not closed the pipe buffer can fill up.
> + }
> + if (proc->pid)
> + virProcessKillPainfully(proc->pid, true);
> + unlink(proc->pidfile);
> + return -1;
> +}
