[libvirt PATCH 07/11] virnetclient: Use 'if' consistently

[Daniel P. Berrangé]

On Mon, Feb 14, 2022 at 08:09:57AM -0800, Andrea Bolognani wrote:
> On Mon, Feb 14, 2022 at 10:25:48AM +0000, Daniel P. Berrangé wrote:
> > > > I understand the motivation, but please don't change this. Applications
> > > > like OpenStack have configured ssh authorized_keys files with the
> > > > specific command that libvirt invokes. So changes like this will break
> > > > their SSH configs.  We caused this pain when we first introduced the
> > > > virt-ssh-helper, but at least that was giving them a functional
> > > > improvement and they could use a URI parameter to force the old command
> > > > string. This change is just prettiness for no functional improvement
> > > > so is not worth breaking apps for.
> > >
> > > Can you please provide pointers to the OpenStack implementation of
> > > this and the issue that resulted from introducing virt-ssh-helper?
> >
> > I don't know where the code is. I just know that they were broken
> > by our changes in this area.
> >
> > > AFAICT the only way to restrict what commands a user can run after
> > > successfully authenticating is to specify command=... before the
> > > corresponding key in authorized_keys and I don't see how this change,
> > > or indeed the one that happened when virt-ssh-helper was added, could
> > > interfere with that.
> >
> > The command that was listed in the authorized_keys file no longer
> > matched what libvirt was actually invoking, so it was rightly
> > rejected.
> 
> I have found some of the issues filed at the time
> 
>   https://bugs.launchpad.net/tripleo/+bug/1918250
>   https://bugzilla.redhat.com/show_bug.cgi?id=1936804
> 
> and the corresponding fix
> 
>   https://github.com/rdo-packages/nova-distgit/commit/d5aba75f3b5589e156afeec915dcfcd4eca4eafe
> 
> The detection logic, as currently implemented, is a bit fragile, but
> updating it so that it keeps working even as we make minor
> adjustments to our ssh tunnel script wasn't particularly difficult
> 
>   https://github.com/andreabolognani/nova-distgit/commit/27cee8da127c1d447cfb694d54c209a94dd804de

It isn't about whether it is difficult or not though. It is showing
that the changes in libvirt are creating a compatibility problem for
existing application code, and that any changes we make here will
require further changes in such applications.  I've not been explicit
about back compatibility in this area, as we didn't realize apps would
be sensitive to changes.  Now we know that, I don't think we should be
knowingly making further changes that are likely to break apps.

> I have posted a new version of this series which cleans up things
> further and actually addresses the original GitLab issue
> 
>   https://listman.redhat.com/archives/libvir-list/2022-February/msg00544.html
> 
> but I will of course not push it until the nova-migration-wrapper
> changes mentioned above have been proposed and accepted, which I
> intend to try next.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|