| Message ID | 20220303160330.2979753-3-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: enable use of TLS on non-TCP transports and other TLS improvements | expand |
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. > @@ -1875,7 +1875,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, > } > > s->conn = nbd_client_connection_new(s->saddr, true, s->export, > - s->x_dirty_bitmap, s->tlscreds); > + s->x_dirty_bitmap, s->tlscreds, > + s->tlshostname); > > if (s->open_timeout) { > nbd_client_connection_enable_retry(s->conn); > 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. Reviewed-by: Eric Blake <eblake@redhat.com>
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
diff --git a/block/nbd.c b/block/nbd.c index 5853d85d60..dd43929207 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -92,7 +92,7 @@ typedef struct BDRVNBDState { SocketAddress *saddr; char *export, *tlscredsid; QCryptoTLSCreds *tlscreds; - const char *hostname; + const char *tlshostname; char *x_dirty_bitmap; bool alloc_depth; @@ -1835,7 +1835,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options, error_setg(errp, "TLS only supported over IP sockets"); goto error; } - s->hostname = s->saddr->u.inet.host; + s->tlshostname = s->saddr->u.inet.host; } s->x_dirty_bitmap = g_strdup(qemu_opt_get(opts, "x-dirty-bitmap")); @@ -1875,7 +1875,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, } s->conn = nbd_client_connection_new(s->saddr, true, s->export, - s->x_dirty_bitmap, s->tlscreds); + s->x_dirty_bitmap, s->tlscreds, + s->tlshostname); if (s->open_timeout) { nbd_client_connection_enable_retry(s->conn); 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); void nbd_client_connection_release(NBDClientConnection *conn); QIOChannel *coroutine_fn diff --git a/nbd/client-connection.c b/nbd/client-connection.c index 2bda42641d..2a632931c3 100644 --- a/nbd/client-connection.c +++ b/nbd/client-connection.c @@ -33,6 +33,7 @@ struct NBDClientConnection { /* Initialization constants, never change */ SocketAddress *saddr; /* address to connect to */ QCryptoTLSCreds *tlscreds; + char *tlshostname; NBDExportInfo initial_info; bool do_negotiation; bool do_retry; @@ -77,7 +78,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) { NBDClientConnection *conn = g_new(NBDClientConnection, 1); @@ -85,6 +87,7 @@ NBDClientConnection *nbd_client_connection_new(const SocketAddress *saddr, *conn = (NBDClientConnection) { .saddr = QAPI_CLONE(SocketAddress, saddr), .tlscreds = tlscreds, + .tlshostname = g_strdup(tlshostname), .do_negotiation = do_negotiation, .initial_info.request_sizes = true, @@ -107,6 +110,7 @@ static void nbd_client_connection_do_free(NBDClientConnection *conn) } error_free(conn->err); qapi_free_SocketAddress(conn->saddr); + g_free(conn->tlshostname); object_unref(OBJECT(conn->tlscreds)); g_free(conn->initial_info.x_dirty_bitmap); g_free(conn->initial_info.name); @@ -120,6 +124,7 @@ static void nbd_client_connection_do_free(NBDClientConnection *conn) */ static int nbd_connect(QIOChannelSocket *sioc, SocketAddress *addr, NBDExportInfo *info, QCryptoTLSCreds *tlscreds, + const char *tlshostname, QIOChannel **outioc, Error **errp) { int ret; @@ -140,7 +145,7 @@ static int nbd_connect(QIOChannelSocket *sioc, SocketAddress *addr, } ret = nbd_receive_negotiate(NULL, QIO_CHANNEL(sioc), tlscreds, - tlscreds ? addr->u.inet.host : NULL, + tlshostname, outioc, info, errp); if (ret < 0) { /* @@ -183,7 +188,8 @@ static void *connect_thread_func(void *opaque) ret = nbd_connect(conn->sioc, conn->saddr, conn->do_negotiation ? &conn->updated_info : NULL, - conn->tlscreds, &conn->ioc, &local_err); + conn->tlscreds, conn->tlshostname, + &conn->ioc, &local_err); /* * conn->updated_info will finally be returned to the user. Clear the
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 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(-)