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

[Eric Blake]

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 <address@hidden>
> ---
>  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 <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature