On 7 Apr 2016, at 21:29, Eric Blake <ebl...@redhat.com> wrote: > Upstream NBD is documenting that servers MAY choose to operate > in a conditional mode, where it is up to the client whether to > use TLS. For qemu's case, we want to always be in FORCEDTLS > mode, because of the risk of man-in-the-middle attacks, and since > we never export more than one device; likewise, the qemu client > will ALWAYS send NBD_OPT_STARTTLS as its first option. But now > that SELECTIVETLS servers exist, it is feasible to encounter a > (non-qemu) client that does not do NBD_OPT_STARTTLS first, but > rather wants to take advantage of the conditional modes it might > find elsewhere. > > Since we require TLS, we are within our rights to drop connections > on any client that doesn't negotiate it right away, or which > attempts to negotiate it incorrectly, without violating the intent > of the NBD Protocol. However, it's better to allow the client to > continue trying, on the grounds that maybe the client will get the > hint to send NBD_OPT_STARTTLS. > > Signed-off-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alex Bligh <a...@alex.org.uk> Looks right to me - untested. > --- > > My earlier patch was arguably incomplete: > https://lists.gnu.org/archive/html/qemu-devel/2016-04/msg01265.html > > But as it is already in a pull request, and as this one is > a bit more controversial, it's best to keep it as a separate patch. > > nbd/server.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index 7843584..2b727f0 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -450,9 +450,12 @@ static int nbd_negotiate_options(NBDClient *client) > > default: > TRACE("Option 0x%x not permitted before TLS", clientflags); > + if (nbd_negotiate_drop_sync(client->ioc, length) != length) { > + return -EIO; > + } > nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_TLS_REQD, > clientflags); > - return -EINVAL; > + break; > } > } else if (fixedNewstyle) { > switch (clientflags) { > @@ -470,6 +473,9 @@ static int nbd_negotiate_options(NBDClient *client) > return nbd_negotiate_handle_export_name(client, length); > > case NBD_OPT_STARTTLS: > + if (nbd_negotiate_drop_sync(client->ioc, length) != length) { > + return -EIO; > + } > if (client->tlscreds) { > TRACE("TLS already enabled"); > nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_INVALID, > @@ -479,7 +485,7 @@ static int nbd_negotiate_options(NBDClient *client) > nbd_negotiate_send_rep(client->ioc, NBD_REP_ERR_POLICY, > clientflags); > } > - return -EINVAL; > + break; > default: > TRACE("Unsupported option 0x%x", clientflags); > if (nbd_negotiate_drop_sync(client->ioc, length) != length) { > -- > 2.5.5 > > -- Alex Bligh
