Re: [Qemu-block] [PATCH v2 RFC] qemu-nbd: Permit TLS with Unix sockets

[Max Reitz]

On 04.07.19 00:47, Eric Blake wrote:
> Although you generally won't use encryption with a Unix socket (after
> all, everything is local, so why waste the CPU power), there are
> situations in testsuites where Unix sockets are much nicer than TCP
> sockets.  Since nbdkit allows encryption over both types of sockets,
> it makes sense for qemu-nbd to do likewise.

Hmm.  The code is simple enough, so I don’t see a good reason not to.

> The restriction has been present since its introduction in commits
> 145614a1 and 75822a12 (v2.6), where the former documented the
> limitation but did not provide any additional explanation why it was
> added; but looking closer, it seems the most likely reason is that
> x509 verification requires a hostname. But we can do the same as
> migration did, and add a tls-hostname parameter to supply that
> information.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> 
> Since this is now adding a new qemu-nbd command-line option, as well
> as new QMP for blockdev-add, it has missed 4.1 softfreeze and should
> probably be delayed to 4.2.
> 
> RFC: The test is racy - it sometimes passes, and sometimes fails with:
> 
>  == check TLS with authorization over Unix ==
>  qemu-img: Could not open 
> 'driver=nbd,path=SOCKET,tls-creds=tls0,tls-hostname=localhost': Failed to 
> read option reply: Cannot read from TLS channel: Input/output error
> -qemu-img: Could not open 
> 'driver=nbd,path=SOCKET,tls-creds=tls0,tls-hostname=localhost': Failed to 
> read option reply: Cannot read from TLS channel: Input/output error
> +qemu-img: Could not open 
> 'driver=nbd,path=SOCKET,tls-creds=tls0,tls-hostname=localhost': Failed to 
> read option reply: Cannot read from TLS channel: Software caused connection 
> abort

Well, the first thing is that over TCP, the reference output shows that
it should indeed fail with ECONNABORTED.  So to me it seems like EIO is
actually the wrong error code.

Um, also, a perhaps stupid question: Why is there no passing test for
client authorization?

> I suspect that there is a bug in the qio TLS channel code when it
> comes to handling a failed TLS handshake, which results in the racy
> output. I'll need help solving that first.  It might also be nice if
> we had a bit more visibility into the gnutls error message when TLS
> handshake fails.

Well, what I can see is that the error code comes from
qcrypto_tls_session_read().  You get ECONNABORTED for
GNUTLS_E_PREMATURE_TERMINATION, and EIO for GNUTLS_E_PULL_ERROR (under
default; but that’s the error that appears if it isn’t
PREMATURE_TERMINATION).

So I suppose you get ECONNABORTED if the first read happens after the
RST is received (or the equivalent on Unix sockets, I have no idea how
they work on the low level); and you get EIO if you try to read before
that (because the TLS connection has just not been established
successfully).

I have experimented a bit, but unfortunately couldn’t find anything to
change the test results in any way... :/

> ---
>  qemu-nbd.texi              |  3 ++
>  qapi/block-core.json       |  5 ++
>  block/nbd.c                | 27 +++++++++--
>  qemu-nbd.c                 | 26 ++++++++---
>  tests/qemu-iotests/233     | 94 ++++++++++++++++++++++++++++++++++++--
>  tests/qemu-iotests/233.out | 61 +++++++++++++++++++++++--
>  tests/qemu-iotests/group   |  2 +-
>  7 files changed, 198 insertions(+), 20 deletions(-)
> 
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 7f55657722bd..764518baef84 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -123,6 +123,9 @@ Store the server's process ID in the given file.
>  Specify the ID of a qauthz object previously created with the
>  --object option. This will be used to authorize connecting users
>  against their x509 distinguished name.
> +@item --tls-hostname=NAME
> +When using list mode with TLS over a Unix socket, supply the hostname
> +to use during validation of the server's x509 certificate.
>  @item -v, --verbose
>  Display extra debugging information.
>  @item -h, --help

qemu-nbd.c has some parameter documentation, too.  Maybe this option
should be listed there.

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0d43d4f37c1a..95da0d44c220 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3856,6 +3856,10 @@
>  #
>  # @tls-creds:   TLS credentials ID
>  #
> +# @tls-hostname: Hostname of the server, required only when using x509 based
> +#                TLS credentials when @server lacks a hostname (such as
> +#                using a Unix socket). (Since 4.1)

Well, 4.2 now.

> +#
>  # @x-dirty-bitmap: A "qemu:dirty-bitmap:NAME" string to query in place of
>  #                  traditional "base:allocation" block status (see
>  #                  NBD_OPT_LIST_META_CONTEXT in the NBD protocol) (since 3.0)

[...]

> diff --git a/block/nbd.c b/block/nbd.c
> index 81edabbf35ed..ce3db21190ce 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c

[...]

> @@ -1624,12 +1629,25 @@ static int nbd_open(BlockDriverState *bs, QDict 
> *options, int flags,
>              goto error;
>          }
> 
> -        /* TODO SOCKET_ADDRESS_KIND_FD where fd has AF_INET or AF_INET6 */
> -        if (s->saddr->type != SOCKET_ADDRESS_TYPE_INET) {
> -            error_setg(errp, "TLS only supported over IP sockets");
> +        switch (s->saddr->type) {
> +        case SOCKET_ADDRESS_TYPE_INET:
> +            hostname = s->saddr->u.inet.host;
> +            if (qemu_opt_get(opts, "tls-hostname")) {
> +                error_setg(errp, "tls-hostname not required with inet 
> socket");

This is more “not allowed”, right?

(Actually, why not?  We could make the information from @server a
default, but this would override it.  Maybe useful just for testing, but
why not.)

> +                goto error;
> +            }
> +            break;
> +        case SOCKET_ADDRESS_TYPE_UNIX:
> +            hostname = qemu_opt_get(opts, "tls-hostname");

Shouldn’t we check that @hostname is non-NULL?  As far as I read the
code now, if this option doesn’t exist, “<null>” will be used as the
hostname somewhere down the stack.  Which probably gives a weird error.

> +            break;
> +        default:
> +            /* TODO SOCKET_ADDRESS_KIND_FD where fd has AF_INET or AF_INET6 
> */
> +            error_setg(errp, "TLS only supported over IP or Unix sockets");
>              goto error;
>          }
> -        hostname = s->saddr->u.inet.host;
> +    } else if (qemu_opt_get(opts, "tls-hostname")) {
> +        error_setg(errp, "tls-hostname not supported without tls-creds");
> +        goto error;
>      }
> 
>      /* NBD handshake */

[...]

> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index a8cb39e51043..40ea1e299dc7 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c

[...]

> @@ -931,18 +937,22 @@ int main(int argc, char **argv)
>      }
> 
>      if (tlscredsid) {
> -        if (sockpath) {
> -            error_report("TLS is only supported with IPv4/IPv6");
> -            exit(EXIT_FAILURE);
> -        }
>          if (device) {
>              error_report("TLS is not supported with a host device");
>              exit(EXIT_FAILURE);
>          }
>          if (tlsauthz && list) {
> -            error_report("TLS authorization is incompatible with export 
> list");
> +            error_report("TLS authorization is incompatible with --list");
>              exit(EXIT_FAILURE);
>          }
> +        if (tlshost) {
> +            if (!list) {
> +                error_report("TLS hostname is only for use with --list");
> +                exit(EXIT_FAILURE);
> +            }
> +        } else {
> +            tlshost = bindto;

Again, I wonder whether there should be a nice error message here if
bindto is NULL.

> +        }
>          tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err);
>          if (local_err) {
>              error_report("Failed to get TLS creds %s",

[...]

> diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
> index 9b46284ab0de..b86bee020649 100644
> --- a/tests/qemu-iotests/233.out
> +++ b/tests/qemu-iotests/233.out

[...]

> +== check TLS works over Unix ==
> +image: nbd+unix://?socket=SOCKET
> +file format: nbd
> +virtual size: 64 MiB (67108864 bytes)
> +disk size: unavailable

This has worked surprisingly well considering you did not pass tls-hostname.

On the same note: If I remove the tls-hostname option from the “perform
I/O over TLS” test, it keeps working.

> +image: nbd+unix://?socket=SOCKET
> +file format: nbd
> +virtual size: 64 MiB (67108864 bytes)
> +disk size: unavailable
> +qemu-nbd: Certificate does not match the hostname 0.0.0.0

Yeah, that’s a weird error message.  I think it could be better.

Max

> +exports available: 1
> + export: ''
> +  size:  67108864
> +  flags: 0x4ed ( flush fua trim zeroes df cache )
> +  min block: 1
> +  opt block: 4096
> +  max block: 33554432
> +  available meta contexts: 1
> +   base:allocation

signature.asc
Description: OpenPGP digital signature