[Qemu-devel] [PATCH 0/6] Misc fixes to NBD

[Qemu-devel] [PATCH 0/6] Misc fixes to NBD

[Daniel P. Berrangé]

This does two minor fixes to the NBD code and adds significant coverage
of the NBD TLS support to detect future problems.

The first two patches should be for 3.1.

The tests can wait till 4.0 if desired.

Daniel P. Berrangé (6):
  nbd: fix whitespace in server error message
  nbd: stop waiting for a NBD response with NBD_CMD_DISC
  tests: pull qemu-nbd iotest helpers into common.nbd file
  tests: check if qemu-nbd is still alive before waiting
  tests: add iotests helpers for dealing with TLS certificates
  tests: exercise NBD server in TLS mode

 block/nbd-client.c            |   1 +
 nbd/server.c                  |   2 +-
 tests/qemu-iotests/058        |  47 ++----------
 tests/qemu-iotests/233        |  99 ++++++++++++++++++++++++
 tests/qemu-iotests/233.out    |  33 ++++++++
 tests/qemu-iotests/common.nbd | 111 +++++++++++++++++++++++++++
 tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group      |   1 +
 8 files changed, 393 insertions(+), 40 deletions(-)
 create mode 100755 tests/qemu-iotests/233
 create mode 100644 tests/qemu-iotests/233.out
 create mode 100644 tests/qemu-iotests/common.nbd
 create mode 100644 tests/qemu-iotests/common.tls

-- 
2.19.1

[Qemu-devel] [PATCH 1/6 for-3.1] nbd: fix whitespace in server error message

[Daniel P. Berrangé]

A space was missing after the option number was printed:

  Option 0x8not permitted before TLS

becomes

  Option 0x8 not permitted before TLS

This fixes

  commit 3668328303429f3bc93ab3365c66331600b06a2d
  Author: Eric Blake <eblake@redhat.com>
  Date:   Fri Oct 14 13:33:09 2016 -0500

    nbd: Send message along with server NBD_REP_ERR errors

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 nbd/server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 4e8f5ae51b..12e8139f95 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1135,7 +1135,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
 
             default:
                 ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
-                                   "Option 0x%" PRIx32
+                                   "Option 0x%" PRIx32 " "
                                    "not permitted before TLS", option);
                 /* Let the client keep trying, unless they asked to
                  * quit. In this mode, we've already sent an error, so
-- 
2.19.1

[Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Daniel P. Berrangé]

When sending a NBD_CMD_DISC message there is no reply expected,
however, the nbd_read_eof() coroutine is still waiting for a reply.
In a plain NBD connection this doesn't matter as it will just get an
EOF, however, on a TLS connection it will get an interrupted TLS data
packet. The nbd_read_eof() will then print an error message on the
console due to missing reply to NBD_CMD_DISC.

This can be seen with qemu-img

  $ qemu-img info \
      --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
      --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
  qemu-img: Cannot read from TLS channel: Input/output error
  image: nbd://127.0.0.1:9000
  file format: nbd
  virtual size: 10M (10485760 bytes)
  disk size: unavailable

Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
get the coroutine to stop waiting for a reply and thus supress the error
message. This fixes a regression introduced in

  commit be41c100c0d67f6072ddd4910c4b6f7d239f025f
  Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
  Date:   Fri May 26 14:09:13 2017 +0300

    nbd/client.c: use errp instead of LOG

    Move to modern errp scheme from just LOGging errors.

    Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
    Message-Id: <20170526110913.89098-1-vsementsov@virtuozzo.com>
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 block/nbd-client.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 76e9ca3abe..5f63e4b8f1 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -961,6 +961,7 @@ void nbd_client_close(BlockDriverState *bs)
     }
 
     nbd_send_request(client->ioc, &request);
+    client->quit = true;
 
     nbd_teardown_connection(bs);
 }
-- 
2.19.1

[Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file

[Daniel P. Berrangé]

The helpers for starting/stopping qemu-nbd in 058 will be useful in
other test cases, so move them into a common.nbd file.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/058        | 47 +++++------------------------
 tests/qemu-iotests/common.nbd | 56 +++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 39 deletions(-)
 create mode 100644 tests/qemu-iotests/common.nbd

diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058
index 5eb8784669..cd73250c48 100755
--- a/tests/qemu-iotests/058
+++ b/tests/qemu-iotests/058
@@ -29,55 +29,19 @@ echo "QA output created by $seq"
 here=`pwd`
 status=1	# failure is the default!
 
-nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
-nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
-rm -f "${TEST_DIR}/qemu-nbd.pid"
-
-_cleanup_nbd()
-{
-    local NBD_SNAPSHOT_PID
-    if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then
-        read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid"
-        rm -f "${TEST_DIR}/qemu-nbd.pid"
-        if [ -n "$NBD_SNAPSHOT_PID" ]; then
-            kill "$NBD_SNAPSHOT_PID"
-        fi
-    fi
-    rm -f "$nbd_unix_socket"
-}
-
-_wait_for_nbd()
-{
-    for ((i = 0; i < 300; i++))
-    do
-        if [ -r "$nbd_unix_socket" ]; then
-            return
-        fi
-        sleep 0.1
-    done
-    echo "Failed in check of unix socket created by qemu-nbd"
-    exit 1
-}
-
-converted_image=$TEST_IMG.converted
-
 _export_nbd_snapshot()
 {
-    _cleanup_nbd
-    $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l $1 &
-    _wait_for_nbd
+    nbd_server_start_unix_socket "$TEST_IMG" -l $1
 }
 
 _export_nbd_snapshot1()
 {
-    _cleanup_nbd
-    $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l snapshot.name=$1 &
-    _wait_for_nbd
+    nbd_server_start_unix_socket "$TEST_IMG" -l snapshot.name=$1
 }
 
 _cleanup()
 {
-    _cleanup_nbd
+    nbd_server_stop
     _cleanup_test_img
     rm -f "$converted_image"
 }
@@ -87,6 +51,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 . ./common.pattern
+. ./common.nbd
 
 _supported_fmt qcow2
 _supported_proto file
@@ -95,6 +60,10 @@ _require_command QEMU_NBD
 # Internal snapshots are (currently) impossible with refcount_bits=1
 _unsupported_imgopts 'refcount_bits=1[^0-9]'
 
+nbd_snapshot_img="nbd:unix:$nbd_unix_socket"
+
+converted_image=$TEST_IMG.converted
+
 # Use -f raw instead of -f $IMGFMT for the NBD connection
 QEMU_IO_NBD="$QEMU_IO -f raw --cache=$CACHEMODE"
 
diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
new file mode 100644
index 0000000000..f920a578f1
--- /dev/null
+++ b/tests/qemu-iotests/common.nbd
@@ -0,0 +1,56 @@
+#!/bin/bash
+# -*- shell-script-mode -*-
+#
+# Helpers for NBD server related config
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
+nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
+
+function nbd_server_stop()
+{
+    local NBD_PID
+    if [ -f "$nbd_pid_file" ]; then
+        read NBD_PID < "$nbd_pid_file"
+        rm -f "$nbd_pid_file"
+        if [ -n "$NBD_PID" ]; then
+            kill "$NBD_PID"
+        fi
+    fi
+    rm -f "$nbd_unix_socket"
+}
+
+function nbd_server_wait_for_unix_socket()
+{
+    for ((i = 0; i < 300; i++))
+    do
+        if [ -r "$nbd_unix_socket" ]; then
+            return
+        fi
+        sleep 0.1
+    done
+    echo "Failed in check of unix socket created by qemu-nbd"
+    exit 1
+}
+
+function nbd_server_start_unix_socket()
+{
+    nbd_server_stop
+    $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
+    nbd_server_wait_for_unix_socket
+}
-- 
2.19.1

[Qemu-devel] [PATCH 4/6] tests: check if qemu-nbd is still alive before waiting

[Daniel P. Berrangé]

If the qemu-nbd UNIX socket has not shown up, the tests will sleep a bit
and then check again repeatedly for upto 30 seconds. This is pointless
if the qemu-nbd process has quit due to an error, so check whether the
pid is still alive before waiting and retrying.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/common.nbd | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index f920a578f1..61e9e90fee 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -37,11 +37,19 @@ function nbd_server_stop()
 
 function nbd_server_wait_for_unix_socket()
 {
+    pid=$1
+
     for ((i = 0; i < 300; i++))
     do
         if [ -r "$nbd_unix_socket" ]; then
             return
         fi
+        kill -s 0 $pid 2>/dev/null
+        if test $? != 0
+        then
+            echo "qemu-nbd unexpectedly quit"
+            exit 1
+        fi
         sleep 0.1
     done
     echo "Failed in check of unix socket created by qemu-nbd"
@@ -52,5 +60,5 @@ function nbd_server_start_unix_socket()
 {
     nbd_server_stop
     $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
-    nbd_server_wait_for_unix_socket
+    nbd_server_wait_for_unix_socket $!
 }
-- 
2.19.1

[Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Daniel P. Berrangé]

Add helpers to common.tls for creating TLS certificates for a CA,
server and client.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)
 create mode 100644 tests/qemu-iotests/common.tls

diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
new file mode 100644
index 0000000000..6178ca5764
--- /dev/null
+++ b/tests/qemu-iotests/common.tls
@@ -0,0 +1,139 @@
+#!/bin/bash
+#
+# Helpers for TLS related config
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+tls_dir="${TEST_DIR}/tls"
+
+function tls_x509_cleanup()
+{
+    rm -f ${tls_dir}/*.pem
+    rm -f ${tls_dir}/*/*.pem
+    rmdir ${tls_dir}/*
+    rmdir ${tls_dir}
+}
+
+
+function tls_x509_init()
+{
+    mkdir "${tls_dir}"
+
+    # use a fixed key so we don't waste system entropy on
+    # each test run
+    cat > ${tls_dir}/key.pem <<EOF
+-----BEGIN PRIVATE KEY-----
+MIICdQIBADANBgkqhkiG9w0BAQEFAASCAl8wggJbAgEAAoGBALVcr
+BL40Tm6yq88FBhJNw1aaoCjmtg0l4dWQZ/e9Fimx4ARxFpT+ji4FE
+Cgl9s/SGqC+1nvlkm9ViSo0j7MKDbnDB+VRHDvMAzQhA2X7e8M0n9
+rPolUY2lIVC83q0BBaOBkCj2RSmT2xTEbbC2xLukSrg2WP/ihVOxc
+kXRuyFtzAgMBAAECgYB7slBexDwXrtItAMIH6m/U+LUpNe0Xx48OL
+IOn4a4whNgO/o84uIwygUK27ZGFZT0kAGAk8CdF9hA6ArcbQ62s1H
+myxrUbF9/mrLsQw1NEqpuUk9Ay2Tx5U/wPx35S3W/X2AvR/ZpTnCn
+2q/7ym9fyiSoj86drD7BTvmKXlOnOwQJBAPOFMp4mMa9NGpGuEssO
+m3Uwbp6lhcP0cA9MK+iOmeANpoKWfBdk5O34VbmeXnGYWEkrnX+9J
+bM4wVhnnBWtgBMCQQC+qAEmvwcfhauERKYznMVUVksyeuhxhCe7EK
+mPh+U2+g0WwdKvGDgO0PPt1gq0ILEjspMDeMHVdTwkaVBo/uMhAkA
+Z5SsZyCP2aTOPFDypXRdI4eqRcjaEPOUBq27r3uYb/jeboVb2weLa
+L1MmVuHiIHoa5clswPdWVI2y0em2IGoDAkBPSp/v9VKJEZabk9Frd
+a+7u4fanrM9QrEjY3KhduslSilXZZSxrWjjAJPyPiqFb3M8XXA26W
+nz1KYGnqYKhLcBAkB7dt57n9xfrhDpuyVEv+Uv1D3VVAhZlsaZ5Pp
+dcrhrkJn2sa/+O8OKvdrPSeeu/N5WwYhJf61+CPoenMp7IFci
+-----END PRIVATE KEY-----
+EOF
+}
+
+
+function tls_x509_create_root_ca()
+{
+    name=$1
+
+    test -z "$name" && name=ca-cert
+
+    cat > ${tls_dir}/ca.info <<EOF
+cn = Cthulu Dark Lord Enterprises $name
+ca
+cert_signing_key
+EOF
+
+    certtool --generate-self-signed \
+             --load-privkey ${tls_dir}/key.pem \
+             --template ${tls_dir}/ca.info \
+             --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1
+
+    rm -f ${tls_dir}/ca.info
+}
+
+
+function tls_x509_create_server()
+{
+    caname=$1
+    name=$2
+
+    mkdir ${tls_dir}/$name
+    cat > ${tls_dir}/cert.info <<EOF
+organization = Cthulu Dark Lord Enterprises $name
+cn = localhost
+dns_name = localhost
+dns_name = localhost.localdomain
+ip_address = 127.0.0.1
+ip_address = ::1
+tls_www_server
+encryption_key
+signing_key
+EOF
+
+    certtool --generate-certificate \
+             --load-ca-privkey ${tls_dir}/key.pem \
+             --load-ca-certificate ${tls_dir}/$caname-cert.pem \
+             --load-privkey ${tls_dir}/key.pem \
+             --template ${tls_dir}/cert.info \
+             --outfile ${tls_dir}/$name/server-cert.pem 2>&1 | head -1
+    ln -s ${tls_dir}/$caname-cert.pem ${tls_dir}/$name/ca-cert.pem
+    ln -s ${tls_dir}/key.pem ${tls_dir}/$name/server-key.pem
+
+    rm -f ${tls_dir}/cert.info
+}
+
+
+function tls_x509_create_client()
+{
+    caname=$1
+    name=$2
+
+    mkdir ${tls_dir}/$name
+    cat > ${tls_dir}/cert.info <<EOF
+country = South Pacific
+locality =  R'lyeh
+organization = Cthulu Dark Lord Enterprises $name
+cn = localhost
+tls_www_client
+encryption_key
+signing_key
+EOF
+
+    certtool --generate-certificate \
+             --load-ca-privkey ${tls_dir}/key.pem \
+             --load-ca-certificate ${tls_dir}/$caname-cert.pem \
+             --load-privkey ${tls_dir}/key.pem \
+             --template  ${tls_dir}/cert.info \
+             --outfile ${tls_dir}/$name/client-cert.pem 2>&1 | head -1
+    ln -s ${tls_dir}/$caname-cert.pem ${tls_dir}/$name/ca-cert.pem
+    ln -s ${tls_dir}/key.pem ${tls_dir}/$name/client-key.pem
+
+    rm -f  ${tls_dir}/cert.info
+}
-- 
2.19.1

[Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P. Berrangé]

Add tests that validate it is possible to connect to an NBD server
running TLS mode. Also test mis-matched TLS vs non-TLS connections
correctly fail.
---
 tests/qemu-iotests/233        | 99 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/233.out    | 33 ++++++++++++
 tests/qemu-iotests/common.nbd | 47 +++++++++++++++++
 tests/qemu-iotests/group      |  1 +
 4 files changed, 180 insertions(+)
 create mode 100755 tests/qemu-iotests/233
 create mode 100644 tests/qemu-iotests/233.out

diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
new file mode 100755
index 0000000000..8f3d5bd013
--- /dev/null
+++ b/tests/qemu-iotests/233
@@ -0,0 +1,99 @@
+#!/bin/bash
+#
+# Test NBD TLS certificate / authorization integration
+#
+# Copyright (C) 2018 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=berrange@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1	# failure is the default!
+
+_cleanup()
+{
+    nbd_server_stop
+    _cleanup_test_img
+    tls_x509_cleanup
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.pattern
+. ./common.tls
+. ./common.nbd
+
+_supported_fmt raw qcow2
+_supported_proto file
+_supported_os Linux
+_require_command QEMU_NBD
+
+nbd_server_set_tcp_port
+tls_x509_init
+
+echo
+echo "== preparing TLS creds =="
+
+tls_x509_create_root_ca "ca1"
+tls_x509_create_root_ca "ca2"
+tls_x509_create_server "ca1" "server1"
+tls_x509_create_client "ca1" "client1"
+tls_x509_create_client "ca2" "client2"
+
+echo
+echo "== preparing image =="
+_make_test_img 64M
+
+
+echo
+echo "== check TLS client to plain server fails =="
+nbd_server_start_tcp_socket "$TEST_IMG"
+
+$QEMU_IMG info --image-opts \
+    --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
+    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
+
+nbd_server_stop
+
+echo
+echo "== check plain client to TLS server fails =="
+
+nbd_server_start_tcp_socket --object tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes --tls-creds tls0 "$TEST_IMG"
+
+$QEMU_IMG info nbd://localhost:$nbd_tcp_port
+
+echo
+echo "== check TLS works =="
+$QEMU_IMG info --image-opts \
+    --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
+    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
+
+echo
+echo "== check TLS with different CA fails =="
+$QEMU_IMG info --image-opts \
+    --object tls-creds-x509,dir=${tls_dir}/client2,endpoint=client,id=tls0 \
+    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
new file mode 100644
index 0000000000..eaa410c270
--- /dev/null
+++ b/tests/qemu-iotests/233.out
@@ -0,0 +1,33 @@
+QA output created by 233
+
+== preparing TLS creds ==
+Generating a self signed certificate...
+Generating a self signed certificate...
+Generating a signed certificate...
+Generating a signed certificate...
+Generating a signed certificate...
+
+== preparing image ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+
+== check TLS client to plain server fails ==
+option negotiation failed: read failed: Unexpected end-of-file before all bytes were read
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for option 5 (starttls)
+server reported: TLS not configured
+
+== check plain client to TLS server fails ==
+option negotiation failed: read failed: Unexpected end-of-file before all bytes were read
+qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required before option 8 (structured reply)
+server reported: Option 0x8 not permitted before TLS
+write failed (error message): Unable to write to socket: Broken pipe
+
+== check TLS works ==
+image: nbd://127.0.0.1:10809
+file format: nbd
+virtual size: 64M (67108864 bytes)
+disk size: unavailable
+
+== check TLS with different CA fails ==
+option negotiation failed: Verify failed: No certificate was found.
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': The certificate hasn't got a known issuer
+*** done
diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 61e9e90fee..0483ea7c55 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -20,6 +20,7 @@
 #
 
 nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
+nbd_tcp_addr="127.0.0.1"
 nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
 
 function nbd_server_stop()
@@ -62,3 +63,49 @@ function nbd_server_start_unix_socket()
     $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
     nbd_server_wait_for_unix_socket $!
 }
+
+function nbd_server_set_tcp_port()
+{
+    for port in `seq 10809 10909`
+    do
+	socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1
+        if test $? != 0
+	then
+	    nbd_tcp_port=$port
+            return
+        fi
+    done
+
+    echo "Cannot find free TCP port for nbd in range 10809-10909"
+    exit 1
+}
+
+function nbd_server_wait_for_tcp_socket()
+{
+    pid=$1
+
+    for ((i = 0; i < 300; i++))
+    do
+        socat TCP:localhost:$nbd_tcp_port STDIO < /dev/null 1>/dev/null 2>&1
+        if test $? == 0
+	then
+            return
+        fi
+        kill -s 0 $pid 2>/dev/null
+        if test $? != 0
+        then
+            echo "qemu-nbd unexpectedly quit"
+            exit 1
+        fi
+        sleep 0.1
+    done
+    echo "Failed in check of unix socket created by qemu-nbd"
+    exit 1
+}
+
+function nbd_server_start_tcp_socket()
+{
+    nbd_server_stop
+    $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port $@ &
+    nbd_server_wait_for_tcp_socket $!
+}
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index ebe4fe78bc..e4327efa42 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -228,3 +228,4 @@
 229 auto quick
 231 auto quick
 232 auto quick
+233 auto quick
-- 
2.19.1

Re: [Qemu-devel] [PATCH 1/6 for-3.1] nbd: fix whitespace in server error message

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> A space was missing after the option number was printed:
> 
>    Option 0x8not permitted before TLS
> 
> becomes
> 
>    Option 0x8 not permitted before TLS
> 
> This fixes
> 
>    commit 3668328303429f3bc93ab3365c66331600b06a2d
>    Author: Eric Blake <eblake@redhat.com>
>    Date:   Fri Oct 14 13:33:09 2016 -0500
> 
>      nbd: Send message along with server NBD_REP_ERR errors
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   nbd/server.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 4e8f5ae51b..12e8139f95 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1135,7 +1135,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>   
>               default:
>                   ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
> -                                   "Option 0x%" PRIx32
> +                                   "Option 0x%" PRIx32 " "
>                                      "not permitted before TLS", option);

Visually, I'd include the space in the next line instead of on its own 
(but that's aesthetics, not semantic). Agree that this is 3.1 material; 
I'll queue it up and send a PR on Monday.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> When sending a NBD_CMD_DISC message there is no reply expected,
> however, the nbd_read_eof() coroutine is still waiting for a reply.
> In a plain NBD connection this doesn't matter as it will just get an
> EOF, however, on a TLS connection it will get an interrupted TLS data
> packet.

Does that depend on whether the server tried to gracefully close the TLS 
connection (and flushed enough packets to do so)?  At any rate, while 
it's nice to wait for the server to cleanly end TLS, we can't control 
the behavior of all servers, and I agree with your analysis that having 
the server silently deal with an abrupt shutdown is nicer than polluting 
stderr, as it's not actually a data loss situation for either the client 
or the server.

> The nbd_read_eof() will then print an error message on the
> console due to missing reply to NBD_CMD_DISC.
> 
> This can be seen with qemu-img
> 
>    $ qemu-img info \
>        --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
>        --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
>    qemu-img: Cannot read from TLS channel: Input/output error
>    image: nbd://127.0.0.1:9000
>    file format: nbd
>    virtual size: 10M (10485760 bytes)
>    disk size: unavailable
> 
> Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
> get the coroutine to stop waiting for a reply and thus supress the error

s/supress/suppress/

> message. This fixes a regression introduced in
> 
>    commit be41c100c0d67f6072ddd4910c4b6f7d239f025f
>    Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>    Date:   Fri May 26 14:09:13 2017 +0300
> 
>      nbd/client.c: use errp instead of LOG
> 
>      Move to modern errp scheme from just LOGging errors.
> 
>      Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>      Message-Id: <20170526110913.89098-1-vsementsov@virtuozzo.com>
>      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   block/nbd-client.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

Agree that this is 3.1 material; will queue.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> The helpers for starting/stopping qemu-nbd in 058 will be useful in
> other test cases, so move them into a common.nbd file.

In fact, I already have a proposed patch 228 that I'm trying to clean up 
for potential inclusion in 3.1 that could use this!  [I'm still 
hammering on those patches, so I don't know if they will make 3.1 or be 
delayed to 4.0 - this patch is not needed in 3.1 unless either your 
later patches are 3.1 material or my patch ends up ready in time]

https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg00308.html

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/058        | 47 +++++------------------------
>   tests/qemu-iotests/common.nbd | 56 +++++++++++++++++++++++++++++++++++
>   2 files changed, 64 insertions(+), 39 deletions(-)
>   create mode 100644 tests/qemu-iotests/common.nbd
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 4/6] tests: check if qemu-nbd is still alive before waiting

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> If the qemu-nbd UNIX socket has not shown up, the tests will sleep a bit
> and then check again repeatedly for upto 30 seconds. This is pointless

s/upto/up to/

> if the qemu-nbd process has quit due to an error, so check whether the
> pid is still alive before waiting and retrying.

"But our tests are perfect and qemu-nbd never fails" :)

Yes, this makes sense.  Not 3.1 material on its own (after all, our 
testsuite isn't showing such failures, so we aren't wasting that time at 
the moment) - but worth including if the later patches end up in 3.1.

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/common.nbd | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> index f920a578f1..61e9e90fee 100644
> --- a/tests/qemu-iotests/common.nbd
> +++ b/tests/qemu-iotests/common.nbd
> @@ -37,11 +37,19 @@ function nbd_server_stop()
>   
>   function nbd_server_wait_for_unix_socket()
>   {
> +    pid=$1
> +
>       for ((i = 0; i < 300; i++))
>       do
>           if [ -r "$nbd_unix_socket" ]; then
>               return
>           fi
> +        kill -s 0 $pid 2>/dev/null
> +        if test $? != 0
> +        then
> +            echo "qemu-nbd unexpectedly quit"
> +            exit 1

Maybe the echo should be redirected to stderr.  But we aren't 
consistently doing that in other tests (_init_error does it, but other 
spots in check are not), so I'm not changing it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> Add helpers to common.tls for creating TLS certificates for a CA,
> server and client.

MUCH appreciated!  We NEED this coverage, easily automated.

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++
>   1 file changed, 139 insertions(+)
>   create mode 100644 tests/qemu-iotests/common.tls
> 
> diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
> new file mode 100644

I was a bit surprised that this wasn't 100755, but this matches the fact 
that none of the other common.* are executable. And after thinking more, 
it makes sense - they aren't standalone scripts, but designed to be 
sourced, and 'source' doesn't care about execute bits.

> +tls_dir="${TEST_DIR}/tls"
> +
> +function tls_x509_cleanup()
> +{
> +    rm -f ${tls_dir}/*.pem
> +    rm -f ${tls_dir}/*/*.pem
> +    rmdir ${tls_dir}/*
> +    rmdir ${tls_dir}

Why not just:
rm -rf $tls_dir

Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain 
spaces, then all uses of ${tls_dir} need to be in "".

> +}
> +
> +
> +function tls_x509_init()
> +{
> +    mkdir "${tls_dir}"

And this just highlights the quoting inconsistency.  Should this use 
mkdir -p?

> +
> +function tls_x509_create_root_ca()
> +{
> +    name=$1
> +
> +    test -z "$name" && name=ca-cert

Could also be shortened as:

name=${1:-ca-cert}

> +
> +    cat > ${tls_dir}/ca.info <<EOF
> +cn = Cthulu Dark Lord Enterprises $name

s/Cthulu/Cthulhu/ - after all, we don't want him coming after us just 
because we botched the spelling of his name :)

> +ca
> +cert_signing_key
> +EOF
> +
> +    certtool --generate-self-signed \
> +             --load-privkey ${tls_dir}/key.pem \
> +             --template ${tls_dir}/ca.info \
> +             --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1

More missing ""

> +
> +    rm -f ${tls_dir}/ca.info
> +}
> +
> +
> +function tls_x509_create_server()
> +{
> +    caname=$1
> +    name=$2
> +
> +    mkdir ${tls_dir}/$name
> +    cat > ${tls_dir}/cert.info <<EOF
> +organization = Cthulu Dark Lord Enterprises $name

Matched spelling

> +function tls_x509_create_client()
> +{
> +    caname=$1
> +    name=$2
> +
> +    mkdir ${tls_dir}/$name
> +    cat > ${tls_dir}/cert.info <<EOF
> +country = South Pacific
> +locality =  R'lyeh
> +organization = Cthulu Dark Lord Enterprises $name

And again

Needs several touch-ups, but the idea itself is sound.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> Add tests that validate it is possible to connect to an NBD server
> running TLS mode. Also test mis-matched TLS vs non-TLS connections
> correctly fail.
> ---
>   tests/qemu-iotests/233        | 99 +++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/233.out    | 33 ++++++++++++
>   tests/qemu-iotests/common.nbd | 47 +++++++++++++++++
>   tests/qemu-iotests/group      |  1 +
>   4 files changed, 180 insertions(+)
>   create mode 100755 tests/qemu-iotests/233
>   create mode 100644 tests/qemu-iotests/233.out

Adding tests is non-invasive, so I have no objection to taking the 
entire series in 3.1.  Do you want me to touch up the nits I found 
earlier, or should I wait for a v2?

Reviewed-by: Eric Blake <eblake@redhat.com>

> +# creator
> +owner=berrange@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +here=`pwd`

Dead code. Mao Zhongyi has a patch to remove it.  I'm thinking of 
putting his patches in first, and then yours.


> +
> +echo
> +echo "== check TLS client to plain server fails =="
> +nbd_server_start_tcp_socket "$TEST_IMG"

The negative testing is just as important as positive testing, to prove 
TLS is adding the promised authorization.

> +
> +$QEMU_IMG info --image-opts \
> +    --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
> +    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
> +
> +nbd_server_stop
> +
> +echo
> +echo "== check plain client to TLS server fails =="
> +
> +nbd_server_start_tcp_socket --object tls-creds-x509,dir=${tls_dir}/server1,endpoint=server,id=tls0,verify-peer=yes --tls-creds tls0 "$TEST_IMG"

Long line

> +
> +$QEMU_IMG info nbd://localhost:$nbd_tcp_port
> +
> +echo
> +echo "== check TLS works =="
> +$QEMU_IMG info --image-opts \
> +    --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
> +    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0

Is it also worth reading or even writing, to ensure that more than just 
the handshake is working?

> +
> +echo
> +echo "== check TLS with different CA fails =="
> +$QEMU_IMG info --image-opts \
> +    --object tls-creds-x509,dir=${tls_dir}/client2,endpoint=client,id=tls0 \
> +    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
> +
> +# success, all done

> +== check TLS client to plain server fails ==
> +option negotiation failed: read failed: Unexpected end-of-file before all bytes were read

Annoying message; I wonder if we can clean that up. But not this patch's 
problem.

> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for option 5 (starttls)
> +server reported: TLS not configured

This 2-line message is better (and as such, explains why I think the 
message about early EOF should be silenced in this case).

> +
> +== check plain client to TLS server fails ==
> +option negotiation failed: read failed: Unexpected end-of-file before all bytes were read
> +qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required before option 8 (structured reply)
> +server reported: Option 0x8 not permitted before TLS

Same story about a redundant message.

> +write failed (error message): Unable to write to socket: Broken pipe

Hmm - is this the client complaining that it can't send more to the 
server (that's a bug if the server hung up, since the protocol allows 
the client to change its mind and try TLS after all), or the server 
complaining that the client hung up abruptly without sending 
NBD_OPT_ABORT? Qemu as client either tries TLS right away, or knows that 
if the server requires TLS and the client has no TLS credentials then 
the client will never succeed, so the client gives up - but it SHOULD be 
giving up with NBD_OPT_ABORT rather than just disconnecting.  I'll have 
to play with that.  Doesn't impact your patch, but might spur a followup 
fix :)

> +
> +== check TLS works ==
> +image: nbd://127.0.0.1:10809
> +file format: nbd
> +virtual size: 64M (67108864 bytes)
> +disk size: unavailable
> +

Again, also doing a read and/or write to ensure the encrypted data is 
handled correctly would be nice. Can be in a followup patch.

> +== check TLS with different CA fails ==
> +option negotiation failed: Verify failed: No certificate was found.
> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': The certificate hasn't got a known issuer
> +*** done
> diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> index 61e9e90fee..0483ea7c55 100644
> --- a/tests/qemu-iotests/common.nbd
> +++ b/tests/qemu-iotests/common.nbd
> @@ -20,6 +20,7 @@
>   #
>   
>   nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
> +nbd_tcp_addr="127.0.0.1"

Should this be in your earlier patch that created common.nbd? Maybe not, 
as it doesn't get used until the functions you add here for tcp support. 
Still, I wonder if we should split the addition of tcp support separate 
from the new test.

>   nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
>   
>   function nbd_server_stop()
> @@ -62,3 +63,49 @@ function nbd_server_start_unix_socket()
>       $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
>       nbd_server_wait_for_unix_socket $!
>   }
> +
> +function nbd_server_set_tcp_port()
> +{
> +    for port in `seq 10809 10909`
> +    do
> +	socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1

This is the first use of socat in iotests.  Might not be the most 
portable, but I don't know if I have better ideas. 
nbdkit.git/tests/test-ip.sh greps the output of 'ss -ltn' to locate free 
ports, but I don't know if ss is any better than socat.

> +        if test $? != 0
> +	then
> +	    nbd_tcp_port=$port
> +            return
> +        fi
> +    done
> +
> +    echo "Cannot find free TCP port for nbd in range 10809-10909"
> +    exit 1

Should we skip instead of fail in this case? Would we do well picking a 
larger port range?


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> The helpers for starting/stopping qemu-nbd in 058 will be useful in
> other test cases, so move them into a common.nbd file.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qemu-iotests/058        | 47 +++++------------------------
>   tests/qemu-iotests/common.nbd | 56 +++++++++++++++++++++++++++++++++++
>   2 files changed, 64 insertions(+), 39 deletions(-)
>   create mode 100644 tests/qemu-iotests/common.nbd
> 

> -
> -_cleanup_nbd()
> -{

> -_wait_for_nbd()
> -{

> +++ b/tests/qemu-iotests/common.nbd
> @@ -0,0 +1,56 @@
> +#!/bin/bash

I know we're using bash,

> +
> +function nbd_server_stop()
> +{

> +function nbd_server_wait_for_unix_socket()

and bash supports 'function', but it is an obsolete syntactic sugar 
thing that I don't recommend using.  (In ksh, it actually makes a 
difference in behavior whether you use 'function' or not, and using it 
in 'bash' makes it harder to port code over to 'ksh' - and hence in bash 
it is obsolete because here it does NOT cause the change in behavior 
that ksh users expect)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file

[Eric Blake]

On 11/16/18 3:41 PM, Eric Blake wrote:

>> +#!/bin/bash
> 
> I know we're using bash,
> 
>> +
>> +function nbd_server_stop()
>> +{
> 
>> +function nbd_server_wait_for_unix_socket()
> 
> and bash supports 'function', but it is an obsolete syntactic sugar 
> thing that I don't recommend using.  (In ksh, it actually makes a 
> difference in behavior whether you use 'function' or not, and using it 
> in 'bash' makes it harder to port code over to 'ksh' - and hence in bash 
> it is obsolete because here it does NOT cause the change in behavior 
> that ksh users expect)
> 

Of course, I hit send too soon, before getting to my punchline:

Since we already have so many existing iotests that use 'function', it's 
better to clean that up as a separate patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> Add tests that validate it is possible to connect to an NBD server
> running TLS mode. Also test mis-matched TLS vs non-TLS connections
> correctly fail.
> ---

Missing your Signed-off-by. Can you please supply that, so I can include 
this in my pull request?

Also, I'm getting failures when trying to test it:

@@ -17,9 +17,9 @@

  == check plain client to TLS server fails ==
  option negotiation failed: read failed: Unexpected end-of-file before 
all bytes were read
+write failed (error message): Unable to write to socket: Broken pipe
  qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation 
required before option 8 (structured reply)
  server reported: Option 0x8 not permitted before TLS
-write failed (error message): Unable to write to socket: Broken pipe


which looks like an output race. :(

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/16/18 11:20 AM, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
>> Add tests that validate it is possible to connect to an NBD server
>> running TLS mode. Also test mis-matched TLS vs non-TLS connections
>> correctly fail.
>> ---

>> +== check TLS client to plain server fails ==
>> +option negotiation failed: read failed: Unexpected end-of-file before 
>> all bytes were read
> 
> Annoying message; I wonder if we can clean that up. But not this patch's 
> problem.
> 

Actually, I tracked this message down to using socat (which actually 
connects and then abruptly exits) when probing whether the socket is up 
and listening.  That is, the message is being produced as a side effect 
of nbd_server_wait_for_tcp_socket rather than during the actual 
$QEMU_IMG command we are interested in testing.


>>   nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
>>   function nbd_server_stop()
>> @@ -62,3 +63,49 @@ function nbd_server_start_unix_socket()
>>       $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
>>       nbd_server_wait_for_unix_socket $!
>>   }
>> +
>> +function nbd_server_set_tcp_port()
>> +{
>> +    for port in `seq 10809 10909`
>> +    do
>> +    socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1
> 
> This is the first use of socat in iotests.  Might not be the most 
> portable, but I don't know if I have better ideas. 
> nbdkit.git/tests/test-ip.sh greps the output of 'ss -ltn' to locate free 
> ports, but I don't know if ss is any better than socat.

So, I'm planning to squash this in, to use ss instead of socat, as follows:

diff --git i/tests/qemu-iotests/common.nbd w/tests/qemu-iotests/common.nbd
index 0483ea7c55a..d73af285abd 100644
--- i/tests/qemu-iotests/common.nbd
+++ w/tests/qemu-iotests/common.nbd
@@ -66,12 +66,12 @@ function nbd_server_start_unix_socket()

  function nbd_server_set_tcp_port()
  {
-    for port in `seq 10809 10909`
+    (ss --help) >/dev/null 2>&1 || _notrun "ss utility not found, 
skipping test"
+
+    for ((port = 10809; port <= 10909; port++))
      do
-	socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1
-        if test $? != 0
-	then
-	    nbd_tcp_port=$port
+        if ! ss -tln | grep -sqE ":$port\b"; then
+            nbd_tcp_port=$port
              return
          fi
      done
@@ -86,9 +86,7 @@ function nbd_server_wait_for_tcp_socket()

      for ((i = 0; i < 300; i++))
      do
-        socat TCP:localhost:$nbd_tcp_port STDIO < /dev/null 1>/dev/null 
2>&1
-        if test $? == 0
-	then
+        if ss -tln | grep -sqE ":$nbd_tcp_port\b"; then
              return
          fi
          kill -s 0 $pid 2>/dev/null
diff --git i/tests/qemu-iotests/233.out w/tests/qemu-iotests/233.out
index eaa410c2703..eb4077f9fd7 100644
--- i/tests/qemu-iotests/233.out
+++ w/tests/qemu-iotests/233.out
@@ -11,12 +11,10 @@ Generating a signed certificate...
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864

  == check TLS client to plain server fails ==
-option negotiation failed: read failed: Unexpected end-of-file before 
all bytes were read
  qemu-img: Could not open 
'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server 
for option 5 (starttls)
  server reported: TLS not configured

  == check plain client to TLS server fails ==
-option negotiation failed: read failed: Unexpected end-of-file before 
all bytes were read
  qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation 
required before option 8 (structured reply)
  server reported: Option 0x8 not permitted before TLS
  write failed (error message): Unable to write to socket: Broken pipe


Also, you have to sanitize 233.out to change 10809 into PORT, so the 
test can still pass when it picked a different port.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/17/18 2:49 PM, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
>> Add tests that validate it is possible to connect to an NBD server
>> running TLS mode. Also test mis-matched TLS vs non-TLS connections
>> correctly fail.
>> ---
> 
> Missing your Signed-off-by. Can you please supply that, so I can include 
> this in my pull request?
> 
> Also, I'm getting failures when trying to test it:
> 
> @@ -17,9 +17,9 @@
> 
>   == check plain client to TLS server fails ==
>   option negotiation failed: read failed: Unexpected end-of-file before 
> all bytes were read
> +write failed (error message): Unable to write to socket: Broken pipe
>   qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation 
> required before option 8 (structured reply)
>   server reported: Option 0x8 not permitted before TLS
> -write failed (error message): Unable to write to socket: Broken pipe
> 
> 
> which looks like an output race. :(

Found and squashed it - commit 37ec36f6 fixed plaintext servers to not 
be noisy for NBD_OPT_ABORT, but did not give equal treatment to TLS 
servers. Patch coming up separately.

So, with my fixes, I can add:

Tested-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH 1.5/6] nbd/server: Ignore write errors when replying to NBD_OPT_ABORT

[Eric Blake]

Commit 37ec36f6 intentionally ignores errors when trying to reply
to an NBD_OPT_ABORT request for plaintext clients, but did not make
the same change for a TLS server.  Since NBD_OPT_ABORT is
documented as being a potential for an EPIPE when the client hangs
up without waiting for our reply, we don't need to pollute the
server's output with that failure.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 nbd/server.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 056cfa5ad47..dc04513de70 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1134,12 +1134,16 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
                 return -EINVAL;

             default:
-                ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
+                /* Let the client keep trying, unless they asked to
+                 * quit. Always try to give an error back to the
+                 * client; but when replying to OPT_ABORT, be aware
+                 * that the client may hang up before receiving the
+                 * error, in which case we are fine ignoring the
+                 * resulting EPIPE. */
+                ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD,
+                                   option == NBD_OPT_ABORT ? NULL : errp,
                                    "Option 0x%" PRIx32
                                    " not permitted before TLS", option);
-                /* Let the client keep trying, unless they asked to
-                 * quit. In this mode, we've already sent an error, so
-                 * we can't ack the abort.  */
                 if (option == NBD_OPT_ABORT) {
                     return 1;
                 }
-- 
2.17.2

Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> When sending a NBD_CMD_DISC message there is no reply expected,
> however, the nbd_read_eof() coroutine is still waiting for a reply.
> In a plain NBD connection this doesn't matter as it will just get an
> EOF, however, on a TLS connection it will get an interrupted TLS data
> packet. The nbd_read_eof() will then print an error message on the
> console due to missing reply to NBD_CMD_DISC.
> 
> This can be seen with qemu-img
> 
>    $ qemu-img info \
>        --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
>        --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
>    qemu-img: Cannot read from TLS channel: Input/output error
>    image: nbd://127.0.0.1:9000
>    file format: nbd
>    virtual size: 10M (10485760 bytes)
>    disk size: unavailable
> 
> Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
> get the coroutine to stop waiting for a reply and thus supress the error
> message.

Actually, it's not quite enough - once you actually start performing 
I/O, enough coroutines are kicked off that the error still happens:

$  qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
--object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\
  driver=nbd,host=localhost,port=10809,tls-creds=tls0
read 1048576/1048576 bytes at offset 1048576
1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec)
wrote 1048576/1048576 bytes at offset 1048576
1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec)
Cannot read from TLS channel: Input/output error

Squashing this in on top of your patch helps, though:

diff --git i/block/nbd-client.c w/block/nbd-client.c
index 5f63e4b8f15..e7916c78996 100644
--- i/block/nbd-client.c
+++ w/block/nbd-client.c
@@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void 
*opaque)
          assert(s->reply.handle == 0);
          ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
          if (local_err) {
-            error_report_err(local_err);
+            /* If we are already quitting, either another error has
+             * already been reported, or we requested NBD_CMD_DISC and
+             * don't need to report anything further.  */
+            if (!s->quit) {
+                error_report_err(local_err);
+            } else {
+                error_free(local_err);
+            }
          }
          if (ret <= 0) {
              break;

But I want to do more testing to make sure I'm not missing out on 
reporting an actual error if I add that.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[Qemu-devel] [PATCH 7/6] iotests: Also test I/O over NBD TLS

[Eric Blake]

Enhance test 233 to also perform I/O beyond the initial handshake.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

Depends on my tweak to 2/6 to suppress an EIO error message
on a failed read after NBD_CMD_DISC.

 tests/qemu-iotests/233     | 12 +++++++++++-
 tests/qemu-iotests/233.out | 10 ++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/233 b/tests/qemu-iotests/233
index a1ba8c09c06..5b6982be6ad 100755
--- a/tests/qemu-iotests/233
+++ b/tests/qemu-iotests/233
@@ -61,7 +61,7 @@ tls_x509_create_client "ca2" "client2"
 echo
 echo "== preparing image =="
 _make_test_img 64M
-
+$QEMU_IO -c 'w -P 0x11 1m 1m' "$TEST_IMG" | _filter_qemu_io

 echo
 echo "== check TLS client to plain server fails =="
@@ -95,6 +95,16 @@ $QEMU_IMG info --image-opts \
     driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
     2>&1 | sed "s/$nbd_tcp_port/PORT/g"

+echo
+echo "== perform I/O over TLS =="
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+$QEMU_IO -c 'r -P 0x11 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
+    --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
+    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0 \
+    2>&1 | sed "s/$nbd_tcp_port/PORT/g" | _filter_qemu_io
+
+$QEMU_IO -f qcow2 -r -U -c 'r -P 0x22 1m 1m' "$TEST_IMG" | _filter_qemu_io
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index 616e9238c89..94acd9b9479 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -9,6 +9,8 @@ Generating a signed certificate...

 == preparing image ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

 == check TLS client to plain server fails ==
 qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Denied by server for option 5 (starttls)
@@ -27,4 +29,12 @@ disk size: unavailable
 == check TLS with different CA fails ==
 option negotiation failed: Verify failed: No certificate was found.
 qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': The certificate hasn't got a known issuer
+
+== perform I/O over TLS ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.17.2

Re: [Qemu-devel] [PATCH 0/6] Misc fixes to NBD

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> This does two minor fixes to the NBD code and adds significant coverage
> of the NBD TLS support to detect future problems.
> 
> The first two patches should be for 3.1.
> 
> The tests can wait till 4.0 if desired.
> 
> Daniel P. Berrangé (6):
>    nbd: fix whitespace in server error message
>    nbd: stop waiting for a NBD response with NBD_CMD_DISC
>    tests: pull qemu-nbd iotest helpers into common.nbd file
>    tests: check if qemu-nbd is still alive before waiting
>    tests: add iotests helpers for dealing with TLS certificates
>    tests: exercise NBD server in TLS mode
> 

I'm still missing your S-o-b on 6. I've posted a preliminary version of 
your series with my touchups incorporated, if you'd like to double check 
it, at:

https://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/nbd


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> The helpers for starting/stopping qemu-nbd in 058 will be useful in
> other test cases, so move them into a common.nbd file.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---

> +function nbd_server_start_unix_socket()
> +{
> +    nbd_server_stop
> +    $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &

Needs to be "$@" to properly preserve whitespace and/or empty arguments 
(the latter if someone passes -x '' for a default-named export).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Daniel P. Berrangé]

On Sat, Nov 17, 2018 at 08:19:10PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > When sending a NBD_CMD_DISC message there is no reply expected,
> > however, the nbd_read_eof() coroutine is still waiting for a reply.
> > In a plain NBD connection this doesn't matter as it will just get an
> > EOF, however, on a TLS connection it will get an interrupted TLS data
> > packet. The nbd_read_eof() will then print an error message on the
> > console due to missing reply to NBD_CMD_DISC.
> > 
> > This can be seen with qemu-img
> > 
> >    $ qemu-img info \
> >        --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
> >        --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
> >    qemu-img: Cannot read from TLS channel: Input/output error
> >    image: nbd://127.0.0.1:9000
> >    file format: nbd
> >    virtual size: 10M (10485760 bytes)
> >    disk size: unavailable
> > 
> > Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
> > get the coroutine to stop waiting for a reply and thus supress the error
> > message.
> 
> Actually, it's not quite enough - once you actually start performing I/O,
> enough coroutines are kicked off that the error still happens:

Hmm, does the client not send requests synchronously  ? I expected that
any other I/O operations would have already received their reply by the
time we sent the DISC command.

> 
> $  qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
> --object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\
>  driver=nbd,host=localhost,port=10809,tls-creds=tls0
> read 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec)
> wrote 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec)
> Cannot read from TLS channel: Input/output error
> 
> Squashing this in on top of your patch helps, though:
> 
> diff --git i/block/nbd-client.c w/block/nbd-client.c
> index 5f63e4b8f15..e7916c78996 100644
> --- i/block/nbd-client.c
> +++ w/block/nbd-client.c
> @@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void
> *opaque)
>          assert(s->reply.handle == 0);
>          ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
>          if (local_err) {
> -            error_report_err(local_err);
> +            /* If we are already quitting, either another error has
> +             * already been reported, or we requested NBD_CMD_DISC and
> +             * don't need to report anything further.  */
> +            if (!s->quit) {
> +                error_report_err(local_err);
> +            } else {
> +                error_free(local_err);
> +            }
>          }
>          if (ret <= 0) {
>              break;
> 
> But I want to do more testing to make sure I'm not missing out on reporting
> an actual error if I add that.

Yes, I'd be a little concerned about missing reporting of an error from
a command other than NBD_CMD_DISC if we did this.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file

[Daniel P. Berrangé]

On Fri, Nov 16, 2018 at 03:43:16PM -0600, Eric Blake wrote:
> On 11/16/18 3:41 PM, Eric Blake wrote:
> 
> > > +#!/bin/bash
> > 
> > I know we're using bash,
> > 
> > > +
> > > +function nbd_server_stop()
> > > +{
> > 
> > > +function nbd_server_wait_for_unix_socket()
> > 
> > and bash supports 'function', but it is an obsolete syntactic sugar
> > thing that I don't recommend using.  (In ksh, it actually makes a
> > difference in behavior whether you use 'function' or not, and using it
> > in 'bash' makes it harder to port code over to 'ksh' - and hence in bash
> > it is obsolete because here it does NOT cause the change in behavior
> > that ksh users expect)
> > 
> 
> Of course, I hit send too soon, before getting to my punchline:
> 
> Since we already have so many existing iotests that use 'function', it's
> better to clean that up as a separate patch.

Yeah, I actually thought 'function' was the preferred syntax and
omitting it was bad since so many iotests used it :-)

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 3/6] tests: pull qemu-nbd iotest helpers into common.nbd file

[Daniel P. Berrangé]

On Sat, Nov 17, 2018 at 09:01:57PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > The helpers for starting/stopping qemu-nbd in 058 will be useful in
> > other test cases, so move them into a common.nbd file.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> 
> > +function nbd_server_start_unix_socket()
> > +{
> > +    nbd_server_stop
> > +    $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
> 
> Needs to be "$@" to properly preserve whitespace and/or empty arguments (the
> latter if someone passes -x '' for a default-named export).

ok

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 4/6] tests: check if qemu-nbd is still alive before waiting

[Daniel P. Berrangé]

On Fri, Nov 16, 2018 at 10:24:54AM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > If the qemu-nbd UNIX socket has not shown up, the tests will sleep a bit
> > and then check again repeatedly for upto 30 seconds. This is pointless
> 
> s/upto/up to/
> 
> > if the qemu-nbd process has quit due to an error, so check whether the
> > pid is still alive before waiting and retrying.
> 
> "But our tests are perfect and qemu-nbd never fails" :)

Well the key benefit for this is actually people writing new tests,
like me with this series, who continually screw up the argv syntax
to qemu-nbd and don't want to wait 30 seconds for it to fail :)

> Yes, this makes sense.  Not 3.1 material on its own (after all, our
> testsuite isn't showing such failures, so we aren't wasting that time at the
> moment) - but worth including if the later patches end up in 3.1.
> 
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/qemu-iotests/common.nbd | 10 +++++++++-
> >   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > 
> > diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> > index f920a578f1..61e9e90fee 100644
> > --- a/tests/qemu-iotests/common.nbd
> > +++ b/tests/qemu-iotests/common.nbd
> > @@ -37,11 +37,19 @@ function nbd_server_stop()
> >   function nbd_server_wait_for_unix_socket()
> >   {
> > +    pid=$1
> > +
> >       for ((i = 0; i < 300; i++))
> >       do
> >           if [ -r "$nbd_unix_socket" ]; then
> >               return
> >           fi
> > +        kill -s 0 $pid 2>/dev/null
> > +        if test $? != 0
> > +        then
> > +            echo "qemu-nbd unexpectedly quit"
> > +            exit 1
> 
> Maybe the echo should be redirected to stderr.  But we aren't consistently
> doing that in other tests (_init_error does it, but other spots in check are
> not), so I'm not changing it.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Daniel P. Berrangé]

On Fri, Nov 16, 2018 at 10:39:03AM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > Add helpers to common.tls for creating TLS certificates for a CA,
> > server and client.
> 
> MUCH appreciated!  We NEED this coverage, easily automated.
> 
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++
> >   1 file changed, 139 insertions(+)
> >   create mode 100644 tests/qemu-iotests/common.tls
> > 
> > diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
> > new file mode 100644
> 
> I was a bit surprised that this wasn't 100755, but this matches the fact
> that none of the other common.* are executable. And after thinking more, it
> makes sense - they aren't standalone scripts, but designed to be sourced,
> and 'source' doesn't care about execute bits.
> 
> > +tls_dir="${TEST_DIR}/tls"
> > +
> > +function tls_x509_cleanup()
> > +{
> > +    rm -f ${tls_dir}/*.pem
> > +    rm -f ${tls_dir}/*/*.pem
> > +    rmdir ${tls_dir}/*
> > +    rmdir ${tls_dir}
> 
> Why not just:
> rm -rf $tls_dir

Yeah, I guess we could do that for simplicity

> Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces,
> then all uses of ${tls_dir} need to be in "".

Hmm, yes.

> > +}
> > +
> > +
> > +function tls_x509_init()
> > +{
> > +    mkdir "${tls_dir}"
> 
> And this just highlights the quoting inconsistency.  Should this use mkdir
> -p?

I assume $TEST_DIR would already exist, so wouldn't need -p.

> > +
> > +function tls_x509_create_root_ca()
> > +{
> > +    name=$1
> > +
> > +    test -z "$name" && name=ca-cert
> 
> Could also be shortened as:
> 
> name=${1:-ca-cert}

ok

> > +
> > +    cat > ${tls_dir}/ca.info <<EOF
> > +cn = Cthulu Dark Lord Enterprises $name
> 
> s/Cthulu/Cthulhu/ - after all, we don't want him coming after us just
> because we botched the spelling of his name :)
> 
> > +ca
> > +cert_signing_key
> > +EOF
> > +
> > +    certtool --generate-self-signed \
> > +             --load-privkey ${tls_dir}/key.pem \
> > +             --template ${tls_dir}/ca.info \
> > +             --outfile ${tls_dir}/$name-cert.pem 2>&1 | head -1
> 
> More missing ""
> 
> > +
> > +    rm -f ${tls_dir}/ca.info
> > +}
> > +
> > +
> > +function tls_x509_create_server()
> > +{
> > +    caname=$1
> > +    name=$2
> > +
> > +    mkdir ${tls_dir}/$name
> > +    cat > ${tls_dir}/cert.info <<EOF
> > +organization = Cthulu Dark Lord Enterprises $name
> 
> Matched spelling
> 
> > +function tls_x509_create_client()
> > +{
> > +    caname=$1
> > +    name=$2
> > +
> > +    mkdir ${tls_dir}/$name
> > +    cat > ${tls_dir}/cert.info <<EOF
> > +country = South Pacific
> > +locality =  R'lyeh
> > +organization = Cthulu Dark Lord Enterprises $name
> 
> And again
> 
> Needs several touch-ups, but the idea itself is sound.

Yes will fix

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P. Berrangé]

On Fri, Nov 16, 2018 at 11:20:26AM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > Add tests that validate it is possible to connect to an NBD server
> > running TLS mode. Also test mis-matched TLS vs non-TLS connections
> > correctly fail.
> > ---
> >   tests/qemu-iotests/233        | 99 +++++++++++++++++++++++++++++++++++
> >   tests/qemu-iotests/233.out    | 33 ++++++++++++
> >   tests/qemu-iotests/common.nbd | 47 +++++++++++++++++
> >   tests/qemu-iotests/group      |  1 +
> >   4 files changed, 180 insertions(+)
> >   create mode 100755 tests/qemu-iotests/233
> >   create mode 100644 tests/qemu-iotests/233.out
> 
> Adding tests is non-invasive, so I have no objection to taking the entire
> series in 3.1.  Do you want me to touch up the nits I found earlier, or
> should I wait for a v2?

If you're happy to touch it up, that's fine with me.

> > +
> > +$QEMU_IMG info nbd://localhost:$nbd_tcp_port
> > +
> > +echo
> > +echo "== check TLS works =="
> > +$QEMU_IMG info --image-opts \
> > +    --object tls-creds-x509,dir=${tls_dir}/client1,endpoint=client,id=tls0 \
> > +    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
> 
> Is it also worth reading or even writing, to ensure that more than just the
> handshake is working?

I was mostly interested in the handshake/cert stuff, but yes, we could
do some qemu-io testing too.

> > +
> > +echo
> > +echo "== check TLS with different CA fails =="
> > +$QEMU_IMG info --image-opts \
> > +    --object tls-creds-x509,dir=${tls_dir}/client2,endpoint=client,id=tls0 \
> > +    driver=nbd,host=$nbd_tcp_addr,port=$nbd_tcp_port,tls-creds=tls0
> > +
> > +# success, all done
> 
> > +== check TLS client to plain server fails ==
> > +option negotiation failed: read failed: Unexpected end-of-file before all bytes were read
> 
> Annoying message; I wonder if we can clean that up. But not this patch's
> problem.

This is a result of using the 'socat' check to poll for qemu-nbd
readiness. When socat finally succeeds in connecting & then drops
the conenction, qemu-nbd prints this message.  Personally I don't
think we need remove it unless we want to make qemu-nbd silently
by default when clients do unusual things.

> > +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for option 5 (starttls)
> > +server reported: TLS not configured
> 
> This 2-line message is better (and as such, explains why I think the message
> about early EOF should be silenced in this case).
> 
> > +
> > +== check plain client to TLS server fails ==
> > +option negotiation failed: read failed: Unexpected end-of-file before all bytes were read
> > +qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required before option 8 (structured reply)
> > +server reported: Option 0x8 not permitted before TLS
> 
> Same story about a redundant message.

Again this was the socat polling.

> 
> > +write failed (error message): Unable to write to socket: Broken pipe
> 
> Hmm - is this the client complaining that it can't send more to the server
> (that's a bug if the server hung up, since the protocol allows the client to
> change its mind and try TLS after all), or the server complaining that the
> client hung up abruptly without sending NBD_OPT_ABORT? Qemu as client either
> tries TLS right away, or knows that if the server requires TLS and the
> client has no TLS credentials then the client will never succeed, so the
> client gives up - but it SHOULD be giving up with NBD_OPT_ABORT rather than
> just disconnecting.  I'll have to play with that.  Doesn't impact your
> patch, but might spur a followup fix :)

I'm unclear if this message comes from the server or the client since
they are intermingled.


> > diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> > index 61e9e90fee..0483ea7c55 100644
> > --- a/tests/qemu-iotests/common.nbd
> > +++ b/tests/qemu-iotests/common.nbd
> > @@ -20,6 +20,7 @@
> >   #
> >   nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
> > +nbd_tcp_addr="127.0.0.1"
> 
> Should this be in your earlier patch that created common.nbd? Maybe not, as
> it doesn't get used until the functions you add here for tcp support. Still,
> I wonder if we should split the addition of tcp support separate from the
> new test.

Yeah, I wanted the earlier patch to be a plain refactor of existing
functionality, not adding anything new.

> >   nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
> >   function nbd_server_stop()
> > @@ -62,3 +63,49 @@ function nbd_server_start_unix_socket()
> >       $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
> >       nbd_server_wait_for_unix_socket $!
> >   }
> > +
> > +function nbd_server_set_tcp_port()
> > +{
> > +    for port in `seq 10809 10909`
> > +    do
> > +	socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1
> 
> This is the first use of socat in iotests.  Might not be the most portable,
> but I don't know if I have better ideas. nbdkit.git/tests/test-ip.sh greps
> the output of 'ss -ltn' to locate free ports, but I don't know if ss is any
> better than socat.

'ss' is a Linux specific command IIUC since it is part of 'iproute',
while 'socat' is in fact available more or less everywhere:

  https://repology.org/metapackage/socat1/versions


> > +        if test $? != 0
> > +	then
> > +	    nbd_tcp_port=$port

Opps, a stray <tab> here

> > +            return
> > +        fi
> > +    done
> > +
> > +    echo "Cannot find free TCP port for nbd in range 10809-10909"
> > +    exit 1
> 
> Should we skip instead of fail in this case? Would we do well picking a
> larger port range?

Yes, we could simply skip. I figure 100 ports is fine, since tests are
normally run on a largely idle system.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P. Berrangé]

On Sat, Nov 17, 2018 at 03:31:34PM -0600, Eric Blake wrote:
> On 11/16/18 11:20 AM, Eric Blake wrote:
> > On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > > Add tests that validate it is possible to connect to an NBD server
> > > running TLS mode. Also test mis-matched TLS vs non-TLS connections
> > > correctly fail.
> > > ---
> 
> > > +== check TLS client to plain server fails ==
> > > +option negotiation failed: read failed: Unexpected end-of-file
> > > before all bytes were read
> > 
> > Annoying message; I wonder if we can clean that up. But not this patch's
> > problem.
> > 
> 
> Actually, I tracked this message down to using socat (which actually
> connects and then abruptly exits) when probing whether the socket is up and
> listening.  That is, the message is being produced as a side effect of
> nbd_server_wait_for_tcp_socket rather than during the actual $QEMU_IMG
> command we are interested in testing.
> 
> 
> > >   nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
> > >   function nbd_server_stop()
> > > @@ -62,3 +63,49 @@ function nbd_server_start_unix_socket()
> > >       $QEMU_NBD -v -t -k "$nbd_unix_socket" $@ &
> > >       nbd_server_wait_for_unix_socket $!
> > >   }
> > > +
> > > +function nbd_server_set_tcp_port()
> > > +{
> > > +    for port in `seq 10809 10909`
> > > +    do
> > > +    socat TCP:$nbd_tcp_addr:$port STDIO < /dev/null 1>/dev/null 2>&1
> > 
> > This is the first use of socat in iotests.  Might not be the most
> > portable, but I don't know if I have better ideas.
> > nbdkit.git/tests/test-ip.sh greps the output of 'ss -ltn' to locate free
> > ports, but I don't know if ss is any better than socat.
> 
> So, I'm planning to squash this in, to use ss instead of socat, as follows:

Personally I prefer socat since it is more portable, per my previous
message.

> diff --git i/tests/qemu-iotests/233.out w/tests/qemu-iotests/233.out
> index eaa410c2703..eb4077f9fd7 100644
> --- i/tests/qemu-iotests/233.out
> +++ w/tests/qemu-iotests/233.out
> @@ -11,12 +11,10 @@ Generating a signed certificate...
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> 
>  == check TLS client to plain server fails ==
> -option negotiation failed: read failed: Unexpected end-of-file before all
> bytes were read
>  qemu-img: Could not open
> 'driver=nbd,host=127.0.0.1,port=10809,tls-creds=tls0': Denied by server for
> option 5 (starttls)
>  server reported: TLS not configured
> 
>  == check plain client to TLS server fails ==
> -option negotiation failed: read failed: Unexpected end-of-file before all
> bytes were read
>  qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required
> before option 8 (structured reply)
>  server reported: Option 0x8 not permitted before TLS
>  write failed (error message): Unable to write to socket: Broken pipe
> 
> 
> Also, you have to sanitize 233.out to change 10809 into PORT, so the test
> can still pass when it picked a different port.

Opps, yes.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P. Berrangé]

On Sat, Nov 17, 2018 at 02:49:22PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > Add tests that validate it is possible to connect to an NBD server
> > running TLS mode. Also test mis-matched TLS vs non-TLS connections
> > correctly fail.
> > ---
> 
> Missing your Signed-off-by. Can you please supply that, so I can include
> this in my pull request?

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>


> 
> Also, I'm getting failures when trying to test it:
> 
> @@ -17,9 +17,9 @@
> 
>  == check plain client to TLS server fails ==
>  option negotiation failed: read failed: Unexpected end-of-file before all
> bytes were read
> +write failed (error message): Unable to write to socket: Broken pipe
>  qemu-img: Could not open 'nbd://localhost:10809': TLS negotiation required
> before option 8 (structured reply)
>  server reported: Option 0x8 not permitted before TLS
> -write failed (error message): Unable to write to socket: Broken pipe
> 
> 
> which looks like an output race. :(

Guess that shows the message is in fact coming from the server rather
than the client then.

We could just silence the qemu-nbd process to null, since we're primarily
interested in what the client displays in this test

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1.5/6] nbd/server: Ignore write errors when replying to NBD_OPT_ABORT

[Daniel P. Berrangé]

On Sat, Nov 17, 2018 at 04:32:21PM -0600, Eric Blake wrote:
> Commit 37ec36f6 intentionally ignores errors when trying to reply
> to an NBD_OPT_ABORT request for plaintext clients, but did not make
> the same change for a TLS server.  Since NBD_OPT_ABORT is
> documented as being a potential for an EPIPE when the client hangs
> up without waiting for our reply, we don't need to pollute the
> server's output with that failure.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  nbd/server.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 056cfa5ad47..dc04513de70 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1134,12 +1134,16 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
>                  return -EINVAL;
> 
>              default:
> -                ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
> +                /* Let the client keep trying, unless they asked to
> +                 * quit. Always try to give an error back to the
> +                 * client; but when replying to OPT_ABORT, be aware
> +                 * that the client may hang up before receiving the
> +                 * error, in which case we are fine ignoring the
> +                 * resulting EPIPE. */
> +                ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD,
> +                                   option == NBD_OPT_ABORT ? NULL : errp,
>                                     "Option 0x%" PRIx32
>                                     " not permitted before TLS", option);
> -                /* Let the client keep trying, unless they asked to
> -                 * quit. In this mode, we've already sent an error, so
> -                 * we can't ack the abort.  */
>                  if (option == NBD_OPT_ABORT) {
>                      return 1;
>                  }

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 7/6] iotests: Also test I/O over NBD TLS

[Daniel P. Berrangé]

On Sat, Nov 17, 2018 at 08:24:03PM -0600, Eric Blake wrote:
> Enhance test 233 to also perform I/O beyond the initial handshake.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Depends on my tweak to 2/6 to suppress an EIO error message
> on a failed read after NBD_CMD_DISC.
> 
>  tests/qemu-iotests/233     | 12 +++++++++++-
>  tests/qemu-iotests/233.out | 10 ++++++++++
>  2 files changed, 21 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

On 19.11.18 11:27, Daniel P. Berrangé wrote:
> On Fri, Nov 16, 2018 at 10:39:03AM -0600, Eric Blake wrote:
>> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
>>> Add helpers to common.tls for creating TLS certificates for a CA,
>>> server and client.
>>
>> MUCH appreciated!  We NEED this coverage, easily automated.
>>
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>   tests/qemu-iotests/common.tls | 139 ++++++++++++++++++++++++++++++++++
>>>   1 file changed, 139 insertions(+)
>>>   create mode 100644 tests/qemu-iotests/common.tls
>>>
>>> diff --git a/tests/qemu-iotests/common.tls b/tests/qemu-iotests/common.tls
>>> new file mode 100644
>>
>> I was a bit surprised that this wasn't 100755, but this matches the fact
>> that none of the other common.* are executable. And after thinking more, it
>> makes sense - they aren't standalone scripts, but designed to be sourced,
>> and 'source' doesn't care about execute bits.
>>
>>> +tls_dir="${TEST_DIR}/tls"
>>> +
>>> +function tls_x509_cleanup()
>>> +{
>>> +    rm -f ${tls_dir}/*.pem
>>> +    rm -f ${tls_dir}/*/*.pem
>>> +    rmdir ${tls_dir}/*
>>> +    rmdir ${tls_dir}
>>
>> Why not just:
>> rm -rf $tls_dir
> 
> Yeah, I guess we could do that for simplicity
> 
>> Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces,
>> then all uses of ${tls_dir} need to be in "".
> 
> Hmm, yes.

Which by the way is a very good reason *not* to blindly use "rm -r".

So far we only seem to have one instance of "rm -r" in the iotests (and
that is on three files, so I don't even know why that has -r), and I'm
glad about that.

I prefer for scripts to only delete what they have created and not
blindly delete something.  Wildcards are already kind of pushing it.

Maybe the user has created the tls dir beforehand, then I'd prefer for
the iotests not to just delete it and everything in it.  But the worst
of course would be if we get escaping wrong somewhere (as demonstrated
here) and suddenly we delete a completely unrelated directory by
accident.  Anyone remember Steam's 'rm -rf "$STEAMROOT"/*'?

Everyone knows they have to be careful with deleting things, but most of
the time it is a bother if you're in an interactive shell and know your
directory structure and all the arguments you're passing perfectly well.
 But a script doesn't know either, and "it's a bother" is not really an
argument if you have to write the code just once.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Daniel P. Berrangé]

On Sat, Nov 17, 2018 at 08:19:10PM -0600, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> > When sending a NBD_CMD_DISC message there is no reply expected,
> > however, the nbd_read_eof() coroutine is still waiting for a reply.
> > In a plain NBD connection this doesn't matter as it will just get an
> > EOF, however, on a TLS connection it will get an interrupted TLS data
> > packet. The nbd_read_eof() will then print an error message on the
> > console due to missing reply to NBD_CMD_DISC.
> > 
> > This can be seen with qemu-img
> > 
> >    $ qemu-img info \
> >        --object tls-creds-x509,dir=tlsdir,id=tls0,endpoint=client \
> >        --image-opts driver=nbd,host=127.0.0.1,port=9000,tls-creds=tls0
> >    qemu-img: Cannot read from TLS channel: Input/output error
> >    image: nbd://127.0.0.1:9000
> >    file format: nbd
> >    virtual size: 10M (10485760 bytes)
> >    disk size: unavailable
> > 
> > Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
> > get the coroutine to stop waiting for a reply and thus supress the error
> > message.
> 
> Actually, it's not quite enough - once you actually start performing I/O,
> enough coroutines are kicked off that the error still happens:
> 
> $  qemu-io -c 'r 1m 1m' -c 'w -P 0x22 1m 1m' --image-opts \
> --object tls-creds-x509,dir=scratch/tls/client1,endpoint=client,id=tls0\
>  driver=nbd,host=localhost,port=10809,tls-creds=tls0
> read 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0430 sec (23.204 MiB/sec and 23.2040 ops/sec)
> wrote 1048576/1048576 bytes at offset 1048576
> 1 MiB, 1 ops; 0.0152 sec (65.479 MiB/sec and 65.4793 ops/sec)
> Cannot read from TLS channel: Input/output error
> 
> Squashing this in on top of your patch helps, though:
> 
> diff --git i/block/nbd-client.c w/block/nbd-client.c
> index 5f63e4b8f15..e7916c78996 100644
> --- i/block/nbd-client.c
> +++ w/block/nbd-client.c
> @@ -79,7 +79,14 @@ static coroutine_fn void nbd_read_reply_entry(void
> *opaque)
>          assert(s->reply.handle == 0);
>          ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
>          if (local_err) {
> -            error_report_err(local_err);
> +            /* If we are already quitting, either another error has
> +             * already been reported, or we requested NBD_CMD_DISC and
> +             * don't need to report anything further.  */
> +            if (!s->quit) {
> +                error_report_err(local_err);
> +            } else {
> +                error_free(local_err);
> +            }
>          }
>          if (ret <= 0) {
>              break;
> 
> But I want to do more testing to make sure I'm not missing out on reporting
> an actual error if I add that.

It occurred to me that it would be nice if the TLS channel behaved the same
a normal channel when seeing EOF on the socket. Unfortunately the reason why
GNUTLS returns an error in this case does in fact make sense, so it isn't
quite as simple as just ignoring the error. Fortunately the NBD client has
decided it wants to shutdown and crucially has called qio_channel_shutdown()
at this point. Thus we can safely ignore the gnutls error code when the
channel has been shutdown for read and just return 0 for EOF as with a plain
socket.

Thus I've sent a patch which solves the problem in that way:

  https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg03616.html

With this applied, both my original patch and your extra chunk here
should be redundant.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 2/6 for-3.1] nbd: stop waiting for a NBD response with NBD_CMD_DISC

[Eric Blake]

On 11/19/18 4:23 AM, Daniel P. Berrangé wrote:

>>> Simply setting the 'quit' flag after sending NBD_CMD_DISC is enough to
>>> get the coroutine to stop waiting for a reply and thus supress the error
>>> message.
>>
>> Actually, it's not quite enough - once you actually start performing I/O,
>> enough coroutines are kicked off that the error still happens:
> 
> Hmm, does the client not send requests synchronously  ? I expected that
> any other I/O operations would have already received their reply by the
> time we sent the DISC command.
> 

During handshake phase (NBD_OPT_* messages), client sends requests 
synchronously (at most one pending read, in response to the most recent 
write). During transmission phase (NBD_CMD_* messages), the client is 
allowed to send messages asynchronously.  However, the NBD protocol 
recommends (but not requires) that the client never send NBD_CMD_DISC to 
disconnect until AFTER the client has collected all responses to any 
earlier in-flight messages. And if I'm reading the block/nbd-client.c 
code correctly, in general we don't try to shutdown a client while there 
are any pending commands with unread replies, except when we are 
shutting down due to detecting a protocol error (at which point we are 
already out of sync with the server and waiting for replies is not going 
to be helpful).


>> But I want to do more testing to make sure I'm not missing out on reporting
>> an actual error if I add that.
> 
> Yes, I'd be a little concerned about missing reporting of an error from
> a command other than NBD_CMD_DISC if we did this.

But it looks like your approach of making the qio code behave more 
nicely is a smarter cleanup than my idea, so I'm willing to ditch my 
code in favor of yours.  Testing your approach now :)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Eric Blake]

On 11/19/18 5:04 AM, Max Reitz wrote:

>>>> +tls_dir="${TEST_DIR}/tls"
>>>> +
>>>> +function tls_x509_cleanup()
>>>> +{
>>>> +    rm -f ${tls_dir}/*.pem
>>>> +    rm -f ${tls_dir}/*/*.pem
>>>> +    rmdir ${tls_dir}/*
>>>> +    rmdir ${tls_dir}
>>>
>>> Why not just:
>>> rm -rf $tls_dir
>>
>> Yeah, I guess we could do that for simplicity
>>
>>> Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces,
>>> then all uses of ${tls_dir} need to be in "".
>>
>> Hmm, yes.
> 
> Which by the way is a very good reason *not* to blindly use "rm -r".

If we ever revive Jeff Cody's patches to let each test run with its own 
temporary directory, then we don't need this function at all.  With that 
in place, you either run the testsuite in debug mode (all temporary 
files preserved) or in normal mode (./check itself does rm -rf on the 
temporary directory).

> 
> So far we only seem to have one instance of "rm -r" in the iotests (and
> that is on three files, so I don't even know why that has -r), and I'm
> glad about that.

But until we have the global implementation of per-test temporary 
directories with cleanup relegated to the testsuite driver, I'm fine 
following your preference of explicit deletion of specific files rather 
than recursive deletion of the tree, even if it is more lines of code.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 5/6] tests: add iotests helpers for dealing with TLS certificates

[Daniel P. Berrangé]

On Mon, Nov 19, 2018 at 08:27:56AM -0600, Eric Blake wrote:
> On 11/19/18 5:04 AM, Max Reitz wrote:
> 
> > > > > +tls_dir="${TEST_DIR}/tls"
> > > > > +
> > > > > +function tls_x509_cleanup()
> > > > > +{
> > > > > +    rm -f ${tls_dir}/*.pem
> > > > > +    rm -f ${tls_dir}/*/*.pem
> > > > > +    rmdir ${tls_dir}/*
> > > > > +    rmdir ${tls_dir}
> > > > 
> > > > Why not just:
> > > > rm -rf $tls_dir
> > > 
> > > Yeah, I guess we could do that for simplicity
> > > 
> > > > Also, the quoting is a bit inconsistent. if ${TEST_DIR} can contain spaces,
> > > > then all uses of ${tls_dir} need to be in "".
> > > 
> > > Hmm, yes.
> > 
> > Which by the way is a very good reason *not* to blindly use "rm -r".
> 
> If we ever revive Jeff Cody's patches to let each test run with its own
> temporary directory, then we don't need this function at all.  With that in
> place, you either run the testsuite in debug mode (all temporary files
> preserved) or in normal mode (./check itself does rm -rf on the temporary
> directory).

That would be very nice - i'm often just commenting out the rm lines to
preserve temp files.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/6 for-3.1] nbd: fix whitespace in server error message

[Philippe Mathieu-Daudé]

On 16/11/18 17:01, Eric Blake wrote:
> On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
>> A space was missing after the option number was printed:
>>
>>    Option 0x8not permitted before TLS
>>
>> becomes
>>
>>    Option 0x8 not permitted before TLS
>>
>> This fixes
>>
>>    commit 3668328303429f3bc93ab3365c66331600b06a2d
>>    Author: Eric Blake <eblake@redhat.com>
>>    Date:   Fri Oct 14 13:33:09 2016 -0500
>>
>>      nbd: Send message along with server NBD_REP_ERR errors
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>   nbd/server.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 4e8f5ae51b..12e8139f95 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1135,7 +1135,7 @@ static int nbd_negotiate_options(NBDClient 
>> *client, uint16_t myflags,
>>               default:
>>                   ret = nbd_opt_drop(client, NBD_REP_ERR_TLS_REQD, errp,
>> -                                   "Option 0x%" PRIx32
>> +                                   "Option 0x%" PRIx32 " "
>>                                      "not permitted before TLS", option);
> 
> Visually, I'd include the space in the next line instead of on its own 

Agreed :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> (but that's aesthetics, not semantic). Agree that this is 3.1 material; 
> I'll queue it up and send a PR on Monday.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/19/18 4:37 AM, Daniel P. Berrangé wrote:

>> Actually, I tracked this message down to using socat (which actually
>> connects and then abruptly exits) when probing whether the socket is up and
>> listening.  That is, the message is being produced as a side effect of
>> nbd_server_wait_for_tcp_socket rather than during the actual $QEMU_IMG
>> command we are interested in testing.

>>> This is the first use of socat in iotests.  Might not be the most
>>> portable, but I don't know if I have better ideas.
>>> nbdkit.git/tests/test-ip.sh greps the output of 'ss -ltn' to locate free
>>> ports, but I don't know if ss is any better than socat.
>>
>> So, I'm planning to squash this in, to use ss instead of socat, as follows:
> 
> Personally I prefer socat since it is more portable, per my previous
> message.

socat is indeed probably more portable, but since tests 233 uses 
'_supported_os Linux', ss availability isn't a problem until a future 
user ports this test to non-Linux.  I'd like to patch qemu-nbd to NOT 
warn about a user that connects but does not consume data (the socat 
case, as well as port probes), but as that patch does not exist yet and 
-rc2 is getting close, I'll go ahead and send the pull request with ss 
instead of socat.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> Add tests that validate it is possible to connect to an NBD server
> running TLS mode. Also test mis-matched TLS vs non-TLS connections
> correctly fail.
> ---

> +++ b/tests/qemu-iotests/common.nbd

> +function nbd_server_wait_for_tcp_socket()
> +{

> +        sleep 0.1
> +    done
> +    echo "Failed in check of unix socket created by qemu-nbd"

Too much copy-and-paste.  Touching up. :)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 7/6] iotests: Also test I/O over NBD TLS

[Eric Blake]

On 11/19/18 4:40 AM, Daniel P. Berrangé wrote:
> On Sat, Nov 17, 2018 at 08:24:03PM -0600, Eric Blake wrote:
>> Enhance test 233 to also perform I/O beyond the initial handshake.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> Depends on my tweak to 2/6 to suppress an EIO error message
>> on a failed read after NBD_CMD_DISC.
>>
>>   tests/qemu-iotests/233     | 12 +++++++++++-
>>   tests/qemu-iotests/233.out | 10 ++++++++++
>>   2 files changed, 21 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Yay - after dropping patch 2/6 (both your work, and the code I was 
questioning whether to squash in), and instead using your patch for 
making qio return 0 on connection abort after shutdown, this test still 
passes without any extra lines from the server.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P. Berrangé]

On Mon, Nov 19, 2018 at 11:00:38AM -0600, Eric Blake wrote:
> On 11/19/18 4:37 AM, Daniel P. Berrangé wrote:
> 
> > > Actually, I tracked this message down to using socat (which actually
> > > connects and then abruptly exits) when probing whether the socket is up and
> > > listening.  That is, the message is being produced as a side effect of
> > > nbd_server_wait_for_tcp_socket rather than during the actual $QEMU_IMG
> > > command we are interested in testing.
> 
> > > > This is the first use of socat in iotests.  Might not be the most
> > > > portable, but I don't know if I have better ideas.
> > > > nbdkit.git/tests/test-ip.sh greps the output of 'ss -ltn' to locate free
> > > > ports, but I don't know if ss is any better than socat.
> > > 
> > > So, I'm planning to squash this in, to use ss instead of socat, as follows:
> > 
> > Personally I prefer socat since it is more portable, per my previous
> > message.
> 
> socat is indeed probably more portable, but since tests 233 uses
> '_supported_os Linux', ss availability isn't a problem until a future user
> ports this test to non-Linux.  I'd like to patch qemu-nbd to NOT warn about
> a user that connects but does not consume data (the socat case, as well as
> port probes), but as that patch does not exist yet and -rc2 is getting
> close, I'll go ahead and send the pull request with ss instead of socat.

Yes, that's ok with me.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Kevin Wolf]

Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben:
> Add tests that validate it is possible to connect to an NBD server
> running TLS mode. Also test mis-matched TLS vs non-TLS connections
> correctly fail.

> +echo
> +echo "== preparing TLS creds =="
> +
> +tls_x509_create_root_ca "ca1"
> +tls_x509_create_root_ca "ca2"
> +tls_x509_create_server "ca1" "server1"
> +tls_x509_create_client "ca1" "client1"
> +tls_x509_create_client "ca2" "client2"

Looks like we can't blindly assume that certtool exists. This test case
fails for me, starting with the following diff:

@@ -1,30 +1,21 @@
 QA output created by 233

 == preparing TLS creds ==
-Generating a self signed certificate...
-Generating a self signed certificate...
-Generating a signed certificate...
-Generating a signed certificate...
-Generating a signed certificate...
+./common.tls: line 71: certtool: command not found
+./common.tls: line 71: certtool: command not found
+./common.tls: line 98: certtool: command not found
+./common.tls: line 127: certtool: command not found
+./common.tls: line 127: certtool: command not found

Of course, after that everything else fails as well.

Kevin

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/20/18 11:27 AM, Kevin Wolf wrote:
> Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben:
>> Add tests that validate it is possible to connect to an NBD server
>> running TLS mode. Also test mis-matched TLS vs non-TLS connections
>> correctly fail.
> 
>> +echo
>> +echo "== preparing TLS creds =="
>> +
>> +tls_x509_create_root_ca "ca1"
>> +tls_x509_create_root_ca "ca2"
>> +tls_x509_create_server "ca1" "server1"
>> +tls_x509_create_client "ca1" "client1"
>> +tls_x509_create_client "ca2" "client2"
> 
> Looks like we can't blindly assume that certtool exists. This test case
> fails for me, starting with the following diff:

Looks like we'll need a followup patch to skip the test if certtool is 
not found. (I already did the same in common.nbd if 'ss' was not found; 
so it should be easy to copy...)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P. Berrangé]

On Tue, Nov 20, 2018 at 11:45:54AM -0600, Eric Blake wrote:
> On 11/20/18 11:27 AM, Kevin Wolf wrote:
> > Am 16.11.2018 um 16:53 hat Daniel P. Berrangé geschrieben:
> > > Add tests that validate it is possible to connect to an NBD server
> > > running TLS mode. Also test mis-matched TLS vs non-TLS connections
> > > correctly fail.
> > 
> > > +echo
> > > +echo "== preparing TLS creds =="
> > > +
> > > +tls_x509_create_root_ca "ca1"
> > > +tls_x509_create_root_ca "ca2"
> > > +tls_x509_create_server "ca1" "server1"
> > > +tls_x509_create_client "ca1" "client1"
> > > +tls_x509_create_client "ca2" "client2"
> > 
> > Looks like we can't blindly assume that certtool exists. This test case
> > fails for me, starting with the following diff:
> 
> Looks like we'll need a followup patch to skip the test if certtool is not
> found. (I already did the same in common.nbd if 'ss' was not found; so it
> should be easy to copy...)

FWIW certtool is part of gnutls-utils and is available on every platform
that QEMU officially supports as a build target.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Eric Blake]

On 11/20/18 11:53 AM, Daniel P. Berrangé wrote:

>>>> +echo
>>>> +echo "== preparing TLS creds =="
>>>> +
>>>> +tls_x509_create_root_ca "ca1"
>>>> +tls_x509_create_root_ca "ca2"
>>>> +tls_x509_create_server "ca1" "server1"
>>>> +tls_x509_create_client "ca1" "client1"
>>>> +tls_x509_create_client "ca2" "client2"
>>>
>>> Looks like we can't blindly assume that certtool exists. This test case
>>> fails for me, starting with the following diff:
>>
>> Looks like we'll need a followup patch to skip the test if certtool is not
>> found. (I already did the same in common.nbd if 'ss' was not found; so it
>> should be easy to copy...)
> 
> FWIW certtool is part of gnutls-utils and is available on every platform
> that QEMU officially supports as a build target.

Ported to the build target != installed on the build machine. The 
failure is happening on machines that do not have all the build 
prerequisites (since it should still be possible to configure qemu to 
build without TLS, right?)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Kevin Wolf]

Am 20.11.2018 um 19:22 hat Eric Blake geschrieben:
> On 11/20/18 11:53 AM, Daniel P. Berrangé wrote:
> 
> > > > > +echo
> > > > > +echo "== preparing TLS creds =="
> > > > > +
> > > > > +tls_x509_create_root_ca "ca1"
> > > > > +tls_x509_create_root_ca "ca2"
> > > > > +tls_x509_create_server "ca1" "server1"
> > > > > +tls_x509_create_client "ca1" "client1"
> > > > > +tls_x509_create_client "ca2" "client2"
> > > > 
> > > > Looks like we can't blindly assume that certtool exists. This test case
> > > > fails for me, starting with the following diff:
> > > 
> > > Looks like we'll need a followup patch to skip the test if certtool is not
> > > found. (I already did the same in common.nbd if 'ss' was not found; so it
> > > should be easy to copy...)
> > 
> > FWIW certtool is part of gnutls-utils and is available on every platform
> > that QEMU officially supports as a build target.
> 
> Ported to the build target != installed on the build machine. The failure is
> happening on machines that do not have all the build prerequisites (since it
> should still be possible to configure qemu to build without TLS, right?)

It happens on my laptop, and qemu builds just fine.

Kevin

Re: [Qemu-devel] [PATCH 6/6] tests: exercise NBD server in TLS mode

[Daniel P. Berrangé]

On Tue, Nov 20, 2018 at 12:22:39PM -0600, Eric Blake wrote:
> On 11/20/18 11:53 AM, Daniel P. Berrangé wrote:
> 
> > > > > +echo
> > > > > +echo "== preparing TLS creds =="
> > > > > +
> > > > > +tls_x509_create_root_ca "ca1"
> > > > > +tls_x509_create_root_ca "ca2"
> > > > > +tls_x509_create_server "ca1" "server1"
> > > > > +tls_x509_create_client "ca1" "client1"
> > > > > +tls_x509_create_client "ca2" "client2"
> > > > 
> > > > Looks like we can't blindly assume that certtool exists. This test case
> > > > fails for me, starting with the following diff:
> > > 
> > > Looks like we'll need a followup patch to skip the test if certtool is not
> > > found. (I already did the same in common.nbd if 'ss' was not found; so it
> > > should be easy to copy...)
> > 
> > FWIW certtool is part of gnutls-utils and is available on every platform
> > that QEMU officially supports as a build target.
> 
> Ported to the build target != installed on the build machine. The failure is
> happening on machines that do not have all the build prerequisites (since it
> should still be possible to configure qemu to build without TLS, right?)

Yes, I just assumed that the iotests could expect a fully featured install

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/6] Misc fixes to NBD

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:
> This does two minor fixes to the NBD code and adds significant coverage
> of the NBD TLS support to detect future problems.
> 
> The first two patches should be for 3.1.
> 
> The tests can wait till 4.0 if desired.

Although this series is now in 3.1, I can think of further enhancements 
we should add for 4.0 (summarizing an IRC conversation with Dan). 
Capturing it here to remember things...

- we need iotests coverage of Pre-Shared Keys (PSK) as an alternative to 
certificates (either add on to 233, or a new test)
- add an optional QMP parameter for specifying the hostname to validate 
a certificate against when using a Unix socket with TLS (compare 
tls-hostname added to 'migrate'), rather than the current restriction 
that using TLS with an NBD client requires TCP

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org