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

[Wouter Verhelst]

On Sat, Apr 09, 2016 at 11:04:09AM +0100, Alex Bligh wrote:
[...]
> > [...]
> >> +The server MUST NOT send `NBD_REP_ERR_TLS_REQD` in reply to
> >> +any command if TLS has already been neogitated. The server
> > 
> > negotiated
> 
> I'd make sure you're looking at the latest version as Eagle Eyed Eric
> pointed out a whole pile of these.

Yeah, I noticed :-)

> > [...]
> >> +The client MAY send `NBD_OPT_STARTTLS` at any time to initiate
> >> +a TLS session, except that the client MUST NOT send
> >> +`NBD_OPT_STARTTLS` if TLS has alreay been initiated. If the
> >> +cllient 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 discconnect.
> > 
> > That, or NBD_REP_ERR_POLICY.
> 
> Yeah I can make that an alternative.

POLICY is the correct message; UNSUP is the alternative ;-)

(as in "for backwards compatibility, a client should also be prepared...")

[...]
> > (actually, "any error". If STARTTLS errors, the server effectively does
> > not support TLS)
> 
> Well NBD_REP_ERR_INVALID means "The option sent by the client is known by
> this server, but was determined by the server to be syntactically invalid."
> which means the client has done something wrong. Given we've defined the
> legal responses to NBD_OPT_STARTTLS I'd rather keep that one.

Fair enough. Also, INVALID is the correct error message when the client sent
NBD_OPT_STARTTLS while inside a TLS connection, too, so that would've been a
contradiction ;-)

> > [...]
> >> - `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_UNSUP`.
> > 
> > (or POLICY)
> 
> OK

-- 
< 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] [PATCH] Improve documentation for TLS

[Wouter Verhelst]

On Sat, Apr 09, 2016 at 11:05:16AM +0100, Alex Bligh wrote:
> 
> On 9 Apr 2016, at 10:50, Wouter Verhelst  wrote:
> 
> > So if I want to swap to qemu-nbd, I cannot also have encrypted
> > connections to the same server. Got it.
> 
> AFAIK qemu-nbd only supports a single export so this isn't
> really an issue.

qemu-nbd does, but the builtin server of qemu will export all virtual
hard disks connected to the VM, so it supports multiple exports.

(yes, that implies that swapping to a multiple-export qemu server is
probably a bad idea)

-- 
< 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] [PATCH] Improve documentation for TLS

[Alex Bligh]

On 9 Apr 2016, at 10:55, Wouter Verhelst  wrote:

> 
> Yes.
> 
>> That way, a client can send ANY option to learn if TLS is required (even
>> an option that the server does not recognize); where NBD_OPT_INFO and
>> NBD_OPT_LIST are probably the two most useful options, but where ANY
>> option works.  A server with TLS but not FIXED_NEWSTYLE is pointless
> 
> Actually, such a server is technically impossible ;-)

Yeah, you really want to be reading v5 of this patch set by which
time we cleared all this up :-)

-- 
Alex Bligh

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

[Alex Bligh]

On 9 Apr 2016, at 10:50, Wouter Verhelst  wrote:

> So if I want to swap to qemu-nbd, I cannot also have encrypted
> connections to the same server. Got it.

AFAIK qemu-nbd only supports a single export so this isn't
really an issue.

-- 
Alex Bligh

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

[Alex Bligh]

On 9 Apr 2016, at 10:36, Wouter Verhelst  wrote:

>> +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_UNSUPP`. The server MUST NOT respond to any
> 
> No. UNSUP (one P) is reserved for "this server doesn't support that"
> (i.e., backwards compatibility). Not configured for supporting TLS would
> be NBD_REP_ERR_POLICY.

'P' fixed in later draft.

Right, but a server which doesn't actually have TLS at all will
by definition be running in NOTLS mode, and thus will be replying
NBD_REP_ERR_UNSUP (because that's what you reply with if you
receive an option you don't understand). Allowing NBD_REP_ERR_POLICY
to be sent as as a reply too would be fine.

> 
>> +command with `NBD_REP_ERR_TLS_REQD`.
>> +
>> + 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.
>> +If the handshake is unsuccessful (for instance the client's
>> +certificate does not match) the server MUST disconnect as
>> +by this stage it is too late to continue without TLS as the
>> +acknowledgement has been sent.
>> +
>> +The server MUST NOT respond to any command with `NBD_REP_ERR_TLS_REQD`,
>> +as TLS is optional.
> 
> I think this mode is effectively the same as what you call selective,
> modulo that no exports have any TLS requirements, so I wouldn't specify
> it as a separate mode of operation (save perhaps that you may want to
> discourage it)

See the section on 'degenerate cases' in the last version. I agree
OPTIONALTLS is not to be encouraged, however it's there so it's possible
for a server to provide optionality when it doesn't support INFO.
As INFO is merely experimental, I didn't want to make a 'MUST' dependency
on it and thus lose all ability to do optional TLS without INFO.

>> +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.
> 
> Right. Clearly this can't be a must, because qemu already implements
> this and doesn't do INFO :-)

Yeah though Eric has (I think) submitted a patch to qemu to support
that.

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

I'd make sure you're looking at the latest version as Eagle Eyed Eric
pointed out a whole pile of these.

> 
> [...]
>> +The client MAY send `NBD_OPT_STARTTLS` at any time to initiate
>> +a TLS session, except that the client MUST NOT send
>> +`NBD_OPT_STARTTLS` if TLS has alreay been initiated. If the
>> +cllient 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 discconnect.
> 
> That, or NBD_REP_ERR_POLICY.

Yeah I can make that an alternative.

> 
> [...]
>> +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 server MAY therefore choose to issue
> 
> client, not server

Again, EEE fixed this.
> 
>> +a `NBD_OPT_STARTTLS`, MAY disconnect 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.
>> +
>> +The client MAY discover the server operates in NOTLS mode by
>> +sending `NBD_OPT_STARTTLS`. If `NBD_REP_ERR_UNSUPP` is
>> +replied, it is guaranteed the server is not in this mode.
> 
> UNSUP or POLICY

OK

> (actually, "any error". If STARTTLS errors, the server effectively does
> not support TLS)

Well NBD_REP_ERR_INVALID means "The option sent by the client is known by
this server, but was determined by the server to be syntactically invalid."
which means the client has done something wrong. Given we've defined the
legal responses to NBD_OPT_STARTTLS I'd rather keep that one.

> 
> [...]
>> - `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 

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

[Wouter Verhelst]

On Thu, Apr 07, 2016 at 08:31:23AM -0600, Eric Blake wrote:
> > +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.
> 
> I'd go one step further:
> 
> If a server uses FORCEDTLS, it MUST implement the
> NBD_FLAG_FIXED_NEWSTYLE flag, and SHOULD implement the INFO extension.

Yes.

> That way, a client can send ANY option to learn if TLS is required (even
> an option that the server does not recognize); where NBD_OPT_INFO and
> NBD_OPT_LIST are probably the two most useful options, but where ANY
> option works.  A server with TLS but not FIXED_NEWSTYLE is pointless

Actually, such a server is technically impossible ;-)

> (since TLS was introduced at the same time as fixed newstyle,

Eh. I don't know where you got that idea, but that's absolutely not
true. Fixed newstyle was introduced five years ago, TLS was introduced
last year or so.

> we can reasonably require, rather than just suggest, that both things
> be implemented at once to be a compliant FORCEDTLS server).

We have to make fixed newstyle a dependency of any form of tls, but
nothing more seems appropriate.

("fixed newstyle" is necessary for *anything* that is not
NBD_OPT_EXPORT_NAME)

[...]

-- 
< 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] [PATCH] Improve documentation for TLS

[Wouter Verhelst]

On Thu, Apr 07, 2016 at 02:56:48PM +0100, Daniel P. Berrange wrote:
> I don't really agree that there's a use case of mixing
> tls & non-tls exports in the same server.

There is: swap-on-NBD and TLS do not mix.

Without special handling, swapping to the network is prone to deadlocks,
because the machine doesn't know when a block of swapped data has been
written to disk until it receives and processes the ACK from the server
that tells it the data is safe. This means it must keep reading from the
server until that ACK is received. Since the reason you're paging memory
to your swap space is, most likely, the fact that you're low on memory,
swapping to the network while simultaneously trying to view a 4K video
from youtube will get you deadlocked in seconds.

To fix that, a PF_MEMALLOC kernel-level socket option was created a few
years ago. If the kernel is low on memory, it will drop all network
packets save those destined for a socket with that option set. In doing
so, the 4K youtube video data will be dropped until the low memory
situation is resolved, but the reply packet from the NBD server will
arrive.

However, only the kernel can set the PF_MEMALLOC socket option; and due
to the way in which we pass the nbd socket to the kernel, having it
decode the TLS conversation seems complicated, if it even is possible.
Therefore, decoding the TLS conversation when the client is going to be
the kernel, will need to be done in userspace. The nbd-client would need
to call socketpair(), fork(), pass one end of the pair in the child
process to the kernel, and be a decoding proxy in the parent.

Since you *need* to have non-TLS NBD if you want to support
non-deadlocking swapspace over the network (one of the major use cases
for NBD), you *need* to have a server which supports non-TLS NBD.

Requiring that a user uses two different NBD servers, one with TLS
enabled and one with TLS disabled, just so swapping to NBD is possible,
seems silly.

Therefore, the reference implementation is not going to *require* that
all exports do TLS if TLS is enabled; however, it *is* going to default
to that.

> > Incidentally, the qemu client does not appear to assume the
> > server is 'FORCEDTLS', and IIRC from time spend staring into
> > Wireshark yesterday sends its NBD_OPT_LIST prior to the TLS
> > upgrade. I can check that if you like.
> 
> If the qemu NBD client has TLS credentials set then it will
> refuse to connect to a server without TLS.

That seems like a valid and safe mode of operation.

> The OPT_TLS is definitely the first thing it sends, becasue the QEMU
> NBD server will reject all options until OPT_TLS has been sent.

So if I want to swap to qemu-nbd, I cannot also have encrypted
connections to the same server. Got it.

-- 
< 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] [PATCH] Improve documentation for TLS

[Wouter Verhelst]

On Thu, Apr 07, 2016 at 12:35:59PM +0100, 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 
> ---
>  doc/proto.md | 267 
> +++
>  1 file changed, 234 insertions(+), 33 deletions(-)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index f117394..9437e9b 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -286,6 +286,226 @@ 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`
> +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.
> +
> +### TLS versions Certificates, authentication and authorisation
> +
> +NBD implementations supporting TLS MUST support TLS version
> +1.2, and MAY support other (earlier or later) versions of
> +TLS/SSL.
> +
> +This standard does not specify what encryption, certification
> +and signature algorithms are used. This standard does not
> +specify authentication and authortisation (for instance
> +whether client and/or server certificates are required and
> +what they should contain); this is implementation dependent.
> +
> +### 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
> +  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 chose 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`, and MAY refuse to provide information
> +  about non-existent exports via `NBD_OPT_INFO`.
> +
> +The server MAY determine the mode in which it operates
> +dependent upon the connection (for instance it might be
> +more liberal with 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.
> +
> +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_UNSUPP`. The server MUST NOT respond to any

No. UNSUP (one P) is reserved for "this server doesn't support that"
(i.e., backwards compatibility). Not configured for supporting TLS would
be NBD_REP_ERR_POLICY.

> +command with `NBD_REP_ERR_TLS_REQD`.
> +
> + 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.
> +If the handshake is unsuccessful (for instance the client's
> +certificate does not match) the server MUST disconnect as
> +by this stage it is too late to continue without TLS as the
> +acknowledgement has been sent.
> +
> +The server MUST NOT respond to any command with `NBD_REP_ERR_TLS_REQD`,
> +as TLS is optional.

I think