Re: [Qemu-block] [PATCH 00/15] Implement TLS support to QEMU NBD server & client

[Wouter Verhelst]

[nbd-general added to Cc]

Hi Daniel,

On Fri, Nov 27, 2015 at 12:20:38PM +0000, Daniel P. Berrange wrote:
> This series of patches implements support for TLS in the QEMU NBD
> server and client code.
> 
> It is implementing the NBD_OPT_STARTTLS option that was previously
> discussed here:
> 
>   https://www.redhat.com/archives/libvir-list/2014-October/msg00506.html
> 
> And is also described in the NBD spec here:
> 
>   https://github.com/yoe/nbd/blob/master/doc/proto.md
> 
> Based on my impl I think the NBD_OPT_STARTTLS specification is
> fine. In my impl, when TLS is requested in the server, I chose
> to *refuse* all NBD options until NBD_OPT_STARTTLS has completed
> successfully. There is an annoying thing about the fixed new style
> protocol in that it made an exception for NBD_OPT_EXPORT_NAME,
> so that it is still not possible to report errors for that option.
> 
> [qupte]
>  - The server will reply to any option apart from `NBD_OPT_EXPORT_NAME`
>     with reply packets in the following format:
> 
>   S: 64 bits, `0x3e889045565a9` (magic number for replies)  
>   S: 32 bits, the option as sent by the client to which this is a reply  
>   S: 32 bits, reply type (e.g., `NBD_REP_ACK` for successful completion,
>      or `NBD_REP_ERR_UNSUP` to mark use of an option not known by this
>      server  
>   S: 32 bits, length of the reply. This may be zero for some replies, in
>      which case the next field is not sent  
>   S: any data as required by the reply (e.g., an export name in the case
>      of `NBD_REP_SERVER`)
> [/quote]
> 
> I wish those words "apart from NBD_OPT_EXPORT_NAME" were not there
> for fixed new style protocol - maybe it needs a very-fixed new
> style protocol...

Yes. This was done in this way to more easily retain backwards compatibility
with non-fixed newstyle. In hindsight, it was probably a mistake to do
things that way, but it's not going to be easy to turn back the clock
now...

I have been thinking of adding a message NBD_OPT_SELECT_EXPORT to
replace NBD_OPT_EXPORT_NAME, which would select an export but not end
negotiation. That would also require another message to end negotiation
and move on to the transmission phase, obviously.

The negotation would then look something like:

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ACK
NBD_OPT_GO
 <- NBD_REP_ACK
[transmission phase]

or, to add some error conditions:

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ERR_UNSUP # this server doesn't support SELECT_EXPORT
NBD_OPT_EXPORT_NAME # fall back to older message
[transmission phase]

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ERR_TLS_REQD # this export requires TLS
NBD_OPT_STARTTLS
 <- NBD_REP_ACK
[... tls ...]
NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ACK
NBD_OPT_GO
 <- NBD_REP_ACK
[transmission phase]

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ACK
NBD_OPT_STARTTLS
 <- NBD_REP_ACK
[... tls ...]
NBD_OPT_GO
 <- NBD_REP_ERR_INVALID # export selected before starttls is not retained

NBD_OPT_SELECT_EXPORT
 <- NBD_REP_ACK
NBD_OPT_SELECT_EXPORT (we actually want this other one)
 <- NBD_REP_ERR_UNKNOWN # unknown export
NBD_OPT_GO
 <- NBD_REP_ERR_INVALID # second select cleared the result of the first one

etc

I'm not sure whether it's worth the trouble, though.

> As is, if the client connects to a TLS enabled NBD server and then
> immediately sends NBD_OPT_EXPORT_NAME, it is not possible for us
> to send back NBD_REP_ERR_TLS_REQD as the spec requires that the
> server close the connection :-(  For this reason I have made the
> qemu NBD client always send NBD_OPT_LIST as the first thing it
> does, so that we can see the NBD_REP_ERR_TLS_REQD response.

I suppose that is a valid workaround.

> I chose to *NOT* implement the NBD_OPT_PEEK_EXPORT option as it is
> inherently insecure to have a client to ask the server which
> exports need TLS, while the protocol is still running in plain text
> mode, as it allows a trivial MITM downgrade attack. I can see value
> in the NBD_OPT_PEEK_EXPORT option for probing other features, but
> only after TLS has been negotiated. As such I believe NBD_F_EXP_TLS_OK
> and NBD_F_EXP_TLS_REQ should be deleted from the spec as it is not
> possible to use them in a secure manner.

Mm. Fair enough.

The reason for adding that flag was to support encrypted and
non-encrypted exports from the same server, but I suppose we can signal
those things with NBD_REP_ERR_TLS_REQD and NBD_REP_ERR_POLICY, too.

> This series of patches has my v3 I/O channels patch series as a
> pre-requisite:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg04208.html
> 
> Usage of TLS is described in the commit messages for each patch,
> but for sake of people who don't want to explore the series, here's
> the summary
> 
> Starting QEMU system emulator with a disk backed by an TLS encrypted
> NBD export
> 
>   $ qemu-system-x86_64 \
>      -object 
> tls-creds-x509,id=tls0,endpoint=client,dir=/home/berrange/security/qemutls
>      -drive driver=nbd,host=localhost,port=9000,tls-creds=tls0
> 
> Starting a standalone NBD server providing a TLS encrypted NBD export
> 
>   $ qemu-nbd \
>      --object 
> tls-creds-x509,id=tls0,endpoint=server,dir=/home/berrange/security/qemutls
>      --tls-creds tls0 \
>      --exportname default
> 
> NB, exportname is mandatory, since TLS requires the fixed-new style
> NBD protocol negotiation.

Note, I have modified the spec of the fixed newstyle negotiation a while
back to include support for a default export:

'A special, "empty", name (i.e., the length field is zero and no name is
specified), is reserved for a "default" export, to be used in cases
where explicitly specifying an export name makes no sense.'

(in the "Option types" section for NBD_OPT_EXPORT_NAME)

This removes the requirement to have an export name specified
externally. Having said that though, obviously adding support for
specifying a name isn't a bad thing.

-- 
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26