[libvirt PATCH v3 09/18] qemu: add functions to start and stop nbdkit

[Jonathon Jongsma]

On 12/9/22 4:09 AM, Peter Krempa wrote:
> On Thu, Oct 20, 2022 at 16:59:00 -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 at redhat.com>
>> ---
>>   src/qemu/qemu_nbdkit.c | 251 +++++++++++++++++++++++++++++++++++++++++
>>   src/qemu/qemu_nbdkit.h |  10 ++
>>   2 files changed, 261 insertions(+)
>>

...


>> +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;
> 
> So there's a bit problem with this, which also explains my reluctance to
> simply supporting the SSH protocol in the current state without properly
> designing other required parameters for SSH authentication. And even
> then it will most likely break "historical compatibility".
> 
> So historically we didn't implement ssh protocol at all. Users (notably
> libguestfs) worked this around by adding a wrapper QCOW2 image and
> setting up the backing store string such that qemu would access the ssh
> image.
> 
> When blockdev support was added I needed a way to port the existing
> functionality specifically for libguestfs. This made me add the ssh
> protocol and forwart the minimum set of properties to the image (thus
> the 'ssh_user' field.
> 
> But there's another thing that was always configured out-of-band (via
> environment variables) and that is the ssh agent socket path or ssh key
> path.
> 
> These are not present in the virStorageSource because they are not even
> configured for the blockdev 'ssh' protocol driver directly.
> 
> Now with switching to nbdkit this means agent/sshkey authentication will
> necessarily break for such usage as we simply don't have the
> information.
> 
> So what to do about it?
> 
> We can either break the old way completely, which I guess would be
> mostly okay since libguestfs most likely now uses nbdkit anyways, and
> then design it properly. Obviously since the qemu ssh blockdev backend
> driver still configures stuff using environment variables thus the new
> configuration will be available only when nbdkit is in use.
> 
> Other possibility, if we want to keep old functionality working as it
> was, is to avoid the use of nbdkit.
> 
> Either way the implementation of SSH as done in this series simply just
> breaks SSH usage completely, by not being able to support the old hacky
> way of using agent/sshkey, not implementing any new support and not even
> implementing password authentication.
> 
> What now? I don't really care too deeply about the hacky impl we have
> now. If libguestfs is no longer using I reckon we can simply break it
> and not deal with the fallout.
> 
> I'm not sure how others care about that though.
> 
> Obviously another option (besides doing a proper desing on
> authentication config) is to not touch anything ssh-related for now.
> 

Adding Rich to cc in case he has anything to add to this discussion.

The problem, as I understand it, is the desire to remove the qemu block 
plugins from distributions. If that happens, then avoiding the use of 
nbdkit within libvirt won't solve anything -- the ssh use case will 
still break due to the absence of these qemu plugins.

I'm afraid I don't have enough background information about when ssh is 
used and how common it is to make these decisions on my own. But I agree 
that the current implementation in this patch series is not complete 
enough to be useful.

Jonathon