Re: [PATCH 02/12] block: pass desired TLS hostname through from block driver client

[Daniel P . Berrangé]

On Thu, Mar 03, 2022 at 02:14:34PM -0600, Eric Blake wrote:
> On Thu, Mar 03, 2022 at 04:03:20PM +0000, Daniel P. Berrangé wrote:
> > In
> > 
> >   commit a71d597b989fd701b923f09b3c20ac4fcaa55e81
> >   Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >   Date:   Thu Jun 10 13:08:00 2021 +0300
> > 
> >     block/nbd: reuse nbd_co_do_establish_connection() in nbd_open()
> > 
> > the use of the 'hostname' field from the BDRVNBDState struct was
> > lost, and 'nbd_connect' just hardcoded it to match the IP socket
> > address. This was a harmless bug at the time since we block use
> > with anything other than IP sockets.
> > 
> > Shortly though, We want to allow the caller to override the hostname
> 
> s/We/we/
> 
> > used in the TLS certificate checks. This is to allow for TLS
> > when doing port forwarding or tunneling. Thus we need to reinstate
> > the passing along of the 'hostname'.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  block/nbd.c             |  7 ++++---
> >  include/block/nbd.h     |  3 ++-
> >  nbd/client-connection.c | 12 +++++++++---
> >  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> Nice - this a great step towards fixing a longstanding annoyance of
> mine that libnbd and nbdkit support TLS over Unix sockets, but qemu
> didn't.


> > diff --git a/include/block/nbd.h b/include/block/nbd.h
> > index 78d101b774..a98eb665da 100644
> > --- a/include/block/nbd.h
> > +++ b/include/block/nbd.h
> > @@ -415,7 +415,8 @@ NBDClientConnection *nbd_client_connection_new(const 
> > SocketAddress *saddr,
> >                                                 bool do_negotiation,
> >                                                 const char *export_name,
> >                                                 const char *x_dirty_bitmap,
> > -                                               QCryptoTLSCreds *tlscreds);
> > +                                               QCryptoTLSCreds *tlscreds,
> > +                                               const char *tlshostname);
> 
> We already have a lot of parameters; does it make sense to bundle
> tlshostname into the QCryptoTLSCreds struct at all?  But that would
> change the QAPI (or maybe you do it later in the series), it is not a
> show-stopper to this patch.

The credentials object is something that can be used for multiple
connections. The TLS hostname override meanwhile is specific to
a single connection. Thus it would not be appropriate to store the
TLS hostname in the credentials struct.


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 :|