| Message ID | 1453208963-16834-10-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 19/01/2016 14:09, Daniel P. Berrange wrote: > The NBD client is currently only capable of using the new style > protocol negotiation if an explicit export name is provided. > This is a problem, because TLS support will require use of the > new style protocol in all cases, and we wish to keep the export > name as an optional request for backwards compatibility. > > The trivial way to deal with this is to use the NBD protocol > option for listing exports and then pick the first returned > export as the one to use. This makes the client "do the right > thing" in the normal case where the qemu-nbd server only > exports a single volume. > > Furthermore, in order to get proper error reporting of unknown > options with fixed new style protocol, we must not send the > NBD_OPT_EXPORT_NAME as the first thing. Thus, even if an export > name is provided we still send NBD_OPT_LIST to enumerate server > exports. This also gives us clearer error reporting in the case > that the requested export name does not exist. If NBD_OPT_LIST > is not supported, we still fallback to using the specified > export name, so as not to break compatibility with old QEMU > NBD server impls that predated NBD_OPT_LIST > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> As a first reaction, I would really avoid magic unless the server provides a single exports. But even in that case, I would prefer to have some synchronization between the server and client command-line. Is an empty NBD_OPT_EXPORT_NAME valid? What about using new-style negotiation with empty NBD_OPT_EXPORT_NAME if TLS is requested? Paolo
On Tue, Jan 19, 2016 at 05:14:32PM +0100, Paolo Bonzini wrote: > > > On 19/01/2016 14:09, Daniel P. Berrange wrote: > > The NBD client is currently only capable of using the new style > > protocol negotiation if an explicit export name is provided. > > This is a problem, because TLS support will require use of the > > new style protocol in all cases, and we wish to keep the export > > name as an optional request for backwards compatibility. > > > > The trivial way to deal with this is to use the NBD protocol > > option for listing exports and then pick the first returned > > export as the one to use. This makes the client "do the right > > thing" in the normal case where the qemu-nbd server only > > exports a single volume. > > > > Furthermore, in order to get proper error reporting of unknown > > options with fixed new style protocol, we must not send the > > NBD_OPT_EXPORT_NAME as the first thing. Thus, even if an export > > name is provided we still send NBD_OPT_LIST to enumerate server > > exports. This also gives us clearer error reporting in the case > > that the requested export name does not exist. If NBD_OPT_LIST > > is not supported, we still fallback to using the specified > > export name, so as not to break compatibility with old QEMU > > NBD server impls that predated NBD_OPT_LIST > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > As a first reaction, I would really avoid magic unless the server > provides a single exports. But even in that case, I would prefer to > have some synchronization between the server and client command-line. > > Is an empty NBD_OPT_EXPORT_NAME valid? What about using new-style > negotiation with empty NBD_OPT_EXPORT_NAME if TLS is requested? The main goal here is to ensure the NBD client gets a decent error message if it tries to connect without TLS. Even if we are using the fixed new style protocol, the client code will send NBD_OPT_EXPORT_NAME as the first thing it does. Thanks to a bit of crazyness is the NBD protocol spec, the server is unable to reply with an error message to NBD_OPT_EXPORT_NAME. So if the client connected to a server reqiuring TLS and does not request TLS enablement, the server will have no choice but to just close the connection with no error. I think this will be pretty nasty for users trying to debug problems with TLS. The NBD_OPT_LIST option is the only option available which we can unconditionally invoke from the client which will get us a clear response from the server that TLS is required. So AFAICT we have no choice but to use that if we want decent error reporting. Regards, Daniel
On 19/01/2016 17:44, Daniel P. Berrange wrote: >> > As a first reaction, I would really avoid magic unless the server >> > provides a single exports. But even in that case, I would prefer to >> > have some synchronization between the server and client command-line. >> > >> > Is an empty NBD_OPT_EXPORT_NAME valid? What about using new-style >> > negotiation with empty NBD_OPT_EXPORT_NAME if TLS is requested? > The main goal here is to ensure the NBD client gets a decent error > message if it tries to connect without TLS. Even if we are using > the fixed new style protocol, the client code will send > NBD_OPT_EXPORT_NAME as the first thing it does. Thanks to a bit of > crazyness is the NBD protocol spec, the server is unable to reply > with an error message to NBD_OPT_EXPORT_NAME. > > So if the client connected to a server reqiuring TLS and does not > request TLS enablement, the server will have no choice but to just > close the connection with no error. I think this will be pretty > nasty for users trying to debug problems with TLS. That's fine. I'm just not sold on using the first answer from NBD_OPT_LIST as the argument to the subsequent NBD_OPT_EXPORT_NAME. In other words, I would prefer to do the following for no export name: 1) server, no TLS: accept either old-style negotiation or new-style negotation with an empty ("") export name; NBD_OPT_LIST returns a single export name, "". 2) server, TLS: accept only new-style negotiation with an empty ("") export name; NBD_OPT_LIST returns a single export name, "". 3) client, no TLS: use old-style negotiation; if the server rejects old-style negotiation, mention the possibility that the server requires TLS 4) client, TLS: use new-style negotiation with an empty ("") export name. The only interesting case for named exports is client, no TLS. Then you can just send a dummy NBD_OPT_LIST unconditionally, and use the result to provide a good error message if the server requires TLS. If it makes the code simpler to use NBD_OPT_LIST always, even if the client supports TLS (making the sequence NBD_OPT_STARTTLS, NBD_OPT_LIST, NBD_OPT_EXPOR_NAME), then that's fine too. Paolo
On Thu, Jan 21, 2016 at 11:30:35AM +0100, Paolo Bonzini wrote: > > > On 19/01/2016 17:44, Daniel P. Berrange wrote: > >> > As a first reaction, I would really avoid magic unless the server > >> > provides a single exports. But even in that case, I would prefer to > >> > have some synchronization between the server and client command-line. > >> > > >> > Is an empty NBD_OPT_EXPORT_NAME valid? What about using new-style > >> > negotiation with empty NBD_OPT_EXPORT_NAME if TLS is requested? > > The main goal here is to ensure the NBD client gets a decent error > > message if it tries to connect without TLS. Even if we are using > > the fixed new style protocol, the client code will send > > NBD_OPT_EXPORT_NAME as the first thing it does. Thanks to a bit of > > crazyness is the NBD protocol spec, the server is unable to reply > > with an error message to NBD_OPT_EXPORT_NAME. > > > > So if the client connected to a server reqiuring TLS and does not > > request TLS enablement, the server will have no choice but to just > > close the connection with no error. I think this will be pretty > > nasty for users trying to debug problems with TLS. > > That's fine. I'm just not sold on using the first answer from > NBD_OPT_LIST as the argument to the subsequent NBD_OPT_EXPORT_NAME. > > In other words, I would prefer to do the following for no export name: > > 1) server, no TLS: accept either old-style negotiation or new-style > negotation with an empty ("") export name; NBD_OPT_LIST returns a single > export name, "". > > 2) server, TLS: accept only new-style negotiation with an empty ("") > export name; NBD_OPT_LIST returns a single export name, "". > > 3) client, no TLS: use old-style negotiation; if the server rejects > old-style negotiation, mention the possibility that the server requires TLS > > 4) client, TLS: use new-style negotiation with an empty ("") export name. > > The only interesting case for named exports is client, no TLS. Then you > can just send a dummy NBD_OPT_LIST unconditionally, and use the result > to provide a good error message if the server requires TLS. If it makes > the code simpler to use NBD_OPT_LIST always, even if the client supports > TLS (making the sequence NBD_OPT_STARTTLS, NBD_OPT_LIST, > NBD_OPT_EXPOR_NAME), then that's fine too. Ok, I'll have a go at this approach Regards, Daniel
diff --git a/nbd/client.c b/nbd/client.c index 22d69ff..3c245a5 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -70,12 +70,187 @@ static QTAILQ_HEAD(, NBDExport) exports = QTAILQ_HEAD_INITIALIZER(exports); */ + +static int nbd_handle_reply_err(uint32_t opt, uint32_t type, Error **errp) +{ + if (!(type & (1 << 31))) { + return 0; + } + + switch (type) { + case NBD_REP_ERR_UNSUP: + error_setg(errp, "Unsupported option type %x", opt); + break; + + case NBD_REP_ERR_INVALID: + error_setg(errp, "Invalid data length for option %x", opt); + break; + + default: + error_setg(errp, "Unknown error code when asking for option %x", opt); + break; + } + + return -1; +} + +static int nbd_receive_list(QIOChannel *ioc, char **name, Error **errp) +{ + uint64_t magic; + uint32_t opt; + uint32_t type; + uint32_t len; + uint32_t namelen; + + *name = NULL; + if (read_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) { + error_setg(errp, "failed to read list option magic"); + return -1; + } + magic = be64_to_cpu(magic); + if (magic != NBD_REP_MAGIC) { + error_setg(errp, "Unexpected option list magic"); + return -1; + } + if (read_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) { + error_setg(errp, "failed to read list option"); + return -1; + } + opt = be32_to_cpu(opt); + if (opt != NBD_OPT_LIST) { + error_setg(errp, "Unexpected option type %x expected %x", + opt, NBD_OPT_LIST); + return -1; + } + + if (read_sync(ioc, &type, sizeof(type)) != sizeof(type)) { + error_setg(errp, "failed to read list option type"); + return -1; + } + type = be32_to_cpu(type); + if (type == NBD_REP_ERR_UNSUP) { + return 0; + } + if (nbd_handle_reply_err(opt, type, errp) < 0) { + return -1; + } + + if (read_sync(ioc, &len, sizeof(len)) != sizeof(len)) { + error_setg(errp, "failed to read option length"); + return -1; + } + len = be32_to_cpu(len); + + if (type == NBD_REP_ACK) { + if (len != 0) { + error_setg(errp, "length too long for option end"); + return -1; + } + } else if (type == NBD_REP_SERVER) { + if (read_sync(ioc, &namelen, sizeof(namelen)) != sizeof(namelen)) { + error_setg(errp, "failed to read option name length"); + return -1; + } + namelen = be32_to_cpu(namelen); + if (len != (namelen + sizeof(namelen))) { + error_setg(errp, "incorrect option mame length"); + return -1; + } + if (namelen > 255) { + error_setg(errp, "export name length too long %d", namelen); + return -1; + } + + *name = g_new0(char, namelen + 1); + if (read_sync(ioc, *name, namelen) != namelen) { + error_setg(errp, "failed to read export name"); + g_free(*name); + *name = NULL; + return -1; + } + (*name)[namelen] = '\0'; + } else { + error_setg(errp, "Unexpected reply type %x expected %x", + type, NBD_REP_SERVER); + return -1; + } + return 1; +} + + +static char *nbd_receive_pick_export(QIOChannel *ioc, + const char *wantname, + Error **errp) +{ + uint64_t magic = cpu_to_be64(NBD_OPTS_MAGIC); + uint32_t opt = cpu_to_be32(NBD_OPT_LIST); + uint32_t length = 0; + char *chosenname = NULL; + + TRACE("Picking export name"); + if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) { + error_setg(errp, "Failed to send list option magic"); + goto fail; + } + + if (write_sync(ioc, &opt, sizeof(opt)) != sizeof(opt)) { + error_setg(errp, "Failed to send list option number"); + goto fail; + } + + if (write_sync(ioc, &length, sizeof(length)) != sizeof(length)) { + error_setg(errp, "Failed to send list option length"); + goto fail; + } + + TRACE("Reading available export names"); + while (1) { + char *name = NULL; + int ret = nbd_receive_list(ioc, &name, errp); + + if (ret < 0) { + g_free(name); + name = NULL; + goto fail; + } + if (ret == 0) { /* Server doesn't support export listing */ + if (wantname == NULL) { + error_setg(errp, "Export name is required"); + goto fail; + } + chosenname = g_strdup(wantname); + break; + } + if (name == NULL) { + TRACE("End of export name list"); + break; + } + if (!chosenname && (!wantname || g_str_equal(name, wantname))) { + chosenname = name; + TRACE("Picked export name %s", name); + } else { + TRACE("Ignored export name %s", name); + g_free(name); + } + } + + if (wantname && !chosenname) { + error_setg(errp, "No export with name %s available", wantname); + goto fail; + } + + fail: + TRACE("Chosen export name %s", chosenname ? chosenname : "<null>"); + return chosenname; +} + int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, off_t *size, Error **errp) { char buf[256]; uint64_t magic, s; int rc; + char *autoname = NULL; TRACE("Receiving negotiation."); @@ -120,27 +295,40 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, uint32_t namesize; uint16_t globalflags; uint16_t exportflags; + bool fixedNewStyle = false; if (read_sync(ioc, &globalflags, sizeof(globalflags)) != sizeof(globalflags)) { error_setg(errp, "Failed to read server flags"); goto fail; } - *flags = be16_to_cpu(globalflags) << 16; + globalflags = be16_to_cpu(globalflags); + *flags = globalflags << 16; + TRACE("Global flags are %x", globalflags); if (globalflags & NBD_FLAG_FIXED_NEWSTYLE) { + fixedNewStyle = true; TRACE("Server supports fixed new style"); clientflags |= NBD_FLAG_C_FIXED_NEWSTYLE; } /* client requested flags */ + clientflags = cpu_to_be32(clientflags); if (write_sync(ioc, &clientflags, sizeof(clientflags)) != sizeof(clientflags)) { error_setg(errp, "Failed to send clientflags field"); goto fail; } /* write the export name */ - if (!name) { - error_setg(errp, "Server requires an export name"); - goto fail; + if (fixedNewStyle) { + autoname = nbd_receive_pick_export(ioc, name, errp); + if (!autoname) { + goto fail; + } + name = autoname; + } else { + if (!name) { + error_setg(errp, "Server requires an export name"); + goto fail; + } } magic = cpu_to_be64(magic); if (write_sync(ioc, &magic, sizeof(magic)) != sizeof(magic)) { @@ -175,7 +363,9 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, error_setg(errp, "Failed to read export flags"); goto fail; } - *flags |= be16_to_cpu(exportflags); + exportflags = be16_to_cpu(exportflags); + *flags |= exportflags; + TRACE("Export flags are %x", exportflags); } else if (magic == NBD_CLIENT_MAGIC) { if (name) { error_setg(errp, "Server does not support export names"); @@ -206,6 +396,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, const char *name, uint32_t *flags, rc = 0; fail: + free(autoname); return rc; } diff --git a/nbd/server.c b/nbd/server.c index 8343a09..5296808 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -291,6 +291,8 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t length) } name[length] = '\0'; + TRACE("Client requested export '%s'", name); + client->exp = nbd_export_find(name); if (!client->exp) { LOG("export not found");
The NBD client is currently only capable of using the new style protocol negotiation if an explicit export name is provided. This is a problem, because TLS support will require use of the new style protocol in all cases, and we wish to keep the export name as an optional request for backwards compatibility. The trivial way to deal with this is to use the NBD protocol option for listing exports and then pick the first returned export as the one to use. This makes the client "do the right thing" in the normal case where the qemu-nbd server only exports a single volume. Furthermore, in order to get proper error reporting of unknown options with fixed new style protocol, we must not send the NBD_OPT_EXPORT_NAME as the first thing. Thus, even if an export name is provided we still send NBD_OPT_LIST to enumerate server exports. This also gives us clearer error reporting in the case that the requested export name does not exist. If NBD_OPT_LIST is not supported, we still fallback to using the specified export name, so as not to break compatibility with old QEMU NBD server impls that predated NBD_OPT_LIST Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- nbd/client.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- nbd/server.c | 2 + 2 files changed, 198 insertions(+), 5 deletions(-)