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

On Fri, Dec 09, 2022 at 11:17:22AM -0600, Jonathon Jongsma wrote: > 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(a)redhat.com&gt; > >>--- > >> 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. Off topic, but I wanted to check what we do now. We generate this XML fragment: $ guestfish --format=raw -a ssh://foo/var/tmp/fedora-36.img -i -vx ... <disk device="disk" type="network"> <source protocol="ssh" name="/var/tmp/fedora-36.img"> <host name="foo"/> </source> <target dev="sda" bus="scsi"/> <driver name="qemu" type="raw" cache="writeback"/> <address type="drive" controller="0" bus="0" target="0" unit="0"/> </disk> which I think is correct(?)

> >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. Right - this doesn't work in fact: Original error from libvirt: internal error: process exited while connecting to monitor: 2022-12-09T18:16:45.674960Z qemu-kvm: -blockdev {"driver":"ssh","path":"/var/tmp/fedora-36.img","server":{"host":"foo","port":"22"},"node-name":"libvirt-2-storage","cache":{"direct":false,"no-flush":false},"auto-read-only":true,"discard":"unmap"}: failed to authenticate using publickey authentication and the identities held by your ssh-agent [code=1 int1=-1]

Am I doing something wrong here? I have to confess it's been a very long time since I tried to use the ssh support directly in libguestfs (see below for why). So I don't quite remember how it's supposed to work, or else it broke and I didn't notice.

> >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. So for libguestfs as above we're using libvirt (and hence indirectly qemu's ssh). That's broken, but no one reported the bug so I guess it's little used.

But you're right that for virt-v2v we switched to using nbdkit-ssh-plugin a while back and that really is used and tested extensively. (In this case we use libvirt + qemu's nbd driver to talk to the nbdkit endpoint.) > >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. From the libguestfs point of view, we are generating the <disk .. type="network"><source protocol="ssh"> fragment above and hoping that libvirt's implementation does the right thing.