diff mbox

[PATCHv8] Improve documentation for TLS

Message ID 1460292452-1348-1-git-send-email-alex@alex.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Bligh April 10, 2016, 12:47 p.m. UTC
* 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 three possible modes of operation of a server.

* Add text defining what 'terminate the session' means during
  negotiation, and when it is available.

Signed-off-by: Alex Bligh <alex@alex.org.uk>
---
 doc/proto.md | 342 +++++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 308 insertions(+), 34 deletions(-)

Changes since v7

* I missed committing the changes re consistent use of 'option' rather than 'command'
  in v7. They are here now.

Changes from v6:

* Introduced language mandating a server to reply with NBD_ERR_INVALID
 to NBD_OPT_STARTTLS if TLS is already negotiatied.

* Removed some duplication in SELECTIVETLS over the prohibition on
 servers not returning NBD_ERR_TLSREQD to options other than
 NBD_OPT_EXPORTNAME, NBD_OPT_INFO and NBD_OPT_GO. The same thing
 was said a different way a couple of paragraphs below.

* Consistently refer to 'options' rather than 'commands' in the
 negotiation phase.

* Eric Blake's nits

Changes from v5:

* Delete OPTIONALTLS (RIP)

* Add NBD_REP_ERR_POLICY

* s/NBD_ERR_REP/NBD_REP_ERR/ in one place

* Consistently use the phrase 'terminate the session' to mean dropping
the connection, as per Wouter. Note there are other inconsistent
uses of 'dropping the connection', 'disconnecting' etc. elsewhere
which I haven't touched.

* Similarly refer to the connection as a 'session' when it doesn't
explicitly mean the L3 TCP connection (TLS section only).

* Introduce a paragraph under newstyle negotiation emphasising that
terminating the session is legal and sometimes required, and defining
it.

Changes from v4

* Minor grammar nit

Changes from v3:

* Delete confusing text about server omitting entries from NBD_OPT_LIST
if TLS is not negotiated and FORCETLS is used, as that (of course)
requires NBD_REP_ERR_TLS_REQD elsewhere in the text.

* Further nits from Eric Blake

Changes from v2:

* The response to a command is a response, not a NBD_REP_ACK

* Make it clear that the response can be errored

* Nits from Eric Blake

Changes from v1:

* Make a NBD_CMD_CLOSE imply a flush

* Nits from Eric Blake

Comments

Wouter Verhelst April 11, 2016, 6:10 a.m. UTC | #1
Mostly there. Final note:

On Sun, Apr 10, 2016 at 01:47:32PM +0100, Alex Bligh wrote:
> diff --git a/doc/proto.md b/doc/proto.md
> index f117394..5005552 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -195,6 +195,13 @@ request before sending the next one of the same type. The server MAY
>  send replies in the order that the requests were received, but is not
>  required to.
>  
> +There is no requirement for the client or server to complete a negotiation
> +if it does not wish to do so. If the client does not find an export it
> +is looking for (for instance) it may simply close the TCP connection.
> +Under certain circumstances either the client or the server may be required
> +by this document to close the TCP connection. In each case, this is
> +referred to as 'terminate the session'.
> +
>  ### Transmission

NAK. If we have disconnect messages (NBD_OPT_ABORT and NBD_CMD_DISC), it
makes sense to say that clients should use them. Protocol violations by
peers are a different matter; but in the general case you should drop a
connection properly, i.e., by using the relevant "close the connection"
command.

(I realize I didn't comment on this earlier, but I changed my mind
during the discussion about DISC).
Alex Bligh April 11, 2016, 7:27 a.m. UTC | #2
Wouter,

On 11 Apr 2016, at 07:10, Wouter Verhelst <w@uter.be> wrote:

> Mostly there. Final note:
> 
> On Sun, Apr 10, 2016 at 01:47:32PM +0100, Alex Bligh wrote:
>> diff --git a/doc/proto.md b/doc/proto.md
>> index f117394..5005552 100644
>> --- a/doc/proto.md
>> +++ b/doc/proto.md
>> @@ -195,6 +195,13 @@ request before sending the next one of the same type. The server MAY
>> send replies in the order that the requests were received, but is not
>> required to.
>> 
>> +There is no requirement for the client or server to complete a negotiation
>> +if it does not wish to do so. If the client does not find an export it
>> +is looking for (for instance) it may simply close the TCP connection.
>> +Under certain circumstances either the client or the server may be required
>> +by this document to close the TCP connection. In each case, this is
>> +referred to as 'terminate the session'.
>> +
>> ### Transmission
> 
> NAK. If we have disconnect messages (NBD_OPT_ABORT and NBD_CMD_DISC), it
> makes sense to say that clients should use them. Protocol violations by
> peers are a different matter; but in the general case you should drop a
> connection properly, i.e., by using the relevant "close the connection"
> command.
> 
> (I realize I didn't comment on this earlier, but I changed my mind
> during the discussion about DISC).

This section only relates to the negotiation phase, so really this is
about use (or not) or NBD_OPT_ABORT, not NBD_OPT_DISC.

Your statement is a bit surprising though as far as NBD_OPT_ABORT
is concerned. 

Firstly, there is no way to the *server* to send NBD_OPT_ABORT.
That's what this paragraph was primarily aimed at.

Secondly proto.md has:

> The phase continues until either side closes the connection.

That implies that either the client or the server can initiate the
close.

I thought on this basis its use was entirely optional.

NBD_OPT_ABORT says:

> - `NBD_OPT_ABORT` (2)
> 
>     The client desires to abort the negotiation and close the
>     connection.
> 

I *presume* it has a reply (all the others do). Should a client
wait for the undocumented reply before closing its end of
the connection or not? I must admit the semantics are sufficiently
opaque though I support it server side (with a reply) I would
never sent it client side.

Note also that in some circumstances where I document 'terminate
the session' it's not possible for the client to send an NBD_OPT_ABORT.
The two obvious ones are:

* After NBD_OPT_EXPORTNAME has been issued - if for instance
  the client does not like the flags.

* After NBD_OPT_STARTTLS has been issued and the NBD_REP_ACK
  has been sent, but the TLS handshake itself fails client
  side (for instance the server cert does not work).

Obviously NBD_OPT_ABORT and aborting the connection needs
more clearing up, but I'm loathe to do it in the TLS patch.

In order not to make things worse, how about:

> There is no requirement for the client or server to complete a
> negotiation if it does not wish to do so. Either end may simply
> close the TCP connection (though see below re prior use
> of NBD_OPT_ABORT). Under certain circumstances either
> the client or the server may be required by this document to close
> the TCP connection. In each case, this is referred to as 'terminate
> the session'.
> 
> If the client wishes to terminate the session in the negotiation
> phase, and is not doing so because it is required to do so
> by this document, it SHOULD send NBD_OPT_ABORT first if the protocol
> permits. There are instances where this is impossible, such as after
> an NBD_OPT_EXPORTNAME has been issued, or on an unsuccessful
> negotiation of TLS.  For instance, if the client does not find an
> export it is looking for, it may simply send an NBD_OPT_ABORT
> and close the TCP connection.
Eric Blake April 11, 2016, 8:14 p.m. UTC | #3
On 04/11/2016 01:27 AM, Alex Bligh wrote:

>>> +There is no requirement for the client or server to complete a negotiation
>>> +if it does not wish to do so. If the client does not find an export it
>>> +is looking for (for instance) it may simply close the TCP connection.
>>> +Under certain circumstances either the client or the server may be required
>>> +by this document to close the TCP connection. In each case, this is
>>> +referred to as 'terminate the session'.
>>> +
>>> ### Transmission
>>
>> NAK. If we have disconnect messages (NBD_OPT_ABORT and NBD_CMD_DISC), it
>> makes sense to say that clients should use them. Protocol violations by
>> peers are a different matter; but in the general case you should drop a
>> connection properly, i.e., by using the relevant "close the connection"
>> command.
>>
>> (I realize I didn't comment on this earlier, but I changed my mind
>> during the discussion about DISC).
> 
> This section only relates to the negotiation phase, so really this is
> about use (or not) or NBD_OPT_ABORT, not NBD_OPT_DISC.
> 
> Your statement is a bit surprising though as far as NBD_OPT_ABORT
> is concerned. 
> 
> Firstly, there is no way to the *server* to send NBD_OPT_ABORT.
> That's what this paragraph was primarily aimed at.
> 
> Secondly proto.md has:
> 
>> The phase continues until either side closes the connection.
> 
> That implies that either the client or the server can initiate the
> close.
> 
> I thought on this basis its use was entirely optional.
> 
> NBD_OPT_ABORT says:
> 
>> - `NBD_OPT_ABORT` (2)
>>
>>     The client desires to abort the negotiation and close the
>>     connection.
>>
> 
> I *presume* it has a reply (all the others do). Should a client
> wait for the undocumented reply before closing its end of
> the connection or not? I must admit the semantics are sufficiently
> opaque though I support it server side (with a reply) I would
> never sent it client side.

Current qemu NBD server implementation does NOT send a reply to
NBD_OPT_ABORT, but immediately closes the connection. I don't know if
that is a bug in qemu (especially given the discussion on NBD_CMD_DISC),
but it is an independent issue from TLS documentation, so may be better
discussed on that thread.

Likewise, current qemu NBD client implementation does NOT send
NBD_OPT_ABORT at all, so it's hard to say whether waiting around for a
reply is worthwhile.

> 
> Obviously NBD_OPT_ABORT and aborting the connection needs
> more clearing up, but I'm loathe to do it in the TLS patch.
> 
> In order not to make things worse, how about:
> 
>> There is no requirement for the client or server to complete a
>> negotiation if it does not wish to do so. Either end may simply
>> close the TCP connection (though see below re prior use

Not sure if the use of "re" is ideal (are you abbreviating for "regarding")?

>> of NBD_OPT_ABORT). Under certain circumstances either
>> the client or the server may be required by this document to close
>> the TCP connection. In each case, this is referred to as 'terminate
>> the session'.
>>
>> If the client wishes to terminate the session in the negotiation
>> phase, and is not doing so because it is required to do so
>> by this document, it SHOULD send NBD_OPT_ABORT first if the protocol
>> permits. There are instances where this is impossible, such as after
>> an NBD_OPT_EXPORTNAME has been issued, or on an unsuccessful
>> negotiation of TLS.  For instance, if the client does not find an
>> export it is looking for, it may simply send an NBD_OPT_ABORT
>> and close the TCP connection.

Otherwise, this seems reasonable, other than the fact that qemu needs
patches to actually start sending NBD_OPT_ABORT where possible.
Alex Bligh April 11, 2016, 8:34 p.m. UTC | #4
Eric,

On 11 Apr 2016, at 21:14, Eric Blake <eblake@redhat.com> wrote:
> Current qemu NBD server implementation does NOT send a reply to
> NBD_OPT_ABORT, but immediately closes the connection. I don't know if
> that is a bug in qemu (especially given the discussion on NBD_CMD_DISC),
> but it is an independent issue from TLS documentation, so may be better
> discussed on that thread.

Ha, neither does mine, despite my reading of the protocol being
that it should.

Reference nbd-server.c doesn't either.

> Likewise, current qemu NBD client implementation does NOT send
> NBD_OPT_ABORT at all, so it's hard to say whether waiting around for a
> reply is worthwhile.

:-)

nbd-client.c only appears to send it after asking for a list,
and not in any error conditions.

>> 
>> Obviously NBD_OPT_ABORT and aborting the connection needs
>> more clearing up, but I'm loathe to do it in the TLS patch.
>> 
>> In order not to make things worse, how about:
>> 
>>> There is no requirement for the client or server to complete a
>>> negotiation if it does not wish to do so. Either end may simply
>>> close the TCP connection (though see below re prior use
> 
> Not sure if the use of "re" is ideal (are you abbreviating for "regarding")?

OK will fix that if Wouter likes the words.

>>> of NBD_OPT_ABORT). Under certain circumstances either
>>> the client or the server may be required by this document to close
>>> the TCP connection. In each case, this is referred to as 'terminate
>>> the session'.
>>> 
>>> If the client wishes to terminate the session in the negotiation
>>> phase, and is not doing so because it is required to do so
>>> by this document, it SHOULD send NBD_OPT_ABORT first if the protocol
>>> permits. There are instances where this is impossible, such as after
>>> an NBD_OPT_EXPORTNAME has been issued, or on an unsuccessful
>>> negotiation of TLS.  For instance, if the client does not find an
>>> export it is looking for, it may simply send an NBD_OPT_ABORT
>>> and close the TCP connection.
> 
> Otherwise, this seems reasonable, other than the fact that qemu needs
> patches to actually start sending NBD_OPT_ABORT where possible.

I'd suggest waiting for a definitive answer on whether it's meant
to have a reply.

--
Alex Bligh
Wouter Verhelst April 12, 2016, 6:01 a.m. UTC | #5
On Mon, Apr 11, 2016 at 09:34:44PM +0100, Alex Bligh wrote:
> Eric,
> 
> On 11 Apr 2016, at 21:14, Eric Blake <eblake@redhat.com> wrote:
> > Current qemu NBD server implementation does NOT send a reply to
> > NBD_OPT_ABORT, but immediately closes the connection. I don't know if
> > that is a bug in qemu (especially given the discussion on NBD_CMD_DISC),
> > but it is an independent issue from TLS documentation, so may be better
> > discussed on that thread.
> 
> Ha, neither does mine, despite my reading of the protocol being
> that it should.
> 
> Reference nbd-server.c doesn't either.

Indeed. That may have been a bad idea.

> > Likewise, current qemu NBD client implementation does NOT send
> > NBD_OPT_ABORT at all, so it's hard to say whether waiting around for a
> > reply is worthwhile.
> 
> :-)
> 
> nbd-client.c only appears to send it after asking for a list,
> and not in any error conditions.

Well, it was added together with NBD_OPT_LIST, and with fixed newstyle (it was
my test case for implementing fixed newstyle ;-)

> >> Obviously NBD_OPT_ABORT and aborting the connection needs
> >> more clearing up, but I'm loathe to do it in the TLS patch.
> >> 
> >> In order not to make things worse, how about:
> >> 
> >>> There is no requirement for the client or server to complete a
> >>> negotiation if it does not wish to do so. Either end may simply
> >>> close the TCP connection (though see below re prior use
> > 
> > Not sure if the use of "re" is ideal (are you abbreviating for "regarding")?
> 
> OK will fix that if Wouter likes the words.
> 
> >>> of NBD_OPT_ABORT). Under certain circumstances either
> >>> the client or the server may be required by this document to close
> >>> the TCP connection. In each case, this is referred to as 'terminate
> >>> the session'.
> >>> 
> >>> If the client wishes to terminate the session in the negotiation
> >>> phase, and is not doing so because it is required to do so
> >>> by this document, it SHOULD send NBD_OPT_ABORT first if the protocol
> >>> permits. There are instances where this is impossible, such as after
> >>> an NBD_OPT_EXPORTNAME has been issued, or on an unsuccessful
> >>> negotiation of TLS.  For instance, if the client does not find an
> >>> export it is looking for, it may simply send an NBD_OPT_ABORT
> >>> and close the TCP connection.
> > 
> > Otherwise, this seems reasonable, other than the fact that qemu needs
> > patches to actually start sending NBD_OPT_ABORT where possible.
> 
> I'd suggest waiting for a definitive answer on whether it's meant
> to have a reply.

Clearly from reading the code it wasn't meant to, at the time.

That doesn't mean OPT_ABORT not having a reply is necessarily a good
idea. Since it's only used by reference nbd-client in just one use case
at this point, I don't think it's particularly bad to change the
definition to say that the server SHOULD send a reply (NBD_REP_ACK),
upon which the server drops the connection.

The client should probably wait for that too, and not close its socket
until either it gets a zero read (indicating that the server closed it
already) or it gets an NBD_REP_ACK from the NBD_OPT_ABORT message.
Alex Bligh April 12, 2016, 7:47 a.m. UTC | #6
On 12 Apr 2016, at 07:01, Wouter Verhelst <w@uter.be> wrote:

> hat doesn't mean OPT_ABORT not having a reply is necessarily a good
> idea. Since it's only used by reference nbd-client in just one use case
> at this point, I don't think it's particularly bad to change the
> definition to say that the server SHOULD send a reply (NBD_REP_ACK),
> upon which the server drops the connection.
> 
> The client should probably wait for that too, and not close its socket
> until either it gets a zero read (indicating that the server closed it
> already) or it gets an NBD_REP_ACK from the NBD_OPT_ABORT message.

Yeah. That way would be a safe change (as the worst that can
happen is the client thinks the server has rudely dropped
the connection).
Wouter Verhelst April 12, 2016, 9:20 a.m. UTC | #7
On Tue, Apr 12, 2016 at 08:47:49AM +0100, Alex Bligh wrote:
> 
> On 12 Apr 2016, at 07:01, Wouter Verhelst <w@uter.be> wrote:
> 
> > hat doesn't mean OPT_ABORT not having a reply is necessarily a good
> > idea. Since it's only used by reference nbd-client in just one use case
> > at this point, I don't think it's particularly bad to change the
> > definition to say that the server SHOULD send a reply (NBD_REP_ACK),
> > upon which the server drops the connection.
> > 
> > The client should probably wait for that too, and not close its socket
> > until either it gets a zero read (indicating that the server closed it
> > already) or it gets an NBD_REP_ACK from the NBD_OPT_ABORT message.
> 
> Yeah. That way would be a safe change (as the worst that can
> happen is the client thinks the server has rudely dropped
> the connection).

Right.

To summarize, there are three ways for the connection to end:

- The client wishes to end the session, and sends the appropriate
  termination message (OPT_ABORT or CMD_DISC). This is a normal
  disconnect.
- Either peer violates a MUST in the spec, and the other side doesn't
  know how to handle the resulting inconsistency. The only proper
  solution at that point is to drop the connection, but that's only
  because there's really nothing else we *can* do. This is an abnormal
  disconnect.
- The server wishes to terminate the session. There isn't actually a
  message for this, so it also results in an abnormal disconnect.

Perhaps we could state that the server can send a message (offset 0,
length 0, handle 0, error EINTR) when it wants to terminate the session
for whatever reason (e.g., because it's being restarted).

Originally, there were a number of termination points where we could
drop the connection without further explanation. It was a mess, because
it resulted in confusing messages (e.g., the server would produce error
messages in system logs for every disconnect because it couldn't
distinguish between clean disconnects and unclean disconnects).

I don't want to go there again.
Alex Bligh April 12, 2016, 9:53 a.m. UTC | #8
Wouter,

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

> To summarize, there are three ways for the connection to end:
> 
> - The client wishes to end the session, and sends the appropriate
>  termination message (OPT_ABORT or CMD_DISC). This is a normal
>  disconnect.
> - Either peer violates a MUST in the spec, and the other side doesn't
>  know how to handle the resulting inconsistency. The only proper
>  solution at that point is to drop the connection, but that's only
>  because there's really nothing else we *can* do. This is an abnormal
>  disconnect.
> - The server wishes to terminate the session. There isn't actually a
>  message for this, so it also results in an abnormal disconnect.

The last case includes (e.g.) 'NBD_OPT_EXPORT_NAME' issued to
a non-existing mount)

> Perhaps we could state that the server can send a message (offset 0,
> length 0, handle 0, error EINTR) when it wants to terminate the session
> for whatever reason (e.g., because it's being restarted).

I think that will make clients' life harder. Most clients don't
expect messages from the server which aren't replies, so I can see
them treating it as a reply to the next message they issue, or
getting into some horrible blocking situation.

(Also please don't use EINTR - that implies you can retry. ETERM?)

> Originally, there were a number of termination points where we could
> drop the connection without further explanation. It was a mess, because
> it resulted in confusing messages (e.g., the server would produce error
> messages in system logs for every disconnect because it couldn't
> distinguish between clean disconnects and unclean disconnects).
> 
> I don't want to go there again.

I think what we should probably say is this (wording needs
tweaking I know):

* One side MAY drop the connection if the other end violates a
  MUST condition.
* The server MUST drop the connection in the 'no way out' situations
  during the negotiation phase (error on NBD_OPT_EXPORT_NAME, error
  in negotiating text).
* As protocol authors we should minimise the number of 'no way out'
  situations.
* The server SHOULD NOT otherwise drop the connection. It can wait
  and error the next command. Clearly there are situations where
  this is going to happen (e.g. server shutdown).
* If the server does need to drop the connection, it SHOULD wait
  until there are no commands in-flight in transmission mode,
  it possible.
* If he client is going to drop the the connection, then other
  than in the event of a protocol violation or a 'no way out'
  situation (e.g. TLS negotiation fails), it MUST use NBD_CMD_DISC
  or NBD_OPT_ABORT
* We should tidy up the semantics and descriptions of NBD_CMD_DISC
  and NBD_OPT_ABORT, viz replies or not to the latter, shutting
  down TLS properly etc.
Wouter Verhelst April 12, 2016, 12:40 p.m. UTC | #9
On Tue, Apr 12, 2016 at 10:53:57AM +0100, Alex Bligh wrote:
> Wouter,
> 
> On 12 Apr 2016, at 10:20, Wouter Verhelst <w@uter.be> wrote:
> 
> > To summarize, there are three ways for the connection to end:
> > 
> > - The client wishes to end the session, and sends the appropriate
> >  termination message (OPT_ABORT or CMD_DISC). This is a normal
> >  disconnect.
> > - Either peer violates a MUST in the spec, and the other side doesn't
> >  know how to handle the resulting inconsistency. The only proper
> >  solution at that point is to drop the connection, but that's only
> >  because there's really nothing else we *can* do. This is an abnormal
> >  disconnect.
> > - The server wishes to terminate the session. There isn't actually a
> >  message for this, so it also results in an abnormal disconnect.
> 
> The last case includes (e.g.) 'NBD_OPT_EXPORT_NAME' issued to
> a non-existing mount)
> 
> > Perhaps we could state that the server can send a message (offset 0,
> > length 0, handle 0, error EINTR) when it wants to terminate the session
> > for whatever reason (e.g., because it's being restarted).
> 
> I think that will make clients' life harder. Most clients don't
> expect messages from the server which aren't replies, so I can see
> them treating it as a reply to the next message they issue, or
> getting into some horrible blocking situation.
> 
> (Also please don't use EINTR - that implies you can retry. ETERM?)
> 
> > Originally, there were a number of termination points where we could
> > drop the connection without further explanation. It was a mess, because
> > it resulted in confusing messages (e.g., the server would produce error
> > messages in system logs for every disconnect because it couldn't
> > distinguish between clean disconnects and unclean disconnects).
> > 
> > I don't want to go there again.
> 
> I think what we should probably say is this (wording needs
> tweaking I know):
> 
> * One side MAY drop the connection if the other end violates a
>   MUST condition.
> * The server MUST drop the connection in the 'no way out' situations
>   during the negotiation phase (error on NBD_OPT_EXPORT_NAME, error
>   in negotiating text).
> * As protocol authors we should minimise the number of 'no way out'
>   situations.
> * The server SHOULD NOT otherwise drop the connection. It can wait
>   and error the next command. Clearly there are situations where
>   this is going to happen (e.g. server shutdown).
> * If the server does need to drop the connection, it SHOULD wait
>   until there are no commands in-flight in transmission mode,
>   it possible.
> * If he client is going to drop the the connection, then other
>   than in the event of a protocol violation or a 'no way out'
>   situation (e.g. TLS negotiation fails), it MUST use NBD_CMD_DISC
>   or NBD_OPT_ABORT
> * We should tidy up the semantics and descriptions of NBD_CMD_DISC
>   and NBD_OPT_ABORT, viz replies or not to the latter, shutting
>   down TLS properly etc.

Right, that sounds good.
Alex Bligh April 12, 2016, 12:57 p.m. UTC | #10
On 12 Apr 2016, at 13:40, Wouter Verhelst <w@uter.be> wrote:

> Right, that sounds good.

Great. I may look at that when the other doc patches are applied.

On which note, back to $subject, how is PATCHv8?
Wouter Verhelst April 12, 2016, 1:01 p.m. UTC | #11
On Tue, Apr 12, 2016 at 01:57:25PM +0100, Alex Bligh wrote:
> 
> On 12 Apr 2016, at 13:40, Wouter Verhelst <w@uter.be> wrote:
> 
> > Right, that sounds good.
> 
> Great. I may look at that when the other doc patches are applied.
> 
> On which note, back to $subject, how is PATCHv8?

It's not being applied because of this ;-)

I can probably apply it and then work on the clarification of that
paragraph, I suppose, but I'd prefer doing it right from the get go.
Alex Bligh April 12, 2016, 1:29 p.m. UTC | #12
Wouter,

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

> It's not being applied because of this ;-)

OK, I've sent a PATCHv9 with my suggested wording (so at least
it doesn't make anything worse), but I really think we should
avoid addressing further nits in the disconnection regime within
a change which is meant to be documenting TLS.

Alex
 

> I can probably apply it and then work on the clarification of that
> paragraph, I suppose, but I'd prefer doing it right from the get go.
diff mbox

Patch

diff --git a/doc/proto.md b/doc/proto.md
index f117394..5005552 100644
--- a/doc/proto.md
+++ b/doc/proto.md
@@ -195,6 +195,13 @@  request before sending the next one of the same type. The server MAY
 send replies in the order that the requests were received, but is not
 required to.
 
+There is no requirement for the client or server to complete a negotiation
+if it does not wish to do so. If the client does not find an export it
+is looking for (for instance) it may simply close the TCP connection.
+Under certain circumstances either the client or the server may be required
+by this document to close the TCP connection. In each case, this is
+referred to as 'terminate the session'.
+
 ### Transmission
 
 There are three message types in the transmission phase: the request,
@@ -286,6 +293,287 @@  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 Transport Layer Security (TLS) (see
+[RFC5246](https://tools.ietf.org/html/rfc5246)
+as updated by
+[RFC6176](https://tools.ietf.org/html/rfc6176)
+).
+
+TLS is negotiated with the `NBD_OPT_STARTTLS`
+option. This is performed as an in-session upgrade. Below the term
+'negotiation' is used to refer to the sending and receiving of
+NBD options and option replies, 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.
+
+### Server-side requirements
+
+There are three modes of operation for a server. The
+server MUST support one of these modes.
+
+* The server operates entirely without TLS ('NOTLS'); 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
+  exports, and is available at the client's option on all
+  other exports ('SELECTIVETLS'). The server does not force
+  the client to upgrade to TLS during option haggling (as
+  if the client ultimately were to choose a non-TLS-only export,
+  stopping TLS is not possible). Instead it permits the client
+  to upgrade as and when it chooses, but unless an upgrade to
+  TLS has already taken place, the server errors attempts
+  to enter transmission mode on TLS-only exports, MAY
+  refuse to provide information about TLS-only exports
+  via `NBD_OPT_INFO`, MAY refuse to provide information
+  about non-existent exports via `NBD_OPT_INFO`, and MAY omit
+  exports that are TLS-only from `NBD_OPT_LIST`.
+
+The server MAY determine the mode in which it operates
+dependent upon the session (for instance it might be
+more liberal with TCP connections made over the loopback
+interface) but it MUST be consistent in its mode
+of operation across the lifespan of a single TCP connection
+to the server. A client MUST NOT assume indications from
+a prior TCP session to a given server will be relevant
+to a subsequent session.
+
+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.
+
+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_POLICY` (if it does not support TLS for
+policy reasons) or `NBD_REP_ERR_UNSUP` (if it does not
+support the `NBD_OPT_STARTTLS` option at all). The server MUST NOT
+respond to any option request with `NBD_REP_ERR_TLS_REQD`.
+
+#### FORCEDTLS mode
+
+If the server receives `NBD_OPT_STARTTLS` prior to negotiating
+TLS, it MUST reply with `NBD_REP_ACK`. If the server receives
+`NBD_OPT_STARTTLS` when TLS has already been negotiated, it
+it MUST reply with `NBD_REP_ERR_INVALID`.
+
+After an `NBD_REP_ACK` 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 session.
+
+As per the TLS standard, the handshake MAY be initiated either
+by the server (having sent the `NBD_REP_ACK`) or by the client.
+If the handshake is unsuccessful (for instance the client's
+certificate does not match) the server MUST terminate the
+session as by this stage it is too late to continue without TLS
+as the acknowledgement has been sent.
+
+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
+terminate the session. 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`.
+
+The server MUST NOT send `NBD_REP_ERR_TLS_REQD` in reply to
+any option if TLS has already been initiated.
+
+The FORCEDTLS mode of operation has an implementation problem in
+that the client MAY legally simply send a `NBD_OPT_EXPORT_NAME`
+to enter transmission mode without previously sending any options.
+Therefore, if a server uses FORCEDTLS, it SHOULD implement the
+INFO extension.
+
+#### SELECTIVETLS mode
+
+If the server receives `NBD_OPT_STARTTLS` prior to negotiating
+TLS, it MUST reply with `NBD_REP_ACK` and initiate TLS as set
+out under 'FORCEDTLS' above. If the server receives
+`NBD_OPT_STARTTLS` when TLS has already been negotiated, it
+it MUST reply with `NBD_REP_ERR_INVALID`.
+
+If the server receives `NBD_OPT_INFO` or `NBD_OPT_GO` 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
+terminate the session.
+
+The server MUST NOT send `NBD_REP_ERR_TLS_REQD` in reply to
+any option if TLS has already been negotiated. The server
+MUST NOT send `NBD_REP_ERR_TLS_REQD` in response to any
+option other than `NBD_OPT_INFO`, `NBD_OPT_GO` and
+`NBD_OPT_EXPORT_NAME`, and only in those cases in respect of
+a TLS-only or non-existent export.
+
+There is a degenerate case of SELECTIVETLS where all
+exports are TLS-only. This is permitted in part to make programming
+of servers easier. Operation is a little different from FORCEDTLS,
+as the client is not forced to upgrade to TLS prior to any options
+being processed, and the server MAY choose to give information on
+non-existent exports via NBD_OPT_INFO exports prior to an upgrade
+to TLS.
+
+The SELECTIVETLS mode of operation has an implementation problem
+in that unless the INFO extension is supported, the client that
+does not use TLS may have its access to exports denied without
+it being able to ascertain the reason. For instance it may
+go into transmission mode using `NBD_OPT_EXPORT_NAME` - which
+does not return an error as no options will be denied with
+`NBD_REP_ERR_TLS_REQD`. Further there is no way to remotely
+determine whether an export requires TLS, and therefore this
+must be initiated between client and server out of band.
+Therefore, if a server uses SELECTIVETLS, it MUST implement
+the INFO extension.
+
+## 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 terminate the session 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 session to TLS. If it receives `NBD_REP_ERR_UNSUP`,
+`NBD_REP_ERR_POLICY` or any other error in response, it indicates
+that the server cannot or will not upgrade the session to TLS,
+and therefore the client MUST either continue the session
+without TLS, or terminate the session.
+
+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 possibility of options being
+compromised by a Man-in-the-Middle attack. Note that the
+`NBD_OPT_STARTTLS` itself may be compromised - see 'downgrade
+attacks' for more details. For this reason, a client which only
+wishes to use TLS SHOULD terminate the session 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 terminate the
+session as by this stage it is too late to continue without TLS.
+
+If the client receives an `NBD_REP_ERR_TLS_REQD` in response
+to any option, it implies that this option cannot be executed
+unless a TLS upgrade is performed. If the option is any
+option other than `NBD_OPT_INFO` or `NBD_OPT_GO`, this
+indicates that no option will succeed unless a TLS upgrade
+is performed; the client MAY therefore choose to issue
+an `NBD_OPT_STARTTLS`, or MAY terminate the session (if
+for instance it does not support TLS or does not have
+appropriate credentials for this server). If the client
+receives `NBD_REP_ERR_TLS_REQD` in response to
+`NBD_OPT_INFO` or `NBD_OPT_GO` this indicates that the
+export referred to within the option is either non-existent
+or requires TLS; the client MAY therefore choose to issue
+an `NBD_OPT_STARTTLS`, MAY terminate the session (if
+for instance it does not support TLS or does not have
+appropriate credentials for this server), or MAY continue
+in another manner without TLS, for instance by querying
+or using other exports.
+
+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`.
+
+### Security considerations
+
+#### TLS versions
+
+NBD implementations supporting TLS MUST support TLS version 1.2,
+SHOULD support any later versions. NBD implementations
+MAY support older versions but SHOULD NOT do so by default
+(i.e. they SHOULD only be available by a configuration change).
+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.
+
+#### Protocol downgrade attacks
+
+A danger inherent in any scheme relying on the negotiation
+of whether TLS should be employed is downgrade attacks within
+the NBD protocol.
+
+There are two main dangers:
+
+* A Man-in-the-Middle (MitM) hijacks a session and impersonates
+  the server (possibly by proxying it) claiming not to support
+  TLS. In this manner, the client is confused into operating
+  in a plain-text manner with the MitM (with the session possibly
+  being proxied in plain-text to the server using the method
+  below).
+
+* The MitM hijacks a session and impersonates the client
+  (possibly by proxying it) claiming not to support TLS. In
+  this manner the server is confused into operating in a plain-text
+  manner with the MitM (with the session being possibly
+  proxied to the client with the method above).
+
+With regard to the first, any client that does not wish
+to be subject to potential downgrade attack SHOULD ensure
+that if a TLS endpoint is specified by the client, it
+ensures that TLS is negotiated prior to sending or
+requesting sensitive data. To recap, the client MAY send
+`NBD_OPT_STARTTLS` at any point during option haggling,
+and MAY terminate the session if `NBD_REP_ACK` is not
+provided.
+
+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.
+
+### Status
+
+This functionality has not yet been implemented by the reference
+implementation, but was implemented by qemu and subsequently
+by other users, so has been moved out of the "experimental" section.
+
 ## Values
 
 This section describes the value and meaning of constants (other than
@@ -366,7 +654,7 @@  of the newstyle negotiation.
     Data: String, name of the export, as free-form text.
     The length of the name is determined from the option header. If the
     chosen export does not exist or requirements for the chosen export
-    are not met (e.g., the client did not negotiate TLS for an export
+    are not met (e.g., the client did not initiate TLS for an export
     where the server requires it), the server should close the
     connection.
 
@@ -391,7 +679,9 @@  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 SHOULD omit entries from this
+    list if TLS has not been negotiated, the server is operating in
+    SELECTIVETLS mode, and the entry concerned is a TLS-only export.
 
 - `NBD_OPT_PEEK_EXPORT` (4)
 
@@ -400,21 +690,15 @@  of the newstyle negotiation.
 
 - `NBD_OPT_STARTTLS` (5)
 
-    The client wishes to initiate TLS. If the server replies with
-    `NBD_REP_ACK`, then the client should immediately initiate a TLS
-    handshake and continue the negotiation in the encrypted channel. If
-    the server is unwilling to perform TLS, it should reply with
-    `NBD_REP_ERR_POLICY`. For backwards compatibility, a client should
-    also be prepared to handle `NBD_REP_ERR_UNSUP`. If the client sent
-    along any data with the request, the server should send back
-    `NBD_REP_ERR_INVALID`. The client MUST NOT send this option if
-    it has already negotiated TLS; if the server receives
-    `NBD_OPT_STARTTLS` when TLS has already been negotiated, the server
-    MUST send back `NBD_REP_ERR_INVALID`.
-
-    This functionality has not yet been implemented by the reference
-    implementation, but was implemented by qemu so has been moved out of
-    the "experimental" section.
+    The client wishes to initiate TLS.
+
+    The server MUST either reply with `NBD_REP_ACK` after which
+    point the connection is upgraded to TLS, or reply with
+    `NBD_REP_ERR_POLICY` (or if it does not support the option
+    at all, `NBD_REP_ERR_UNSUP`, or if TLS has already been
+    negotiated, `NBD_REP_ERR_INVALID`).
+
+    See the section on TLS above for further details.
 
 - `NBD_OPT_INFO` (6)
 
@@ -489,20 +773,10 @@  case that data is an error message string suitable for display to the user.
 * `NBD_REP_ERR_TLS_REQD` (2^31 + 5)
 
     The server is unwilling to continue negotiation unless TLS is
-    negotiated first. A server MUST NOT send this error if it has one or
-    more exports that do not require TLS; not even if the client indicated
-    interest (by way of `NBD_OPT_PEEK_EXPORT`) in an export which requires
-    TLS.
-
-    If this reply is used, servers SHOULD send it in reply to each and every
-    unencrypted `NBD_OPT_*` message (apart from `NBD_OPT_STARTTLS`).
-
-    This functionality has not yet been implemented by the reference
-    implementation, but was implemented by qemu so has been moved out of
-    the "experimental" section.
-
-    The experimental `INFO` extension makes small but compatible
-    changes to the semantics of this error message; see below.
+    initiated first. In the case of `NBD_OPT_INFO` and `NBD_OPT_GO`
+    this unwillingness MAY (depending on the TLS mode) be limited
+    to the export in question. See the section on TLS above for
+    further details.
 
 * `NBD_REP_ERR_UNKNOWN` (2^31 + 6)
 
@@ -735,13 +1009,13 @@  Therefore these commands share common documentation.
     - `NBD_REP_ERR_UNKNOWN`: The chosen export does not exist on this
       server.
     - `NBD_REP_ERR_TLS_REQD`: The server does not wish to export this
-      block device unless the client negotiates TLS first.
+      block device unless the client initiates TLS first.
     - `NBD_REP_SERVER`: The server accepts the chosen export.
 
-    Additionally, if TLS has not been negotiated, the server MAY reply
+    Additionally, if TLS has not been initiated, the server MAY reply
     with `NBD_REP_ERR_TLS_REQD` (instead of `NBD_REP_ERR_UNKNOWN`)
     to requests for exports that are unknown. This is so that clients
-    that have not negotiated TLS cannot enumerate exports.
+    that have not initiated TLS cannot enumerate exports.
 
     In the case of `NBD_REP_SERVER`, the message's data takes on a different
     interpretation than the default (so as to provide additional