[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that d
From: |
Alex Bligh |
Subject: |
Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS |
Date: |
Thu, 7 Apr 2016 23:32:21 +0100 |
On 7 Apr 2016, at 21:29, Eric Blake <address@hidden> 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 <address@hidden>
Reviewed-by: Alex Bligh <address@hidden>
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