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

[Eric Blake <eblake@redhat.com>]

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

On 04/07/2016 09:33 AM, 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 | 336 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 302 insertions(+), 34 deletions(-)

> +++ b/doc/proto.md
> @@ -286,6 +286,289 @@ S: (*length* bytes of data if the request is of type `NBD_CMD_READ`)
>  This reply type MUST NOT be used except as documented by the
>  experimental `STRUCTURED_REPLY` extension; see below.
>  
> +## TLS support
> +
> +The NBD protocol supports TLS via negotiation with the `NBD_OPT_STARTTLS`

Worth spelling out the TLS acronym here, and/or making this a link to a
relevant web page? Not sure what the best page would be, though.

> +option. This is performed as an in-session upgrade. Below the term
> +'negotiation' is used to refer to the sending and receiving of
> +NBD commands, and the term 'initiation' of TLS is used to refer to
> +the actual upgrade to TLS.
> +
> +### Certificates, authentication and authorisation
> +
> +This standard does not specify what encryption, certification
> +and signature algorithms are used. This standard does not
> +specify authentication and authorisation (for instance
> +whether client and/or server certificates are required and
> +what they should contain); this is implementation dependent.
> +
> +TLS requires fixed newstyle negotiation to have completed.

Sounds awkward - fixed newstyle flags are advertised by the server and
replied by the client, but overall negotiation is not completed until
all client options have been sent.  Maybe:

TLS requires both server and client to support fixed newstyle negotiation.

> +
> +### Server-side requirements
> +
> +There are four modes of operation for a server. The
> +server MUST support one of these modes.
> +

> +The server MUST operate in NOTLS mode unless the server
> +set flag NBD_FLAG_FIXED_NEWSTYLE and the client replied
> +with NBD_FLAG_C_FIXED_NEWSTYLE in the fixed newstyle
> +negotiation.

Good.

> +
> +These modes of operations are described in detail below.
> +
> +#### NOTLS mode
> +
> +If the server receives `NBD_OPT_STARTTLS` it MUST respond with
> +`NBD_REP_ERR_UNSUP`. The server MUST NOT respond to any
> +command with `NBD_REP_ERR_TLS_REQD`.

Is 'option request' a better word than 'command'?

> +#### FORCEDTLS mode
> +
> +If the server receives `NBD_OPT_STARTTLS` it MUST reply with
> +`NBD_REP_ACK` and initiate TLS as set out under 'OPTIONALTLS'
> +above.
> +
> +If the server receives any other option, including `NBD_OPT_INFO`,
> +and unsupported options, it SHOULD reply with `NBD_REP_ERR_TLS_REQD`

I'm thinking MUST is better than SHOULD in this mode (and matches qemu's
implementation).

no comma after `NBD_OPT_INFO`

> +if TLS has not been initiated; `NBD_OPT_INFO` is included as in this

s/;/./

> +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
> +enter transmission mode via `NBD_OPT_GO` when TLS has not been
> +initiated, it MUST error with `NBD_REP_ERR_TLS_REQD`.

> +#### SELECTIVETLS mode
> +
> +If the server receives `NBD_OPT_STARTTLS` it MUST reply with
> +`NBD_REP_ACK` and initiate TLS as set out under 'OPTIONALTLS'
> +above.
> +
> +If the server receives any other option except `NBD_OPT_INFO`,
> +it MUST NOT reply with `NBD_REP_ERR_TLS_REQD`. If the

Should that be `NBD_OPT_INFO` or `NBD_OPT_GO`, since we want to allow
NBD_OPT_GO to fail with ERR_TLS_REQD on a TLS-required export?

> +server receives `NBD_OPT_INFO` and TLS has not been
> +initiated, it MAY reply with `NBD_REP_ERR_TLS_REQD` if that
> +export is non-existent, and MUST reply with `NBD_REP_ERR_TLS_REQD`
> +if that export is TLS-only.
> +
> +If the server receives a request to enter transmission mode
> +via `NBD_OPT_EXPORT_NAME` on a TLS-only export when TLS has not
> +been initiated, then as this request cannot error, it MUST
> +disconnect the connection. If the server receives a request to
> +enter transmission mode via `NBD_OPT_GO` when TLS has not been

via `NBD_OPT_GO` on a TLS-only export

> +initiated, it MUST error with `NBD_REP_ERR_TLS_REQD`.

Maybe put this paragraph before the one about "any other option", and
then that paragraph can limit its exclusion to NBD_OPT_INFO.

> +
> +The server MUST NOT send `NBD_REP_ERR_TLS_REQD` in reply to
> +any command if TLS has already been neogitated. The server

s/neogitated/negotiated/

> +MUST NOT send `NBD_REP_ERR_TLS_REQD` in response to any
> +option other than `NBD_OPT_INFO` or `NBD_OPT_GO`, and
> +only in those cases in respect of a TLS-only or non-existent
> +export.
> +

> +
> +## Client-side requirements
> +
> +If the client supports TLS at all, it MUST be prepared
> +to deal with servers operating in any of the above modes.
> +Notwithstanding, a client MAY always disconnect or
> +refuse to connect to a particular export if TLS is
> +not available and the user requires TLS.
> +
> +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.
> +
> +The client MUST NOT issue `NBD_OPT_STARTTLS` if TLS has already
> +been initiated.
> +
> +Subject to the above two limitations, the client MAY send
> +`NBD_OPT_STARTTLS` at any time to initiate a TLS session. If the
> +client receives `NBD_REP_ACK` in response, it MUST immediately
> +upgrade the connection to TLS. If it receives `NBD_ERR_REP_UNSUP`
> +in response or any other error, it indicates that the server cannot
> +or will not upgrade the connection to TLS and therefore MUST either
> +continue the connection without TLS, or disconnect.
> +
> +A client that prefers to use TLS irrespective of whether
> +the server makes TLS mandatory SHOULD send `NBD_OPT_STARTTLS`
> +as the first option. This will ensure option haggling is subject
> +to TLS, and will thus prevent the possibilty of options being

s/possibilty/possibility/

> +compromised by a MitM attack. Note that the `NBD_OPT_STARTTLS`

You spell out MitM later so I'm not too worried, but maybe it's worth
floating the definition up higher.

> +itself may be compromised - see 'downgrade attacks' for
> +more details. For this reasons a client which only wishes

s/reasons/reason/

> +to use TLS SHOULD disconnect if the `NBD_OPT_STARTTLS`
> +replies with an error.
> +
> +If the TLS handshake is unsuccessful (for instance the server's
> +certificate does not validate) the client MUST disconnect as
> +by this stage it is too late to continue without TLS.
> +

> +
> +### Security considerations
> +
> +#### TLS versions
> +

We crossed mail; I had some reviews on the security implications that
still need fixing in v3, which I won't repeat here.

> +NBD implementations supporting TLS MUST support TLS version 1.2,
> +SHOULD support any later versions, and MAY support older versions.
> +Older versions SHOULD NOT be used where there is a risk of security
> +problems with those older versions or of a downgrade attack
> +against TLS versions.
> +


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