* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS
2016-04-07 20:29 [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS Eric Blake
@ 2016-04-07 22:32 ` Alex Bligh
2016-04-14 15:25 ` Eric Blake
2016-04-14 21:08 ` Max Reitz
2 siblings, 0 replies; 6+ messages in thread
From: Alex Bligh @ 2016-04-07 22:32 UTC (permalink / raw)
To: Eric Blake; +Cc: Alex Bligh, qemu-devel@nongnu.org, Paolo Bonzini
On 7 Apr 2016, at 21:29, Eric Blake <eblake@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 <eblake@redhat.com>
Reviewed-by: Alex Bligh <alex@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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS
2016-04-07 20:29 [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS Eric Blake
2016-04-07 22:32 ` Alex Bligh
@ 2016-04-14 15:25 ` Eric Blake
2016-04-14 15:43 ` Alex Bligh
2016-04-14 21:08 ` Max Reitz
2 siblings, 1 reply; 6+ messages in thread
From: Eric Blake @ 2016-04-14 15:25 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, alex, qemu block
[-- Attachment #1: Type: text/plain, Size: 3365 bytes --]
[adding qemu-block in cc, since Paolo can't send pull request]
On 04/07/2016 02:29 PM, Eric Blake 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 <eblake@redhat.com>
> ---
>
> 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) {
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS
2016-04-14 15:25 ` Eric Blake
@ 2016-04-14 15:43 ` Alex Bligh
0 siblings, 0 replies; 6+ messages in thread
From: Alex Bligh @ 2016-04-14 15:43 UTC (permalink / raw)
To: Eric Blake; +Cc: Alex Bligh, qemu-devel@nongnu.org, Paolo Bonzini, qemu block
[-- Attachment #1: Type: text/plain, Size: 4089 bytes --]
On 14 Apr 2016, at 16:25, Eric Blake <eblake@redhat.com> wrote:
> [adding qemu-block in cc, since Paolo can't send pull request]
>
> On 04/07/2016 02:29 PM, Eric Blake 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 <eblake@redhat.com>
Reviewed-by: Alex Bligh <alex@alex.org.uk>
It is worth noting that this change (assuming I've read
it right) in no way means that qemu would be serving resources
without TLS enabled when configured with TLS, or even
providing information about those resources. It's
simply saying "TLS is required" (and not dropping the
connection so the client can ask for TLS) if the client
doesn't ask for TLS first thing. This removes the need
for the client and server to communicate out of band
that TLS is required (as well as conforming to the spec).
Alex
>> ---
>>
>> 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) {
>>
>
> --
> Eric Blake eblake redhat com +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
--
Alex Bligh
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS
2016-04-07 20:29 [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS Eric Blake
2016-04-07 22:32 ` Alex Bligh
2016-04-14 15:25 ` Eric Blake
@ 2016-04-14 21:08 ` Max Reitz
2016-04-14 21:46 ` Eric Blake
2 siblings, 1 reply; 6+ messages in thread
From: Max Reitz @ 2016-04-14 21:08 UTC (permalink / raw)
To: Eric Blake, qemu-devel; +Cc: pbonzini, alex
[-- Attachment #1.1: Type: text/plain, Size: 3416 bytes --]
On 07.04.2016 22:29, Eric Blake 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 <eblake@redhat.com>
> ---
>
> 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;
> }
What about NBD_OPT_EXPORTNAME? The specification says that this option
does not allow for errors, and so the session must be terminated if this
option is sent in FORCEDTLS mode without TLS having been negotiated.
Max
> } 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) {
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't kill server on client that doesn't request TLS
2016-04-14 21:08 ` Max Reitz
@ 2016-04-14 21:46 ` Eric Blake
0 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2016-04-14 21:46 UTC (permalink / raw)
To: Max Reitz, qemu-devel; +Cc: pbonzini, alex
[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]
On 04/14/2016 03:08 PM, Max Reitz wrote:
> On 07.04.2016 22:29, Eric Blake 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 <eblake@redhat.com>
>> ---
>> +++ 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;
>> }
>
> What about NBD_OPT_EXPORTNAME? The specification says that this option
> does not allow for errors, and so the session must be terminated if this
> option is sent in FORCEDTLS mode without TLS having been negotiated.
Oh, good catch. v2 coming up.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread