[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [Nbd] [PATCHv2] Improve documentation for TLS



Eric,

>> +++ 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.

Can do.

https://tools.ietf.org/html/rfc5246

as updated by:

https://tools.ietf.org/html/rfc6176

>> +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.

I don't understand. Newstyle negotiation is determined by the server
in the second 32 bit word it sends. There is no client interaction.
Fixed newstyle is negotiated by the server sending NBD_FLAG_FIXED_NEWSTYLE
in the first packet (handshake flags) to which the client responds
with NBD_FLAG_C_FIXED_NEWSTYLE in its client flags immediately. Therefore
fixed newstyle negotiation is completed before the very first option is
sent.

>> +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'?

It is certainly a better word to describe option requests (rather
than commands) :-)

>> +#### 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`

Agree.

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

I think the semicolon is correct as it signifies the two phrases
are tightly bound, and specifically that 'included' refers to
'included within "any other option"'.

>> +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?

Yes, and below.

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

Now handled with the above case so deleted.

>> +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.

Deleted as above.

>> +
>> +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/

thx

>> +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/

thx

> 
>> +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.

fixed

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

ok

>> +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.

fixed

I'll send a v3.

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

--
Alex Bligh




Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail


Reply to: