Re: [Qemu-devel] [PATCHv3] Improve documentation for TLS

Re: [Qemu-devel] [PATCHv3] Improve documentation for TLS

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4943 bytes --]

On 04/07/2016 12:32 PM, Alex Bligh wrote:
> * Call out TLS into a separate section
> 
> * Add details of the TLS protocol itself
> 
> * Emphasise that actual TLS session initiation (i.e. the TLS handshake) can
>   be initiated from either side (as required by the TLS standard I believe
>   and as actually works in practice)
> 
> * Clarify what is a requirement on servers, and what is a requirement on
>   clients, separately, specifying their behaviour in a single place
>   in the document.
> 
> * Document the four possible modes of operation of a server.
> 
> Signed-off-by: Alex Bligh <alex@alex.org.uk>
> ---
>  doc/proto.md | 341 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 307 insertions(+), 34 deletions(-)
> 


> +
> +If a client supports TLS, it SHOULD also support the INFO
> +extension, and SHOULD use `NBD_OPT_GO` if available in place
> +of `NBD_OPT_EXPORT_NAME`. The reason for this is set out in
> +the final paragraphs of the sections under 'FORCEDTLS'
> +and 'SELECTIVETLS': this gives an opportunity for the
> +server to transmit that an error going into transmission
> +mode is due to the client's failure to initiate TLS,
> +and the fact that the client may obtain information about
> +which exports are TLS-only through `NBD_OPT_INFO`.

Side note (no change needed to your text):

Qemu's initial implementation of TLS in the client is binary (you either
want TLS or plaintext; there's no way to connect to a server and then
decide whether to upgrade to TLS - a plaintext client will never use TLS
of an OPTIONALTLS server).  In TLS mode, the client always sends
NBD_OPT_STARTTLS first (and gets a sane error message if the server
can't/won't use TLS).  In plaintext mode, the client always sends
NBD_OPT_LIST first - which will get a nice NBD_OPT_ERR_TLS_REQD from a
FORCEDTLS server, but does NOT error out for a SELECTIVETLS server.  But
if it DOES get a listing, it then checks that the desired export was
present in the listing, which means a SELECTIVETLS server that requires
TLS on the particular export the client was hoping for will cause the
client to fail with a graceful error message of "export not present"
generated by the client, rather than attempting NBD_OPT_EXPORT_NAME, so
long as the SELECTIVETLS server omitted the TLS-only export in its
listing.  Only for a really old server (such as non-fixed newstyle),
where NBD_OPT_LIST cannot be attempted or fails with NBD_OPT_ERR_UNSUP,
is the client still in the dark about whether TLS is required, but in
that case, the server probably doesn't support TLS (especially since TLS
requires fixed newstyle).

But the reason I see no need to modify your text: I'm planning on
changing qemu's plaintext client to try NBD_OPT_GO rather than
NBD_OPT_LIST.  For a FORCEDTLS server, the command will fail with
NBD_REP_ERR_TLS_REQD (whether or not the server knows NBD_OPT_GO); and
for a SELECTIVETLS server, we just mandated that NBD_OPT_GO must be
recognized, so it will fail with NBD_REP_ERR_TLS_REQD for a TLS-only
export, and will succeed (in plaintext) otherwise.  Which means there is
no longer a need to fall back to NBD_OPT_LIST.


> +
> +With regard to the second, any server that does not wish
> +to be subject to a potential downgrade attack SHOULD either
> +used FORCEDTLS mode, or should force TLS on those exports
> +it is concerned about using SELECTIVE mode and TLS-only
> +exports. It is not possible to avoid downgrade attacks
> +on exports which may be served either via TLS or in plain
> +text unless the client insists on TLS. OPTIONALTLS SHOULD NOT
> +be used where man-in-the-midle attacks are a concern.

s/midle/middle/


> @@ -391,7 +679,10 @@ of the newstyle negotiation.
>  - `NBD_OPT_LIST` (3)
>  
>      Return a number of `NBD_REP_SERVER` replies, one for each export,
> -    followed by `NBD_REP_ACK`.
> +    followed by `NBD_REP_ACK`. The server MAY omit entries from this
> +    list if TLS has not been negotiated and either the server is
> +    operating in FORCEDTLS mode or the server is operating in
> +    SELECTIVETLS mode and the entry concerned is a TLS-only export.

Not quite right - in FORCEDTLS mode, the server MUST reply with
NBD_REP_ERR_TLS_REQD.  Correct would be:

The server MAY omit entries from this list if TLS has not been
negotiated and the server is operating in SELECTIVETLS mode, where the
entry concerned is a TLS-only export.

Maybe even strengthen it to SHOULD, particularly given my above side
note about qemu's usage of NBD_OPT_LIST to determine if a plaintext
client is talking to a server that wants TLS.

I'm down to just 2 findings and a side comment, which means we're close
enough that you can add:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 --]

Re: [Qemu-devel] [PATCHv3] Improve documentation for TLS

[Alex Bligh]

[-- Attachment #1: Type: text/plain, Size: 4038 bytes --]

Eric,

> Qemu's initial implementation of TLS in the client is binary (you either
> want TLS or plaintext; there's no way to connect to a server and then
> decide whether to upgrade to TLS - a plaintext client will never use TLS
> of an OPTIONALTLS server).  In TLS mode, the client always sends
> NBD_OPT_STARTTLS first (and gets a sane error message if the server
> can't/won't use TLS).  In plaintext mode, the client always sends
> NBD_OPT_LIST first - which will get a nice NBD_OPT_ERR_TLS_REQD from a
> FORCEDTLS server, but does NOT error out for a SELECTIVETLS server.

... assuming the export is non-TLS.

>  But
> if it DOES get a listing, it then checks that the desired export was
> present in the listing, which means a SELECTIVETLS server that requires
> TLS on the particular export the client was hoping for will cause the
> client to fail with a graceful error message of "export not present"
> generated by the client, rather than attempting NBD_OPT_EXPORT_NAME, so
> long as the SELECTIVETLS server omitted the TLS-only export in its
> listing.  Only for a really old server (such as non-fixed newstyle),
> where NBD_OPT_LIST cannot be attempted or fails with NBD_OPT_ERR_UNSUP,
> is the client still in the dark about whether TLS is required, but in
> that case, the server probably doesn't support TLS (especially since TLS
> requires fixed newstyle).

Yes.

> But the reason I see no need to modify your text: I'm planning on
> changing qemu's plaintext client to try NBD_OPT_GO rather than
> NBD_OPT_LIST.  For a FORCEDTLS server, the command will fail with
> NBD_REP_ERR_TLS_REQD (whether or not the server knows NBD_OPT_GO); and
> for a SELECTIVETLS server, we just mandated that NBD_OPT_GO must be
> recognized, so it will fail with NBD_REP_ERR_TLS_REQD for a TLS-only
> export, and will succeed (in plaintext) otherwise.  Which means there is
> no longer a need to fall back to NBD_OPT_LIST.

Which is better, as support for INFO is required for TLS
anyway, whereas LIST is optional.

>> +
>> +With regard to the second, any server that does not wish
>> +to be subject to a potential downgrade attack SHOULD either
>> +used FORCEDTLS mode, or should force TLS on those exports
>> +it is concerned about using SELECTIVE mode and TLS-only
>> +exports. It is not possible to avoid downgrade attacks
>> +on exports which may be served either via TLS or in plain
>> +text unless the client insists on TLS. OPTIONALTLS SHOULD NOT
>> +be used where man-in-the-midle attacks are a concern.
> 
> s/midle/middle/

thx

>> @@ -391,7 +679,10 @@ of the newstyle negotiation.
>> - `NBD_OPT_LIST` (3)
>> 
>>     Return a number of `NBD_REP_SERVER` replies, one for each export,
>> -    followed by `NBD_REP_ACK`.
>> +    followed by `NBD_REP_ACK`. The server MAY omit entries from this
>> +    list if TLS has not been negotiated and either the server is
>> +    operating in FORCEDTLS mode or the server is operating in
>> +    SELECTIVETLS mode and the entry concerned is a TLS-only export.
> 
> Not quite right - in FORCEDTLS mode, the server MUST reply with
> NBD_REP_ERR_TLS_REQD.  Correct would be:
> 
> The server MAY omit entries from this list if TLS has not been
> negotiated and the server is operating in SELECTIVETLS mode, where the
> entry concerned is a TLS-only export.
> 
> Maybe even strengthen it to SHOULD, particularly given my above side
> note about qemu's usage of NBD_OPT_LIST to determine if a plaintext
> client is talking to a server that wants TLS.

I went with SHOULD. Note the requirement to reply with NBD_REP_ERR_TLS_REQD
in FORCEDTLS mode if TLS has not been negotiated is documented elsewhere
and of course applies to all options other then NBD_OPT_STARTTLS.

> I'm down to just 2 findings and a side comment, which means we're close
> enough that you can add:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks - will send another version anyway.

Alex



> --
> 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 --]

Re: [Qemu-devel] [Nbd] [PATCHv3] Improve documentation for TLS

[Wouter Verhelst]

On Thu, Apr 07, 2016 at 07:32:47PM +0100, Alex Bligh wrote:
[...]
> +### Server-side requirements
> +
> +There are four modes of operation for a server. The
> +server MUST support one of these modes.
> +
> +* The server operates entirely without TLS ('NOTLS'); OR
> +
> +* The server makes TLS available (for all exports) and
> +  it is available at the option of the client ('OPTIONALTLS'); OR
> +
> +* The server insists upon TLS, and forces the client to
> +  upgrade by erroring any NBD options other than `NBD_OPT_STARTTLS`
> +  with `NBD_REP_ERR_TLS_REQD` ('FORCEDTLS'); this in practice means
> +  that all option negotiation (apart from the `NBD_OPT_STARTTLS`
> +  itself) is carried out with TLS; OR
> +
> +* The server provides TLS, and it is mandatory on zero or more

Since you say zero here, how is it different from OPTIONALTLS?

If "not at all", just drop optional.

[...]
> +#### OPTIONALTLS mode
> +
> +If the server receives `NBD_OPT_STARTTLS` it MUST reply with
> +`NBD_REP_ACK`. After this reply has been sent, the server MUST
> +be prepared for a TLS handshake, and all further data MUST
> +be sent and received over TLS. There is no downgrade to a
> +non-TLS connection.
> +
> +As per the TLS standard, the handshake MAY be initiated either
> +by the server (having sent the `NBD_REP_ACK`) or by the client.

I'm not *that* well versed in the details of TLS, but isn't it better to
specify which side should go first?

[...]
> +If the server receives any other option, including `NBD_OPT_INFO`
> +and unsupported options, it MUST reply with `NBD_REP_ERR_TLS_REQD`
> +if TLS has not been initiated; `NBD_OPT_INFO` is included as in this
> +mode, all exports are TLS-only. If the server receives a request to
> +enter transmission mode via `NBD_OPT_EXPORT_NAME` when TLS has not
> +been initiated, then as this request cannot error, it MUST
> +disconnect the connection. If the server receives a request to

s/disconnect/terminate/ reads slightly better.

[...]
> +The client MUST NOT issue `NBD_OPT_STARTTLS` unless the server
> +set flag NBD_FLAG_FIXED_NEWSTYLE and the client replied
> +with NBD_FLAG_C_FIXED_NEWSTYLE in the fixed newstyle
> +negotiation.

Why not "unless fixed newstyle negotiation is in effect"? No need to
repeat that definition.

[...]
> +### Security considerations
> +
> +#### TLS versions
> +
> +NBD implementations supporting TLS MUST support TLS version 1.2,
> +SHOULD support any later versions, and MAY support older versions.

I would prefer "SHOULD NOT allow TLS versions older than 1.2" here.
There are some serious flaws in older TLS versions; currently these are
still supported by most web browsers for backwards compatibility
reasons, but that does not apply for us.

[...]

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Qemu-devel] [Nbd] [PATCHv3] Improve documentation for TLS

[Alex Bligh]

On 9 Apr 2016, at 11:11, Wouter Verhelst <w@uter.be> wrote:
> Since you say zero here, how is it different from OPTIONALTLS?
> 
> If "not at all", just drop optional.

As per previous message, because SELECTIVETLS requires INFO,
but OPTIONALTLS doesn't.

> I'm not *that* well versed in the details of TLS, but isn't it better to
> specify which side should go first?

I believe it's a design feature that you need not. Essentially both
parties start in a 'no handshake has taken place' state, and on the
first read or write from either end, one party starts the handshake
(and there is a provision in case they collide). Alternatively
either end can explicitly request the handshake.

There is actually an implementation advantage to the server doing it
(having just written it) which is that the server can then capture
any error (invalid credentials or whatever) and report it with whatever
logging it does for the STARTTLS option; otherwise invalid certificate
responses come with the next option it receives.

> [...]
>> +If the server receives any other option, including `NBD_OPT_INFO`
>> +and unsupported options, it MUST reply with `NBD_REP_ERR_TLS_REQD`
>> +if TLS has not been initiated; `NBD_OPT_INFO` is included as in this
>> +mode, all exports are TLS-only. If the server receives a request to
>> +enter transmission mode via `NBD_OPT_EXPORT_NAME` when TLS has not
>> +been initiated, then as this request cannot error, it MUST
>> +disconnect the connection. If the server receives a request to
> 
> s/disconnect/terminate/ reads slightly better.

OK

> [...]
>> +The client MUST NOT issue `NBD_OPT_STARTTLS` unless the server
>> +set flag NBD_FLAG_FIXED_NEWSTYLE and the client replied
>> +with NBD_FLAG_C_FIXED_NEWSTYLE in the fixed newstyle
>> +negotiation.
> 
> Why not "unless fixed newstyle negotiation is in effect"? No need to
> repeat that definition.

Can do; I just wanted to be explicit that both server and client
must support it.

> 
> [...]
>> +### Security considerations
>> +
>> +#### TLS versions
>> +
>> +NBD implementations supporting TLS MUST support TLS version 1.2,
>> +SHOULD support any later versions, and MAY support older versions.
> 
> I would prefer "SHOULD NOT allow TLS versions older than 1.2" here.
> There are some serious flaws in older TLS versions; currently these are
> still supported by most web browsers for backwards compatibility
> reasons, but that does not apply for us.

I'd be all for that. Or certainly "SHOULD NOT support LS versions older
than 1.2 by default" (gonbdserver allows you to configure minimum and
maximum TLS versions).

-- 
Alex Bligh

Re: [Qemu-devel] [Nbd] [PATCHv3] Improve documentation for TLS

[Wouter Verhelst]

On Sat, Apr 09, 2016 at 11:26:23AM +0100, Alex Bligh wrote:
> 
> On 9 Apr 2016, at 11:11, Wouter Verhelst <w@uter.be> wrote:
> > Since you say zero here, how is it different from OPTIONALTLS?
> > 
> > If "not at all", just drop optional.
> 
> As per previous message, because SELECTIVETLS requires INFO,
> but OPTIONALTLS doesn't.

Um. So you're suggesting that if a client sends INFO, we're suddenly in
a whole different mode of operation?

That seems to make little sense (other than "complicate matters for no
particularly good reason")

> > I'm not *that* well versed in the details of TLS, but isn't it better to
> > specify which side should go first?
> 
> I believe it's a design feature that you need not. Essentially both
> parties start in a 'no handshake has taken place' state, and on the
> first read or write from either end, one party starts the handshake
> (and there is a provision in case they collide). Alternatively
> either end can explicitly request the handshake.
> 
> There is actually an implementation advantage to the server doing it
> (having just written it) which is that the server can then capture
> any error (invalid credentials or whatever) and report it with whatever
> logging it does for the STARTTLS option; otherwise invalid certificate
> responses come with the next option it receives.

Okay, fine then.

[...]
> > [...]
> >> +The client MUST NOT issue `NBD_OPT_STARTTLS` unless the server
> >> +set flag NBD_FLAG_FIXED_NEWSTYLE and the client replied
> >> +with NBD_FLAG_C_FIXED_NEWSTYLE in the fixed newstyle
> >> +negotiation.
> > 
> > Why not "unless fixed newstyle negotiation is in effect"? No need to
> > repeat that definition.
> 
> Can do; I just wanted to be explicit that both server and client
> must support it.

Sure, just want to make things not *too* verbose, is all.

> > [...]
> >> +### Security considerations
> >> +
> >> +#### TLS versions
> >> +
> >> +NBD implementations supporting TLS MUST support TLS version 1.2,
> >> +SHOULD support any later versions, and MAY support older versions.
> > 
> > I would prefer "SHOULD NOT allow TLS versions older than 1.2" here.
> > There are some serious flaws in older TLS versions; currently these are
> > still supported by most web browsers for backwards compatibility
> > reasons, but that does not apply for us.
> 
> I'd be all for that. Or certainly "SHOULD NOT support LS versions older
> than 1.2 by default"

Or that. The point is that doing TLS < 1.2 is stupid, especially for a
new protocol, so I think we should make it explicit that clients should
not try that save in exceptional circumstances.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Qemu-devel] [Nbd] [PATCHv3] Improve documentation for TLS

[Alex Bligh]

Wouter,

On 9 Apr 2016, at 11:38, Wouter Verhelst <w@uter.be> wrote:
>> 
>> As per previous message, because SELECTIVETLS requires INFO,
>> but OPTIONALTLS doesn't.
> 
> Um. So you're suggesting that if a client sends INFO, we're suddenly in
> a whole different mode of operation?
> 
> That seems to make little sense (other than "complicate matters for no
> particularly good reason")

Nope, not at all. Sorry I should have been clearer.

There are four modes. Nothing depends on whether a client sends info.
The modes are:
* NOTLS
* OPTIONALTLS
* SELECTIVETLS
* FORCEDTLS

The server needs to implement at least one (even if only NOTLS
which is what the reference server currently does!), and can operate
in any mode it chooses. How it responds to the client is a product
of which mode it is in. The client doesn't control the mode in any way.

However, if it operates in SELECTIVETLS mode, it MUST also support the
INFO option. That 'MUST' requirement does not apply to 'OPTIONALTLS'.

Your question was (paraphrasing) 'why even bother having the OPTIONALTLS
mode if it's essentially a degenerate case of SELECTIVETLS'. The answer
is because a server that does not support INFO cannot support SELECTIVETLS,
but can support OPTIONALTLS.

An alternative route would be to delete OPTIONALTLS, and make some of
the MUST requirements in SELECTIVETLS say "MUST xyz unless there are
no TLS-only exports". However, this makes it rather harder to read,
so I described that case as a separate mode.

>> I'd be all for that. Or certainly "SHOULD NOT support LS versions older
>> than 1.2 by default"
> 
> Or that. The point is that doing TLS < 1.2 is stupid, especially for a
> new protocol, so I think we should make it explicit that clients should
> not try that save in exceptional circumstances.

+1. Do you want to ping me when you have had a chance to review v5 and
I will collate all of these in to a v6?

-- 
Alex Bligh

Re: [Qemu-devel] [Nbd] [PATCHv3] Improve documentation for TLS

[Wouter Verhelst]

On Sat, Apr 09, 2016 at 12:21:03PM +0100, Alex Bligh wrote:
> An alternative route would be to delete OPTIONALTLS, and make some of
> the MUST requirements in SELECTIVETLS say "MUST xyz unless there are
> no TLS-only exports". However, this makes it rather harder to read,
> so I described that case as a separate mode.

I understand now.

However, although I disagree with Daniel on the idea of having a server
which can (in the same process) support both TLS-enabled and
non-TLS-enabled exports, I do agree with him that what you call
OPTIONALTLS is a bad idea, and that it should be discouraged.

Mentioning that option explicitly is counter to that goal, and I would
therefore prefer that you not add it.

Also, while we try to negotiate the protocol in such a way that things
remain compatible between implementations who implement a disjoint set
of features from the protocol, I think the long-term goal should be that
STARTTLS and INFO are supported by all implementations (or at least,
that INFO is). In that context, explicitly explaining (in much detail)
what happens when a client doesn't support INFO but does support
STARTTLS seems contraproductive.

So I'd just drop optional.

> >> I'd be all for that. Or certainly "SHOULD NOT support LS versions older
> >> than 1.2 by default"
> > 
> > Or that. The point is that doing TLS < 1.2 is stupid, especially for a
> > new protocol, so I think we should make it explicit that clients should
> > not try that save in exceptional circumstances.
> 
> +1. Do you want to ping me when you have had a chance to review v5 and
> I will collate all of these in to a v6?

I have, but did not have any further comments.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12

Re: [Qemu-devel] [Nbd] [PATCHv3] Improve documentation for TLS

[Alex Bligh]

Wouter,

On 9 Apr 2016, at 12:38, Wouter Verhelst <w@uter.be> wrote:

> On Sat, Apr 09, 2016 at 12:21:03PM +0100, Alex Bligh wrote:
>> An alternative route would be to delete OPTIONALTLS, and make some of
>> the MUST requirements in SELECTIVETLS say "MUST xyz unless there are
>> no TLS-only exports". However, this makes it rather harder to read,
>> so I described that case as a separate mode.
> 
> I understand now.
> 
> However, although I disagree with Daniel on the idea of having a server
> which can (in the same process) support both TLS-enabled and
> non-TLS-enabled exports, I do agree with him that what you call
> OPTIONALTLS is a bad idea, and that it should be discouraged.
> 
> Mentioning that option explicitly is counter to that goal, and I would
> therefore prefer that you not add it.
> 
> Also, while we try to negotiate the protocol in such a way that things
> remain compatible between implementations who implement a disjoint set
> of features from the protocol, I think the long-term goal should be that
> STARTTLS and INFO are supported by all implementations (or at least,
> that INFO is). In that context, explicitly explaining (in much detail)
> what happens when a client doesn't support INFO but does support
> STARTTLS seems contraproductive.
> 
> So I'd just drop optional.

OK. I will kill it in v6.

In practice it means 'if you want to export some things with
TLS and some without then you need to implement INFO'.
This would be a *good* thing if INFO is brought into the main
standard (i.e. taken beyond experimental). Eric's just sent
patches for Qemu to qemu-devel. I need to check the implementation
on my server is still compliant, but it's basically done. So
I may argue for INFO to be put into the body of the standard.

>>>> I'd be all for that. Or certainly "SHOULD NOT support LS versions older
>>>> than 1.2 by default"
>>> 
>>> Or that. The point is that doing TLS < 1.2 is stupid, especially for a
>>> new protocol, so I think we should make it explicit that clients should
>>> not try that save in exceptional circumstances.
>> 
>> +1. Do you want to ping me when you have had a chance to review v5 and
>> I will collate all of these in to a v6?
> 
> I have, but did not have any further comments.

Great. v6 coming up.

-- 
Alex Bligh