[PULL 00/11] Crypto patches

[PULL 00/11] Crypto patches

[Daniel P. Berrangé]

The following changes since commit 6410f877f5ed535acd01bbfaa4baec379e44d0ef:

  Merge tag 'hw-misc-20240723' of https://github.com/philmd/qemu into staging (2024-07-24 15:39:43 +1000)

are available in the Git repository at:

  https://gitlab.com/berrange/qemu tags/misc-fixes-pull-request

for you to fetch changes up to 97f7bf113eb50fcdaf0c73aa2ee01e5355abc073:

  crypto: propagate errors from TLS session I/O callbacks (2024-07-24 10:39:10 +0100)

----------------------------------------------------------------

* Drop unused 'detached-header' QAPI field from LUKS create options
* Improve tracing of TLS sockets and TLS chardevs
* Improve error messages from TLS I/O failures
* Add docs about use of LUKS detached header options
* Allow building without libtasn1, but with GNUTLS
* Fix detection of libgcrypt when libgcrypt-config is absent

----------------------------------------------------------------

Daniel P. Berrangé (6):
  qapi: drop unused QCryptoBlockCreateOptionsLUKS.detached-header
  meson: build chardev trace files when have_block
  chardev: add tracing of socket error conditions
  crypto: drop gnutls debug logging support
  crypto: push error reporting into TLS session I/O APIs
  crypto: propagate errors from TLS session I/O callbacks

Hyman Huang (1):
  docs/devel: Add introduction to LUKS volume with detached header

Philippe Mathieu-Daudé (3):
  crypto: Remove 'crypto-tls-x509-helpers.h' from
    crypto-tls-psk-helpers.c
  crypto: Restrict pkix_asn1_tab[] to crypto-tls-x509-helpers.c
  crypto: Allow building with GnuTLS but without Libtasn1

Yao Zi (1):
  meson.build: fix libgcrypt detection on system without
    libgcrypt-config

 MAINTAINERS                                   |   3 +-
 chardev/char-socket.c                         |  37 ++--
 chardev/trace-events                          |  10 +
 crypto/init.c                                 |  15 +-
 crypto/tlssession.c                           | 124 ++++++++----
 docs/devel/crypto.rst                         |  10 +
 docs/devel/index-internals.rst                |   1 +
 docs/devel/luks-detached-header.rst           | 182 ++++++++++++++++++
 include/crypto/tlssession.h                   |  33 +++-
 io/channel-tls.c                              |  66 +++----
 meson.build                                   |   4 +-
 qapi/crypto.json                              |   5 +-
 tests/qtest/meson.build                       |   3 +-
 tests/unit/crypto-tls-psk-helpers.c           |   1 -
 tests/unit/crypto-tls-x509-helpers.c          |   6 +-
 tests/unit/crypto-tls-x509-helpers.h          |   3 -
 tests/unit/meson.build                        |   6 +-
 .../{pkix_asn1_tab.c => pkix_asn1_tab.c.inc}  |   5 +-
 tests/unit/test-crypto-tlssession.c           |  30 ++-
 19 files changed, 418 insertions(+), 126 deletions(-)
 create mode 100644 docs/devel/crypto.rst
 create mode 100644 docs/devel/luks-detached-header.rst
 rename tests/unit/{pkix_asn1_tab.c => pkix_asn1_tab.c.inc} (99%)

-- 
2.45.2

[PULL 01/11] crypto: Remove 'crypto-tls-x509-helpers.h' from crypto-tls-psk-helpers.c

[Daniel P. Berrangé]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

crypto-tls-psk-helpers.c doesn't access the declarations
of "crypto-tls-x509-helpers.h", remove the include line
to avoid when building with GNUTLS but without Libtasn1:

  In file included from tests/unit/crypto-tls-psk-helpers.c:23:
  tests/unit/crypto-tls-x509-helpers.h:26:10: fatal error:
  libtasn1.h: No such file or directory
     26 | #include <libtasn1.h>
        |          ^~~~~~~~~~~~
  compilation terminated.

Fixes: e1a6dc91dd ("crypto: Implement TLS Pre-Shared Keys (PSK).")
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/unit/crypto-tls-psk-helpers.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/unit/crypto-tls-psk-helpers.c b/tests/unit/crypto-tls-psk-helpers.c
index c6cc740772..36527fd655 100644
--- a/tests/unit/crypto-tls-psk-helpers.c
+++ b/tests/unit/crypto-tls-psk-helpers.c
@@ -20,7 +20,6 @@
 
 #include "qemu/osdep.h"
 
-#include "crypto-tls-x509-helpers.h"
 #include "crypto-tls-psk-helpers.h"
 #include "qemu/sockets.h"
 
-- 
2.45.2

[PULL 02/11] crypto: Restrict pkix_asn1_tab[] to crypto-tls-x509-helpers.c

[Daniel P. Berrangé]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

pkix_asn1_tab[] is only accessed by crypto-tls-x509-helpers.c,
rename pkix_asn1_tab.c as pkix_asn1_tab.c.inc and include it once.

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
[berrange: updated MAINTAINERS for changed filename]
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 MAINTAINERS                                         | 2 +-
 tests/qtest/meson.build                             | 3 +--
 tests/unit/crypto-tls-x509-helpers.c                | 6 +++++-
 tests/unit/crypto-tls-x509-helpers.h                | 3 ---
 tests/unit/meson.build                              | 6 +++---
 tests/unit/{pkix_asn1_tab.c => pkix_asn1_tab.c.inc} | 5 +----
 6 files changed, 11 insertions(+), 14 deletions(-)
 rename tests/unit/{pkix_asn1_tab.c => pkix_asn1_tab.c.inc} (99%)

diff --git a/MAINTAINERS b/MAINTAINERS
index dd01288992..73040829b1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3484,7 +3484,7 @@ F: qapi/crypto.json
 F: tests/unit/test-crypto-*
 F: tests/bench/benchmark-crypto-*
 F: tests/unit/crypto-tls-*
-F: tests/unit/pkix_asn1_tab.c
+F: tests/unit/pkix_asn1_tab.c.inc
 F: qemu.sasl
 
 Coroutines
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index ff9200f882..e7ab2a4312 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -322,8 +322,7 @@ if gnutls.found()
   migration_files += [files('../unit/crypto-tls-psk-helpers.c'), gnutls]
 
   if tasn1.found()
-    migration_files += [files('../unit/crypto-tls-x509-helpers.c',
-                              '../unit/pkix_asn1_tab.c'), tasn1]
+    migration_files += [files('../unit/crypto-tls-x509-helpers.c'), tasn1]
   endif
 endif
 
diff --git a/tests/unit/crypto-tls-x509-helpers.c b/tests/unit/crypto-tls-x509-helpers.c
index e9937f60d8..3e74ec5b5d 100644
--- a/tests/unit/crypto-tls-x509-helpers.c
+++ b/tests/unit/crypto-tls-x509-helpers.c
@@ -20,15 +20,19 @@
 
 #include "qemu/osdep.h"
 
+#include <libtasn1.h>
+
 #include "crypto-tls-x509-helpers.h"
 #include "crypto/init.h"
 #include "qemu/sockets.h"
 
+#include "pkix_asn1_tab.c.inc"
+
 /*
  * This stores some static data that is needed when
  * encoding extensions in the x509 certs
  */
-asn1_node pkix_asn1;
+static asn1_node pkix_asn1;
 
 /*
  * To avoid consuming random entropy to generate keys,
diff --git a/tests/unit/crypto-tls-x509-helpers.h b/tests/unit/crypto-tls-x509-helpers.h
index 247e7160eb..562c160653 100644
--- a/tests/unit/crypto-tls-x509-helpers.h
+++ b/tests/unit/crypto-tls-x509-helpers.h
@@ -23,7 +23,6 @@
 
 #include <gnutls/gnutls.h>
 #include <gnutls/x509.h>
-#include <libtasn1.h>
 
 
 #define QCRYPTO_TLS_TEST_CLIENT_NAME "ACME QEMU Client"
@@ -171,6 +170,4 @@ void test_tls_cleanup(const char *keyfile);
     };                                                                  \
     test_tls_generate_cert(&varname, cavarname.crt)
 
-extern const asn1_static_node pkix_asn1_tab[];
-
 #endif
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 26c109c968..490ab8182d 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -99,11 +99,11 @@ if have_block
      tasn1.found() and \
      host_os != 'windows'
     tests += {
-      'test-crypto-tlscredsx509': ['crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c',
+      'test-crypto-tlscredsx509': ['crypto-tls-x509-helpers.c',
                                    tasn1, crypto, gnutls],
-      'test-crypto-tlssession': ['crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c', 'crypto-tls-psk-helpers.c',
+      'test-crypto-tlssession': ['crypto-tls-x509-helpers.c', 'crypto-tls-psk-helpers.c',
                                  tasn1, crypto, gnutls],
-      'test-io-channel-tls': ['io-channel-helpers.c', 'crypto-tls-x509-helpers.c', 'pkix_asn1_tab.c',
+      'test-io-channel-tls': ['io-channel-helpers.c', 'crypto-tls-x509-helpers.c',
                               tasn1, io, crypto, gnutls]}
   endif
   if pam.found()
diff --git a/tests/unit/pkix_asn1_tab.c b/tests/unit/pkix_asn1_tab.c.inc
similarity index 99%
rename from tests/unit/pkix_asn1_tab.c
rename to tests/unit/pkix_asn1_tab.c.inc
index 89521408a1..fe29c4102a 100644
--- a/tests/unit/pkix_asn1_tab.c
+++ b/tests/unit/pkix_asn1_tab.c.inc
@@ -3,10 +3,7 @@
  * and is under copyright of various GNUTLS contributors.
  */
 
-#include "qemu/osdep.h"
-#include "crypto-tls-x509-helpers.h"
-
-const asn1_static_node pkix_asn1_tab[] = {
+static const asn1_static_node pkix_asn1_tab[] = {
   {"PKIX1", 536875024, 0},
   {0, 1073741836, 0},
   {"id-ce", 1879048204, 0},
-- 
2.45.2

[PULL 03/11] crypto: Allow building with GnuTLS but without Libtasn1

[Daniel P. Berrangé]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

We only use Libtasn1 in unit tests. As noted in commit d47b83b118
("tests: add migration tests of TLS with x509 credentials"), having
GnuTLS without Libtasn1 is a valid configuration, so do not require
Libtasn1, to avoid:

  Dependency gnutls found: YES 3.7.1 (cached)
  Run-time dependency libtasn1 found: NO (tried pkgconfig)

  ../meson.build:1914:10: ERROR: Dependency "libtasn1" not found, tried pkgconfig

Fixes: ba7ed407e6 ("configure, meson: convert libtasn1 detection to meson")
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meson.build b/meson.build
index af9f0380e2..4eca361319 100644
--- a/meson.build
+++ b/meson.build
@@ -1979,6 +1979,7 @@ endif
 tasn1 = not_found
 if gnutls.found()
   tasn1 = dependency('libtasn1',
+                     required: false,
                      method: 'pkg-config')
 endif
 keyutils = not_found
-- 
2.45.2

[PULL 04/11] docs/devel: Add introduction to LUKS volume with detached header

[Daniel P. Berrangé]

From: Hyman Huang <yong.huang@smartx.com>

Signed-off-by: Hyman Huang <yong.huang@smartx.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 MAINTAINERS                         |   1 +
 docs/devel/crypto.rst               |  10 ++
 docs/devel/index-internals.rst      |   1 +
 docs/devel/luks-detached-header.rst | 182 ++++++++++++++++++++++++++++
 4 files changed, 194 insertions(+)
 create mode 100644 docs/devel/crypto.rst
 create mode 100644 docs/devel/luks-detached-header.rst

diff --git a/MAINTAINERS b/MAINTAINERS
index 73040829b1..98eddf7ae1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3451,6 +3451,7 @@ Detached LUKS header
 M: Hyman Huang <yong.huang@smartx.com>
 S: Maintained
 F: tests/qemu-iotests/tests/luks-detached-header
+F: docs/devel/luks-detached-header.rst
 
 D-Bus
 M: Marc-André Lureau <marcandre.lureau@redhat.com>
diff --git a/docs/devel/crypto.rst b/docs/devel/crypto.rst
new file mode 100644
index 0000000000..39b1c910e7
--- /dev/null
+++ b/docs/devel/crypto.rst
@@ -0,0 +1,10 @@
+.. _crypto-ref:
+
+====================
+Cryptography in QEMU
+====================
+
+.. toctree::
+   :maxdepth: 2
+
+   luks-detached-header
diff --git a/docs/devel/index-internals.rst b/docs/devel/index-internals.rst
index 5636e9cf1d..4ac7725d72 100644
--- a/docs/devel/index-internals.rst
+++ b/docs/devel/index-internals.rst
@@ -20,3 +20,4 @@ Details about QEMU's various subsystems including how to add features to them.
    vfio-iommufd
    writing-monitor-commands
    virtio-backends
+   crypto
diff --git a/docs/devel/luks-detached-header.rst b/docs/devel/luks-detached-header.rst
new file mode 100644
index 0000000000..94ec285c27
--- /dev/null
+++ b/docs/devel/luks-detached-header.rst
@@ -0,0 +1,182 @@
+================================
+LUKS volume with detached header
+================================
+
+Introduction
+============
+
+This document gives an overview of the design of LUKS volume with detached
+header and how to use it.
+
+Background
+==========
+
+The LUKS format has ability to store the header in a separate volume from
+the payload. We could extend the LUKS driver in QEMU to support this use
+case.
+
+Normally a LUKS volume has a layout:
+
+::
+
+         +-----------------------------------------------+
+         |         |                |                    |
+ disk    | header  |  key material  |  disk payload data |
+         |         |                |                    |
+         +-----------------------------------------------+
+
+With a detached LUKS header, you need 2 disks so getting:
+
+::
+
+         +--------------------------+
+ disk1   |   header  | key material |
+         +--------------------------+
+         +---------------------+
+ disk2   |  disk payload data  |
+         +---------------------+
+
+There are a variety of benefits to doing this:
+
+ * Secrecy - the disk2 cannot be identified as containing LUKS
+             volume since there's no header
+ * Control - if access to the disk1 is restricted, then even
+             if someone has access to disk2 they can't unlock
+             it. Might be useful if you have disks on NFS but
+             want to restrict which host can launch a VM
+             instance from it, by dynamically providing access
+             to the header to a designated host
+ * Flexibility - your application data volume may be a given
+                 size and it is inconvenient to resize it to
+                 add encryption.You can store the LUKS header
+                 separately and use the existing storage
+                 volume for payload
+ * Recovery - corruption of a bit in the header may make the
+              entire payload inaccessible. It might be
+              convenient to take backups of the header. If
+              your primary disk header becomes corrupt, you
+              can unlock the data still by pointing to the
+              backup detached header
+
+Architecture
+============
+
+Take the qcow2 encryption, for example. The architecture of the
+LUKS volume with detached header is shown in the diagram below.
+
+There are two children of the root node: a file and a header.
+Data from the disk payload is stored in the file node. The
+LUKS header and key material are located in the header node,
+as previously mentioned.
+
+::
+
+                       +-----------------------------+
+  Root node            |          foo[luks]          |
+                       +-----------------------------+
+                          |                       |
+                     file |                header |
+                          |                       |
+               +---------------------+    +------------------+
+  Child node   |payload-format[qcow2]|    |header-format[raw]|
+               +---------------------+    +------------------+
+                          |                       |
+                     file |                 file  |
+                          |                       |
+               +----------------------+  +---------------------+
+  Child node   |payload-protocol[file]|  |header-protocol[file]|
+               +----------------------+  +---------------------+
+                          |                       |
+                          |                       |
+                          |                       |
+                     Host storage            Host storage
+
+Usage
+=====
+
+Create a LUKS disk with a detached header using qemu-img
+--------------------------------------------------------
+
+Shell commandline::
+
+  # qemu-img create --object secret,id=sec0,data=abc123 -f luks \
+    -o cipher-alg=aes-256,cipher-mode=xts -o key-secret=sec0 \
+    -o detached-header=true test-header.img
+  # qemu-img create -f qcow2 test-payload.qcow2 200G
+  # qemu-img info 'json:{"driver":"luks","file":{"filename": \
+    "test-payload.img"},"header":{"filename":"test-header.img"}}'
+
+Set up a VM's LUKS volume with a detached header
+------------------------------------------------
+
+Qemu commandline::
+
+  # qemu-system-x86_64 ... \
+    -object '{"qom-type":"secret","id":"libvirt-3-format-secret", \
+    "data":"abc123"}' \
+    -blockdev '{"driver":"file","filename":"/path/to/test-header.img", \
+    "node-name":"libvirt-1-storage"}' \
+    -blockdev '{"node-name":"libvirt-1-format","read-only":false, \
+    "driver":"raw","file":"libvirt-1-storage"}' \
+    -blockdev '{"driver":"file","filename":"/path/to/test-payload.qcow2", \
+    "node-name":"libvirt-2-storage"}' \
+    -blockdev '{"node-name":"libvirt-2-format","read-only":false, \
+    "driver":"qcow2","file":"libvirt-2-storage"}' \
+    -blockdev '{"node-name":"libvirt-3-format","driver":"luks", \
+    "file":"libvirt-2-format","header":"libvirt-1-format","key-secret": \
+    "libvirt-3-format-secret"}' \
+    -device '{"driver":"virtio-blk-pci","bus":XXX,"addr":YYY,"drive": \
+    "libvirt-3-format","id":"virtio-disk1"}'
+
+Add LUKS volume to a VM with a detached header
+----------------------------------------------
+
+1. object-add the secret for decrypting the cipher stored in
+   LUKS header above::
+
+    # virsh qemu-monitor-command vm '{"execute":"object-add", \
+      "arguments":{"qom-type":"secret", "id": \
+      "libvirt-4-format-secret", "data":"abc123"}}'
+
+2. block-add the protocol node for LUKS header::
+
+    # virsh qemu-monitor-command vm '{"execute":"blockdev-add", \
+      "arguments":{"node-name":"libvirt-1-storage", "driver":"file", \
+      "filename": "/path/to/test-header.img" }}'
+
+3. block-add the raw-drived node for LUKS header::
+
+    # virsh qemu-monitor-command vm '{"execute":"blockdev-add", \
+      "arguments":{"node-name":"libvirt-1-format", "driver":"raw", \
+      "file":"libvirt-1-storage"}}'
+
+4. block-add the protocol node for disk payload image::
+
+    # virsh qemu-monitor-command vm '{"execute":"blockdev-add", \
+      "arguments":{"node-name":"libvirt-2-storage", "driver":"file", \
+      "filename":"/path/to/test-payload.qcow2"}}'
+
+5. block-add the qcow2-drived format node for disk payload data::
+
+    # virsh qemu-monitor-command vm '{"execute":"blockdev-add", \
+      "arguments":{"node-name":"libvirt-2-format", "driver":"qcow2", \
+      "file":"libvirt-2-storage"}}'
+
+6. block-add the luks-drived format node to link the qcow2 disk
+   with the LUKS header by specifying the field "header"::
+
+    # virsh qemu-monitor-command vm '{"execute":"blockdev-add", \
+      "arguments":{"node-name":"libvirt-3-format", "driver":"luks", \
+      "file":"libvirt-2-format", "header":"libvirt-1-format", \
+      "key-secret":"libvirt-2-format-secret"}}'
+
+7. hot-plug the virtio-blk device finally::
+
+    # virsh qemu-monitor-command vm '{"execute":"device_add", \
+      "arguments": {"driver":"virtio-blk-pci", \
+      "drive": "libvirt-3-format", "id":"virtio-disk2"}}
+
+TODO
+====
+
+1. Support the shared detached LUKS header within the VM.
-- 
2.45.2

[PULL 05/11] meson.build: fix libgcrypt detection on system without libgcrypt-config

[Daniel P. Berrangé]

From: Yao Zi <ziyao@disroot.org>

libgcrypt starts providing correct pkg-config configuration since 1.9,
in parallel with libgcrypt-config. Since 1.11 it may also stop
installing libgcrypt-config in some scenarios. Use the auto method for
detection of libgcrypt, in which meson will try both pkg-config and
libgcrypt-config.

Auto method for libgcrypt is supported by meson since 0.49.0, which is
higher than the version qemu requires.

Signed-off-by: Yao Zi <ziyao@disroot.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build | 1 -
 1 file changed, 1 deletion(-)

diff --git a/meson.build b/meson.build
index 4eca361319..ec6fb7d69c 100644
--- a/meson.build
+++ b/meson.build
@@ -1696,7 +1696,6 @@ endif
 if not gnutls_crypto.found()
   if (not get_option('gcrypt').auto() or have_system) and not get_option('nettle').enabled()
     gcrypt = dependency('libgcrypt', version: '>=1.8',
-                        method: 'config-tool',
                         required: get_option('gcrypt'))
     # Debian has removed -lgpg-error from libgcrypt-config
     # as it "spreads unnecessary dependencies" which in
-- 
2.45.2

[PULL 06/11] qapi: drop unused QCryptoBlockCreateOptionsLUKS.detached-header

[Daniel P. Berrangé]

The 'detached-header' field in QCryptoBlockCreateOptionsLUKS
was left over from earlier patch iterations.

Acked-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qapi/crypto.json | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/qapi/crypto.json b/qapi/crypto.json
index e102be337b..f03bdab8c9 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -226,8 +226,6 @@
 # @iter-time: number of milliseconds to spend in PBKDF passphrase
 #     processing.  Currently defaults to 2000.  (since 2.8)
 #
-# @detached-header: create a detached LUKS header.  (since 9.0)
-#
 # Since: 2.6
 ##
 { 'struct': 'QCryptoBlockCreateOptionsLUKS',
@@ -237,8 +235,7 @@
             '*ivgen-alg': 'QCryptoIVGenAlgorithm',
             '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
             '*hash-alg': 'QCryptoHashAlgorithm',
-            '*iter-time': 'int',
-            '*detached-header': 'bool'}}
+            '*iter-time': 'int' }}
 
 ##
 # @QCryptoBlockOpenOptions:
-- 
2.45.2

[PULL 07/11] meson: build chardev trace files when have_block

[Daniel P. Berrangé]

The QSD depends on chardev code, and is built when have_tools is
true. This means conditionalizing chardev trace on have_system
is wrong, we need have_block which is set have_system || have_tools.

This latent bug was historically harmless because only the spice
chardev included tracing, which wasn't built in a !have_system
scenario.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index ec6fb7d69c..5613b62a4f 100644
--- a/meson.build
+++ b/meson.build
@@ -3343,6 +3343,7 @@ if have_block
   trace_events_subdirs += [
     'authz',
     'block',
+    'chardev',
     'io',
     'nbd',
     'scsi',
@@ -3354,7 +3355,6 @@ if have_system
     'audio',
     'backends',
     'backends/tpm',
-    'chardev',
     'ebpf',
     'hw/9pfs',
     'hw/acpi',
-- 
2.45.2

[PULL 08/11] chardev: add tracing of socket error conditions

[Daniel P. Berrangé]

This adds trace points to every error scenario in the chardev socket
backend that can lead to termination of the connection.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 37 ++++++++++++++++++++++++++-----------
 chardev/trace-events  | 10 ++++++++++
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 812d7aa38a..1ca9441b1b 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -33,6 +33,7 @@
 #include "qapi/clone-visitor.h"
 #include "qapi/qapi-visit-sockets.h"
 #include "qemu/yank.h"
+#include "trace.h"
 
 #include "chardev/char-io.h"
 #include "chardev/char-socket.h"
@@ -126,6 +127,7 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
         if (ret < 0 && errno != EAGAIN) {
             if (tcp_chr_read_poll(chr) <= 0) {
                 /* Perform disconnect and return error. */
+                trace_chr_socket_poll_err(chr, chr->label);
                 tcp_chr_disconnect_locked(chr);
             } /* else let the read handler finish it properly */
         }
@@ -279,15 +281,16 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
     size_t i;
     int *msgfds = NULL;
     size_t msgfds_num = 0;
+    Error *err = NULL;
 
     if (qio_channel_has_feature(s->ioc, QIO_CHANNEL_FEATURE_FD_PASS)) {
         ret = qio_channel_readv_full(s->ioc, &iov, 1,
                                      &msgfds, &msgfds_num,
-                                     0, NULL);
+                                     0, &err);
     } else {
         ret = qio_channel_readv_full(s->ioc, &iov, 1,
                                      NULL, NULL,
-                                     0, NULL);
+                                     0, &err);
     }
 
     if (msgfds_num) {
@@ -322,7 +325,11 @@ static ssize_t tcp_chr_recv(Chardev *chr, char *buf, size_t len)
         errno = EAGAIN;
         ret = -1;
     } else if (ret == -1) {
+        trace_chr_socket_recv_err(chr, chr->label, error_get_pretty(err));
+        error_free(err);
         errno = EIO;
+    } else if (ret == 0) {
+        trace_chr_socket_recv_eof(chr, chr->label);
     }
 
     return ret;
@@ -463,6 +470,7 @@ static void tcp_chr_disconnect_locked(Chardev *chr)
     SocketChardev *s = SOCKET_CHARDEV(chr);
     bool emit_close = s->state == TCP_CHARDEV_STATE_CONNECTED;
 
+    trace_chr_socket_disconnect(chr, chr->label);
     tcp_chr_free_connection(chr);
 
     if (s->listener) {
@@ -521,6 +529,7 @@ static gboolean tcp_chr_hup(QIOChannel *channel,
                                void *opaque)
 {
     Chardev *chr = CHARDEV(opaque);
+    trace_chr_socket_hangup(chr, chr->label);
     tcp_chr_disconnect(chr);
     return G_SOURCE_REMOVE;
 }
@@ -672,15 +681,18 @@ static gboolean tcp_chr_telnet_init_io(QIOChannel *ioc,
     SocketChardev *s = user_data;
     Chardev *chr = CHARDEV(s);
     TCPChardevTelnetInit *init = s->telnet_init;
+    Error *err = NULL;
     ssize_t ret;
 
     assert(init);
 
-    ret = qio_channel_write(ioc, init->buf, init->buflen, NULL);
+    ret = qio_channel_write(ioc, init->buf, init->buflen, &err);
     if (ret < 0) {
         if (ret == QIO_CHANNEL_ERR_BLOCK) {
             ret = 0;
         } else {
+            trace_chr_socket_write_err(chr, chr->label, error_get_pretty(err));
+            error_free(err);
             tcp_chr_disconnect(chr);
             goto end;
         }
@@ -765,9 +777,9 @@ static void tcp_chr_websock_handshake(QIOTask *task, gpointer user_data)
     Error *err = NULL;
 
     if (qio_task_propagate_error(task, &err)) {
-        error_reportf_err(err,
-                          "websock handshake of character device %s failed: ",
-                          chr->label);
+        trace_chr_socket_ws_handshake_err(chr, chr->label,
+                                          error_get_pretty(err));
+        error_free(err);
         tcp_chr_disconnect(chr);
     } else {
         if (s->do_telnetopt) {
@@ -805,9 +817,9 @@ static void tcp_chr_tls_handshake(QIOTask *task,
     Error *err = NULL;
 
     if (qio_task_propagate_error(task, &err)) {
-        error_reportf_err(err,
-                          "TLS handshake of character device %s failed: ",
-                          chr->label);
+        trace_chr_socket_tls_handshake_err(chr, chr->label,
+                                           error_get_pretty(err));
+        error_free(err);
         tcp_chr_disconnect(chr);
     } else {
         if (s->is_websock) {
@@ -826,19 +838,22 @@ static void tcp_chr_tls_init(Chardev *chr)
     SocketChardev *s = SOCKET_CHARDEV(chr);
     QIOChannelTLS *tioc;
     gchar *name;
+    Error *err = NULL;
 
     if (s->is_listen) {
         tioc = qio_channel_tls_new_server(
             s->ioc, s->tls_creds,
             s->tls_authz,
-            NULL);
+            &err);
     } else {
         tioc = qio_channel_tls_new_client(
             s->ioc, s->tls_creds,
             s->addr->u.inet.host,
-            NULL);
+            &err);
     }
     if (tioc == NULL) {
+        trace_chr_socket_tls_init_err(chr, chr->label, error_get_pretty(err));
+        error_free(err);
         tcp_chr_disconnect(chr);
         return;
     }
diff --git a/chardev/trace-events b/chardev/trace-events
index 027107b0c1..7e97b8a988 100644
--- a/chardev/trace-events
+++ b/chardev/trace-events
@@ -17,3 +17,13 @@ spice_vmc_register_interface(void *scd) "spice vmc registered interface %p"
 spice_vmc_unregister_interface(void *scd) "spice vmc unregistered interface %p"
 spice_vmc_event(int event) "spice vmc event %d"
 
+# char-socket.c
+chr_socket_poll_err(void *chrdev, const char *label) "chardev socket poll error %p (%s)"
+chr_socket_recv_err(void *chrdev, const char *label, const char *err) "chardev socket recv error %p (%s): %s"
+chr_socket_recv_eof(void *chrdev, const char *label) "chardev socket recv end-of-file %p (%s)"
+chr_socket_write_err(void *chrdev, const char *label, const char *err) "chardev socket write error %p (%s): %s"
+chr_socket_disconnect(void *chrdev, const char *label) "chardev socket disconnect %p (%s)"
+chr_socket_hangup(void *chrdev, const char *label) "chardev socket hangup %p (%s)"
+chr_socket_ws_handshake_err(void *chrdev, const char *label, const char *err) "chardev socket websock handshake error %p (%s): %s"
+chr_socket_tls_handshake_err(void *chrdev, const char *label, const char *err) "chardev socket TLS handshake error %p (%s): %s"
+chr_socket_tls_init_err(void *chrdev, const char *label, const char *err) "chardev socket TLS init error %p (%s): %s"
-- 
2.45.2

[PULL 09/11] crypto: drop gnutls debug logging support

[Daniel P. Berrangé]

GNUTLS already supports dynamically enabling its logging at runtime by
setting the env var 'GNUTLS_DEBUG_LEVEL=10', so there is no need to
re-invent this logic in QEMU in a way that requires a re-compile.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/init.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/crypto/init.c b/crypto/init.c
index fb7f1bff10..674d237fa9 100644
--- a/crypto/init.c
+++ b/crypto/init.c
@@ -34,14 +34,11 @@
 
 #include "crypto/random.h"
 
-/* #define DEBUG_GNUTLS */
-#ifdef DEBUG_GNUTLS
-static void qcrypto_gnutls_log(int level, const char *str)
-{
-    fprintf(stderr, "%d: %s", level, str);
-}
-#endif
 
+/*
+ * To debug GNUTLS see env vars listed in
+ * https://gnutls.org/manual/html_node/Debugging-and-auditing.html
+ */
 int qcrypto_init(Error **errp)
 {
 #ifdef CONFIG_GNUTLS
@@ -53,10 +50,6 @@ int qcrypto_init(Error **errp)
                    gnutls_strerror(ret));
         return -1;
     }
-#ifdef DEBUG_GNUTLS
-    gnutls_global_set_log_level(10);
-    gnutls_global_set_log_function(qcrypto_gnutls_log);
-#endif
 #endif
 
 #ifdef CONFIG_GCRYPT
-- 
2.45.2

[PULL 10/11] crypto: push error reporting into TLS session I/O APIs

[Daniel P. Berrangé]

The current TLS session I/O APIs just return a synthetic errno
value on error, which has been translated from a gnutls error
value. This looses a large amount of valuable information that
distinguishes different scenarios.

Pushing population of the "Error *errp" object into the TLS
session I/O APIs gives more detailed error information.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlssession.c         | 60 ++++++++++++++++++-------------------
 include/crypto/tlssession.h | 23 +++++++++++---
 io/channel-tls.c            | 48 +++++++++++++----------------
 3 files changed, 68 insertions(+), 63 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 1e98f44e0d..926f19c115 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -441,23 +441,20 @@ qcrypto_tls_session_set_callbacks(QCryptoTLSSession *session,
 ssize_t
 qcrypto_tls_session_write(QCryptoTLSSession *session,
                           const char *buf,
-                          size_t len)
+                          size_t len,
+                          Error **errp)
 {
     ssize_t ret = gnutls_record_send(session->handle, buf, len);
 
     if (ret < 0) {
-        switch (ret) {
-        case GNUTLS_E_AGAIN:
-            errno = EAGAIN;
-            break;
-        case GNUTLS_E_INTERRUPTED:
-            errno = EINTR;
-            break;
-        default:
-            errno = EIO;
-            break;
+        if (ret == GNUTLS_E_AGAIN) {
+            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
+        } else {
+            error_setg(errp,
+                       "Cannot write to TLS channel: %s",
+                       gnutls_strerror(ret));
+            return -1;
         }
-        ret = -1;
     }
 
     return ret;
@@ -467,26 +464,24 @@ qcrypto_tls_session_write(QCryptoTLSSession *session,
 ssize_t
 qcrypto_tls_session_read(QCryptoTLSSession *session,
                          char *buf,
-                         size_t len)
+                         size_t len,
+                         bool gracefulTermination,
+                         Error **errp)
 {
     ssize_t ret = gnutls_record_recv(session->handle, buf, len);
 
     if (ret < 0) {
-        switch (ret) {
-        case GNUTLS_E_AGAIN:
-            errno = EAGAIN;
-            break;
-        case GNUTLS_E_INTERRUPTED:
-            errno = EINTR;
-            break;
-        case GNUTLS_E_PREMATURE_TERMINATION:
-            errno = ECONNABORTED;
-            break;
-        default:
-            errno = EIO;
-            break;
+        if (ret == GNUTLS_E_AGAIN) {
+            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
+        } else if ((ret == GNUTLS_E_PREMATURE_TERMINATION) &&
+                   gracefulTermination){
+            return 0;
+        } else {
+            error_setg(errp,
+                       "Cannot read from TLS channel: %s",
+                       gnutls_strerror(ret));
+            return -1;
         }
-        ret = -1;
     }
 
     return ret;
@@ -605,9 +600,10 @@ qcrypto_tls_session_set_callbacks(
 ssize_t
 qcrypto_tls_session_write(QCryptoTLSSession *sess,
                           const char *buf,
-                          size_t len)
+                          size_t len,
+                          Error **errp)
 {
-    errno = -EIO;
+    error_setg(errp, "TLS requires GNUTLS support");
     return -1;
 }
 
@@ -615,9 +611,11 @@ qcrypto_tls_session_write(QCryptoTLSSession *sess,
 ssize_t
 qcrypto_tls_session_read(QCryptoTLSSession *sess,
                          char *buf,
-                         size_t len)
+                         size_t len,
+                         bool gracefulTermination,
+                         Error **errp)
 {
-    errno = -EIO;
+    error_setg(errp, "TLS requires GNUTLS support");
     return -1;
 }
 
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index 571049bd0e..291e602540 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -107,6 +107,7 @@
 
 typedef struct QCryptoTLSSession QCryptoTLSSession;
 
+#define QCRYPTO_TLS_SESSION_ERR_BLOCK -2
 
 /**
  * qcrypto_tls_session_new:
@@ -212,6 +213,7 @@ void qcrypto_tls_session_set_callbacks(QCryptoTLSSession *sess,
  * @sess: the TLS session object
  * @buf: the plain text to send
  * @len: the length of @buf
+ * @errp: pointer to hold returned error object
  *
  * Encrypt @len bytes of the data in @buf and send
  * it to the remote peer using the callback previously
@@ -221,32 +223,45 @@ void qcrypto_tls_session_set_callbacks(QCryptoTLSSession *sess,
  * qcrypto_tls_session_get_handshake_status() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
  *
- * Returns: the number of bytes sent, or -1 on error
+ * Returns: the number of bytes sent,
+ * or QCRYPTO_TLS_SESSION_ERR_BLOCK if the write would block,
+ * or -1 on error.
  */
 ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
                                   const char *buf,
-                                  size_t len);
+                                  size_t len,
+                                  Error **errp);
 
 /**
  * qcrypto_tls_session_read:
  * @sess: the TLS session object
  * @buf: to fill with plain text received
  * @len: the length of @buf
+ * @gracefulTermination: treat premature termination as graceful EOF
+ * @errp: pointer to hold returned error object
  *
  * Receive up to @len bytes of data from the remote peer
  * using the callback previously registered with
  * qcrypto_tls_session_set_callbacks(), decrypt it and
  * store it in @buf.
  *
+ * If @gracefulTermination is true, then a premature termination
+ * of the TLS session will be treated as indicating EOF, as
+ * opposed to an error.
+ *
  * It is an error to call this before
  * qcrypto_tls_session_get_handshake_status() returns
  * QCRYPTO_TLS_HANDSHAKE_COMPLETE
  *
- * Returns: the number of bytes received, or -1 on error
+ * Returns: the number of bytes received,
+ * or QCRYPTO_TLS_SESSION_ERR_BLOCK if the receive would block,
+ * or -1 on error.
  */
 ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
                                  char *buf,
-                                 size_t len);
+                                 size_t len,
+                                 bool gracefulTermination,
+                                 Error **errp);
 
 /**
  * qcrypto_tls_session_check_pending:
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 67b9700006..9d8bb158d1 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -277,24 +277,19 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
     ssize_t got = 0;
 
     for (i = 0 ; i < niov ; i++) {
-        ssize_t ret = qcrypto_tls_session_read(tioc->session,
-                                               iov[i].iov_base,
-                                               iov[i].iov_len);
-        if (ret < 0) {
-            if (errno == EAGAIN) {
-                if (got) {
-                    return got;
-                } else {
-                    return QIO_CHANNEL_ERR_BLOCK;
-                }
-            } else if (errno == ECONNABORTED &&
-                       (qatomic_load_acquire(&tioc->shutdown) &
-                        QIO_CHANNEL_SHUTDOWN_READ)) {
-                return 0;
+        ssize_t ret = qcrypto_tls_session_read(
+            tioc->session,
+            iov[i].iov_base,
+            iov[i].iov_len,
+            qatomic_load_acquire(&tioc->shutdown) & QIO_CHANNEL_SHUTDOWN_READ,
+            errp);
+        if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
+            if (got) {
+                return got;
+            } else {
+                return QIO_CHANNEL_ERR_BLOCK;
             }
-
-            error_setg_errno(errp, errno,
-                             "Cannot read from TLS channel");
+        } else if (ret < 0) {
             return -1;
         }
         got += ret;
@@ -321,18 +316,15 @@ static ssize_t qio_channel_tls_writev(QIOChannel *ioc,
     for (i = 0 ; i < niov ; i++) {
         ssize_t ret = qcrypto_tls_session_write(tioc->session,
                                                 iov[i].iov_base,
-                                                iov[i].iov_len);
-        if (ret <= 0) {
-            if (errno == EAGAIN) {
-                if (done) {
-                    return done;
-                } else {
-                    return QIO_CHANNEL_ERR_BLOCK;
-                }
+                                                iov[i].iov_len,
+                                                errp);
+        if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
+            if (done) {
+                return done;
+            } else {
+                return QIO_CHANNEL_ERR_BLOCK;
             }
-
-            error_setg_errno(errp, errno,
-                             "Cannot write to TLS channel");
+        } else if (ret < 0) {
             return -1;
         }
         done += ret;
-- 
2.45.2

[PULL 11/11] crypto: propagate errors from TLS session I/O callbacks

[Daniel P. Berrangé]

GNUTLS doesn't know how to perform I/O on anything other than plain
FDs, so the TLS session provides it with some I/O callbacks. The
GNUTLS API design requires these callbacks to return a unix errno
value, which means we're currently loosing the useful QEMU "Error"
object.

This changes the I/O callbacks in QEMU to stash the "Error" object
in the QCryptoTLSSession class, and fetch it when seeing an I/O
error returned from GNUTLS, thus preserving useful error messages.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlssession.c                 | 76 +++++++++++++++++++++++++----
 include/crypto/tlssession.h         | 10 +++-
 io/channel-tls.c                    | 18 +++----
 tests/unit/test-crypto-tlssession.c | 30 ++++++++++--
 4 files changed, 108 insertions(+), 26 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 926f19c115..77286e23f4 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -44,6 +44,13 @@ struct QCryptoTLSSession {
     QCryptoTLSSessionReadFunc readFunc;
     void *opaque;
     char *peername;
+
+    /*
+     * Allow concurrent reads and writes, so track
+     * errors separately
+     */
+    Error *rerr;
+    Error *werr;
 };
 
 
@@ -54,6 +61,9 @@ qcrypto_tls_session_free(QCryptoTLSSession *session)
         return;
     }
 
+    error_free(session->rerr);
+    error_free(session->werr);
+
     gnutls_deinit(session->handle);
     g_free(session->hostname);
     g_free(session->peername);
@@ -67,13 +77,26 @@ static ssize_t
 qcrypto_tls_session_push(void *opaque, const void *buf, size_t len)
 {
     QCryptoTLSSession *session = opaque;
+    ssize_t ret;
 
     if (!session->writeFunc) {
         errno = EIO;
         return -1;
     };
 
-    return session->writeFunc(buf, len, session->opaque);
+    error_free(session->werr);
+    session->werr = NULL;
+
+    ret = session->writeFunc(buf, len, session->opaque, &session->werr);
+    if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
+        errno = EAGAIN;
+        return -1;
+    } else if (ret < 0) {
+        errno = EIO;
+        return -1;
+    } else {
+        return ret;
+    }
 }
 
 
@@ -81,13 +104,26 @@ static ssize_t
 qcrypto_tls_session_pull(void *opaque, void *buf, size_t len)
 {
     QCryptoTLSSession *session = opaque;
+    ssize_t ret;
 
     if (!session->readFunc) {
         errno = EIO;
         return -1;
     };
 
-    return session->readFunc(buf, len, session->opaque);
+    error_free(session->rerr);
+    session->rerr = NULL;
+
+    ret = session->readFunc(buf, len, session->opaque, &session->rerr);
+    if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
+        errno = EAGAIN;
+        return -1;
+    } else if (ret < 0) {
+        errno = EIO;
+        return -1;
+    } else {
+        return ret;
+    }
 }
 
 #define TLS_PRIORITY_ADDITIONAL_ANON "+ANON-DH"
@@ -450,9 +486,14 @@ qcrypto_tls_session_write(QCryptoTLSSession *session,
         if (ret == GNUTLS_E_AGAIN) {
             return QCRYPTO_TLS_SESSION_ERR_BLOCK;
         } else {
-            error_setg(errp,
-                       "Cannot write to TLS channel: %s",
-                       gnutls_strerror(ret));
+            if (session->werr) {
+                error_propagate(errp, session->werr);
+                session->werr = NULL;
+            } else {
+                error_setg(errp,
+                           "Cannot write to TLS channel: %s",
+                           gnutls_strerror(ret));
+            }
             return -1;
         }
     }
@@ -477,9 +518,14 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
                    gracefulTermination){
             return 0;
         } else {
-            error_setg(errp,
-                       "Cannot read from TLS channel: %s",
-                       gnutls_strerror(ret));
+            if (session->rerr) {
+                error_propagate(errp, session->rerr);
+                session->rerr = NULL;
+            } else {
+                error_setg(errp,
+                           "Cannot read from TLS channel: %s",
+                           gnutls_strerror(ret));
+            }
             return -1;
         }
     }
@@ -507,11 +553,21 @@ qcrypto_tls_session_handshake(QCryptoTLSSession *session,
             ret == GNUTLS_E_AGAIN) {
             ret = 1;
         } else {
-            error_setg(errp, "TLS handshake failed: %s",
-                       gnutls_strerror(ret));
+            if (session->rerr || session->werr) {
+                error_setg(errp, "TLS handshake failed: %s: %s",
+                           gnutls_strerror(ret),
+                           error_get_pretty(session->rerr ?
+                                            session->rerr : session->werr));
+            } else {
+                error_setg(errp, "TLS handshake failed: %s",
+                           gnutls_strerror(ret));
+            }
             ret = -1;
         }
     }
+    error_free(session->rerr);
+    error_free(session->werr);
+    session->rerr = session->werr = NULL;
 
     return ret;
 }
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index 291e602540..f694a5c3c5 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -178,12 +178,18 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoTLSSession, qcrypto_tls_session_free)
 int qcrypto_tls_session_check_credentials(QCryptoTLSSession *sess,
                                           Error **errp);
 
+/*
+ * These must return QCRYPTO_TLS_SESSION_ERR_BLOCK if the I/O
+ * would block, but on other errors, must fill 'errp'
+ */
 typedef ssize_t (*QCryptoTLSSessionWriteFunc)(const char *buf,
                                               size_t len,
-                                              void *opaque);
+                                              void *opaque,
+                                              Error **errp);
 typedef ssize_t (*QCryptoTLSSessionReadFunc)(char *buf,
                                              size_t len,
-                                             void *opaque);
+                                             void *opaque,
+                                             Error **errp);
 
 /**
  * qcrypto_tls_session_set_callbacks:
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 9d8bb158d1..aab630e5ae 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -28,17 +28,16 @@
 
 static ssize_t qio_channel_tls_write_handler(const char *buf,
                                              size_t len,
-                                             void *opaque)
+                                             void *opaque,
+                                             Error **errp)
 {
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(opaque);
     ssize_t ret;
 
-    ret = qio_channel_write(tioc->master, buf, len, NULL);
+    ret = qio_channel_write(tioc->master, buf, len, errp);
     if (ret == QIO_CHANNEL_ERR_BLOCK) {
-        errno = EAGAIN;
-        return -1;
+        return QCRYPTO_TLS_SESSION_ERR_BLOCK;
     } else if (ret < 0) {
-        errno = EIO;
         return -1;
     }
     return ret;
@@ -46,17 +45,16 @@ static ssize_t qio_channel_tls_write_handler(const char *buf,
 
 static ssize_t qio_channel_tls_read_handler(char *buf,
                                             size_t len,
-                                            void *opaque)
+                                            void *opaque,
+                                            Error **errp)
 {
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(opaque);
     ssize_t ret;
 
-    ret = qio_channel_read(tioc->master, buf, len, NULL);
+    ret = qio_channel_read(tioc->master, buf, len, errp);
     if (ret == QIO_CHANNEL_ERR_BLOCK) {
-        errno = EAGAIN;
-        return -1;
+        return QCRYPTO_TLS_SESSION_ERR_BLOCK;
     } else if (ret < 0) {
-        errno = EIO;
         return -1;
     }
     return ret;
diff --git a/tests/unit/test-crypto-tlssession.c b/tests/unit/test-crypto-tlssession.c
index b12e7b6879..3395f73560 100644
--- a/tests/unit/test-crypto-tlssession.c
+++ b/tests/unit/test-crypto-tlssession.c
@@ -35,18 +35,40 @@
 #define PSKFILE WORKDIR "keys.psk"
 #define KEYFILE WORKDIR "key-ctx.pem"
 
-static ssize_t testWrite(const char *buf, size_t len, void *opaque)
+static ssize_t
+testWrite(const char *buf, size_t len, void *opaque, Error **errp)
 {
     int *fd = opaque;
+    int ret;
 
-    return write(*fd, buf, len);
+    ret = write(*fd, buf, len);
+    if (ret < 0) {
+        if (errno == EAGAIN) {
+            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
+        } else {
+            error_setg_errno(errp, errno, "unable to write");
+            return -1;
+        }
+    }
+    return ret;
 }
 
-static ssize_t testRead(char *buf, size_t len, void *opaque)
+static ssize_t
+testRead(char *buf, size_t len, void *opaque, Error **errp)
 {
     int *fd = opaque;
+    int ret;
 
-    return read(*fd, buf, len);
+    ret = read(*fd, buf, len);
+    if (ret < 0) {
+        if (errno == EAGAIN) {
+            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
+        } else {
+            error_setg_errno(errp, errno, "unable to read");
+            return -1;
+        }
+    }
+    return ret;
 }
 
 static QCryptoTLSCreds *test_tls_creds_psk_create(
-- 
2.45.2

Re: [PULL 00/11] Crypto patches

[Richard Henderson]

On 7/24/24 19:46, Daniel P. Berrangé wrote:
> The following changes since commit 6410f877f5ed535acd01bbfaa4baec379e44d0ef:
> 
>    Merge tag 'hw-misc-20240723' ofhttps://github.com/philmd/qemu into staging (2024-07-24 15:39:43 +1000)
> 
> are available in the Git repository at:
> 
>    https://gitlab.com/berrange/qemu tags/misc-fixes-pull-request
> 
> for you to fetch changes up to 97f7bf113eb50fcdaf0c73aa2ee01e5355abc073:
> 
>    crypto: propagate errors from TLS session I/O callbacks (2024-07-24 10:39:10 +0100)
> 
> ----------------------------------------------------------------
> 
> * Drop unused 'detached-header' QAPI field from LUKS create options
> * Improve tracing of TLS sockets and TLS chardevs
> * Improve error messages from TLS I/O failures
> * Add docs about use of LUKS detached header options
> * Allow building without libtasn1, but with GNUTLS
> * Fix detection of libgcrypt when libgcrypt-config is absent

Applied, thanks.  Please update https://wiki.qemu.org/ChangeLog/9.1 as appropriate.

r~

Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs

[Thomas Huth]

On 24/07/2024 11.47, Daniel P. Berrangé wrote:
> The current TLS session I/O APIs just return a synthetic errno
> value on error, which has been translated from a gnutls error
> value. This looses a large amount of valuable information that
> distinguishes different scenarios.
> 
> Pushing population of the "Error *errp" object into the TLS
> session I/O APIs gives more detailed error information.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---

  Hi Daniel!

iotest 233 is failing for me with -raw now, and bisection
points to this commit. Output is:

--- .../qemu/tests/qemu-iotests/233.out
+++ /tmp/qemu/tests/qemu-iotests/scratch/raw-file-233/233.out.bad
@@ -69,8 +69,8 @@
  1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)

  == check TLS with authorization ==
-qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
-qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.
+qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.

  == check TLS fail over UNIX with no hostname ==
  qemu-img: Could not open 'driver=nbd,path=SOCK_DIR/qemu-nbd.sock,tls-creds=tls0': No hostname for certificate validation
@@ -103,14 +103,14 @@
  qemu-nbd: TLS handshake failed: The TLS connection was non-properly terminated.

  == final server log ==
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: The TLS connection was non-properly terminated.
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: The TLS connection was non-properly terminated.
  qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
  qemu-nbd: option negotiation failed: Verify failed: No certificate was found.
  qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied
  qemu-nbd: option negotiation failed: TLS x509 authz check for DISTINGUISHED-NAME is denied
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
-qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: Software caused connection abort
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: The TLS connection was non-properly terminated.
+qemu-nbd: option negotiation failed: Failed to read opts magic: Cannot read from TLS channel: The TLS connection was non-properly terminated.
  qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received.
  qemu-nbd: option negotiation failed: TLS handshake failed: An illegal parameter has been received.
  *** done

Could you please have a look?

  Thanks,
   Thomas

> 
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> index 1e98f44e0d..926f19c115 100644
> --- a/crypto/tlssession.c
> +++ b/crypto/tlssession.c
> @@ -441,23 +441,20 @@ qcrypto_tls_session_set_callbacks(QCryptoTLSSession *session,
>   ssize_t
>   qcrypto_tls_session_write(QCryptoTLSSession *session,
>                             const char *buf,
> -                          size_t len)
> +                          size_t len,
> +                          Error **errp)
>   {
>       ssize_t ret = gnutls_record_send(session->handle, buf, len);
>   
>       if (ret < 0) {
> -        switch (ret) {
> -        case GNUTLS_E_AGAIN:
> -            errno = EAGAIN;
> -            break;
> -        case GNUTLS_E_INTERRUPTED:
> -            errno = EINTR;
> -            break;
> -        default:
> -            errno = EIO;
> -            break;
> +        if (ret == GNUTLS_E_AGAIN) {
> +            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
> +        } else {
> +            error_setg(errp,
> +                       "Cannot write to TLS channel: %s",
> +                       gnutls_strerror(ret));
> +            return -1;
>           }
> -        ret = -1;
>       }
>   
>       return ret;
> @@ -467,26 +464,24 @@ qcrypto_tls_session_write(QCryptoTLSSession *session,
>   ssize_t
>   qcrypto_tls_session_read(QCryptoTLSSession *session,
>                            char *buf,
> -                         size_t len)
> +                         size_t len,
> +                         bool gracefulTermination,
> +                         Error **errp)
>   {
>       ssize_t ret = gnutls_record_recv(session->handle, buf, len);
>   
>       if (ret < 0) {
> -        switch (ret) {
> -        case GNUTLS_E_AGAIN:
> -            errno = EAGAIN;
> -            break;
> -        case GNUTLS_E_INTERRUPTED:
> -            errno = EINTR;
> -            break;
> -        case GNUTLS_E_PREMATURE_TERMINATION:
> -            errno = ECONNABORTED;
> -            break;
> -        default:
> -            errno = EIO;
> -            break;
> +        if (ret == GNUTLS_E_AGAIN) {
> +            return QCRYPTO_TLS_SESSION_ERR_BLOCK;
> +        } else if ((ret == GNUTLS_E_PREMATURE_TERMINATION) &&
> +                   gracefulTermination){
> +            return 0;
> +        } else {
> +            error_setg(errp,
> +                       "Cannot read from TLS channel: %s",
> +                       gnutls_strerror(ret));
> +            return -1;
>           }
> -        ret = -1;
>       }
>   
>       return ret;
> @@ -605,9 +600,10 @@ qcrypto_tls_session_set_callbacks(
>   ssize_t
>   qcrypto_tls_session_write(QCryptoTLSSession *sess,
>                             const char *buf,
> -                          size_t len)
> +                          size_t len,
> +                          Error **errp)
>   {
> -    errno = -EIO;
> +    error_setg(errp, "TLS requires GNUTLS support");
>       return -1;
>   }
>   
> @@ -615,9 +611,11 @@ qcrypto_tls_session_write(QCryptoTLSSession *sess,
>   ssize_t
>   qcrypto_tls_session_read(QCryptoTLSSession *sess,
>                            char *buf,
> -                         size_t len)
> +                         size_t len,
> +                         bool gracefulTermination,
> +                         Error **errp)
>   {
> -    errno = -EIO;
> +    error_setg(errp, "TLS requires GNUTLS support");
>       return -1;
>   }
>   
> diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
> index 571049bd0e..291e602540 100644
> --- a/include/crypto/tlssession.h
> +++ b/include/crypto/tlssession.h
> @@ -107,6 +107,7 @@
>   
>   typedef struct QCryptoTLSSession QCryptoTLSSession;
>   
> +#define QCRYPTO_TLS_SESSION_ERR_BLOCK -2
>   
>   /**
>    * qcrypto_tls_session_new:
> @@ -212,6 +213,7 @@ void qcrypto_tls_session_set_callbacks(QCryptoTLSSession *sess,
>    * @sess: the TLS session object
>    * @buf: the plain text to send
>    * @len: the length of @buf
> + * @errp: pointer to hold returned error object
>    *
>    * Encrypt @len bytes of the data in @buf and send
>    * it to the remote peer using the callback previously
> @@ -221,32 +223,45 @@ void qcrypto_tls_session_set_callbacks(QCryptoTLSSession *sess,
>    * qcrypto_tls_session_get_handshake_status() returns
>    * QCRYPTO_TLS_HANDSHAKE_COMPLETE
>    *
> - * Returns: the number of bytes sent, or -1 on error
> + * Returns: the number of bytes sent,
> + * or QCRYPTO_TLS_SESSION_ERR_BLOCK if the write would block,
> + * or -1 on error.
>    */
>   ssize_t qcrypto_tls_session_write(QCryptoTLSSession *sess,
>                                     const char *buf,
> -                                  size_t len);
> +                                  size_t len,
> +                                  Error **errp);
>   
>   /**
>    * qcrypto_tls_session_read:
>    * @sess: the TLS session object
>    * @buf: to fill with plain text received
>    * @len: the length of @buf
> + * @gracefulTermination: treat premature termination as graceful EOF
> + * @errp: pointer to hold returned error object
>    *
>    * Receive up to @len bytes of data from the remote peer
>    * using the callback previously registered with
>    * qcrypto_tls_session_set_callbacks(), decrypt it and
>    * store it in @buf.
>    *
> + * If @gracefulTermination is true, then a premature termination
> + * of the TLS session will be treated as indicating EOF, as
> + * opposed to an error.
> + *
>    * It is an error to call this before
>    * qcrypto_tls_session_get_handshake_status() returns
>    * QCRYPTO_TLS_HANDSHAKE_COMPLETE
>    *
> - * Returns: the number of bytes received, or -1 on error
> + * Returns: the number of bytes received,
> + * or QCRYPTO_TLS_SESSION_ERR_BLOCK if the receive would block,
> + * or -1 on error.
>    */
>   ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
>                                    char *buf,
> -                                 size_t len);
> +                                 size_t len,
> +                                 bool gracefulTermination,
> +                                 Error **errp);
>   
>   /**
>    * qcrypto_tls_session_check_pending:
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 67b9700006..9d8bb158d1 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -277,24 +277,19 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc,
>       ssize_t got = 0;
>   
>       for (i = 0 ; i < niov ; i++) {
> -        ssize_t ret = qcrypto_tls_session_read(tioc->session,
> -                                               iov[i].iov_base,
> -                                               iov[i].iov_len);
> -        if (ret < 0) {
> -            if (errno == EAGAIN) {
> -                if (got) {
> -                    return got;
> -                } else {
> -                    return QIO_CHANNEL_ERR_BLOCK;
> -                }
> -            } else if (errno == ECONNABORTED &&
> -                       (qatomic_load_acquire(&tioc->shutdown) &
> -                        QIO_CHANNEL_SHUTDOWN_READ)) {
> -                return 0;
> +        ssize_t ret = qcrypto_tls_session_read(
> +            tioc->session,
> +            iov[i].iov_base,
> +            iov[i].iov_len,
> +            qatomic_load_acquire(&tioc->shutdown) & QIO_CHANNEL_SHUTDOWN_READ,
> +            errp);
> +        if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
> +            if (got) {
> +                return got;
> +            } else {
> +                return QIO_CHANNEL_ERR_BLOCK;
>               }
> -
> -            error_setg_errno(errp, errno,
> -                             "Cannot read from TLS channel");
> +        } else if (ret < 0) {
>               return -1;
>           }
>           got += ret;
> @@ -321,18 +316,15 @@ static ssize_t qio_channel_tls_writev(QIOChannel *ioc,
>       for (i = 0 ; i < niov ; i++) {
>           ssize_t ret = qcrypto_tls_session_write(tioc->session,
>                                                   iov[i].iov_base,
> -                                                iov[i].iov_len);
> -        if (ret <= 0) {
> -            if (errno == EAGAIN) {
> -                if (done) {
> -                    return done;
> -                } else {
> -                    return QIO_CHANNEL_ERR_BLOCK;
> -                }
> +                                                iov[i].iov_len,
> +                                                errp);
> +        if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
> +            if (done) {
> +                return done;
> +            } else {
> +                return QIO_CHANNEL_ERR_BLOCK;
>               }
> -
> -            error_setg_errno(errp, errno,
> -                             "Cannot write to TLS channel");
> +        } else if (ret < 0) {
>               return -1;
>           }
>           done += ret;

Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs

[Daniel P. Berrangé]

On Mon, Aug 12, 2024 at 05:38:41PM +0200, Thomas Huth wrote:
> On 24/07/2024 11.47, Daniel P. Berrangé wrote:
> > The current TLS session I/O APIs just return a synthetic errno
> > value on error, which has been translated from a gnutls error
> > value. This looses a large amount of valuable information that
> > distinguishes different scenarios.
> > 
> > Pushing population of the "Error *errp" object into the TLS
> > session I/O APIs gives more detailed error information.
> > 
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> 
>  Hi Daniel!
> 
> iotest 233 is failing for me with -raw now, and bisection
> points to this commit. Output is:
> 
> --- .../qemu/tests/qemu-iotests/233.out
> +++ /tmp/qemu/tests/qemu-iotests/scratch/raw-file-233/233.out.bad
> @@ -69,8 +69,8 @@
>  1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> 
>  == check TLS with authorization ==
> -qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
> -qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.

This is an expected change. Previously squashed the real GNUTLS error
into ECONNABORTED:

-        case GNUTLS_E_PREMATURE_TERMINATION:
-            errno = ECONNABORTED;
-            break;


now we report the original gnutls root cause.

IOW, we need to update the expected output files.


With 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: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Aug 12, 2024 at 05:38:41PM +0200, Thomas Huth wrote:
>> On 24/07/2024 11.47, Daniel P. Berrangé wrote:
>> > The current TLS session I/O APIs just return a synthetic errno
>> > value on error, which has been translated from a gnutls error
>> > value. This looses a large amount of valuable information that
>> > distinguishes different scenarios.
>> > 
>> > Pushing population of the "Error *errp" object into the TLS
>> > session I/O APIs gives more detailed error information.
>> > 
>> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> 
>>  Hi Daniel!
>> 
>> iotest 233 is failing for me with -raw now, and bisection
>> points to this commit. Output is:
>> 
>> --- .../qemu/tests/qemu-iotests/233.out
>> +++ /tmp/qemu/tests/qemu-iotests/scratch/raw-file-233/233.out.bad
>> @@ -69,8 +69,8 @@
>>  1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> 
>>  == check TLS with authorization ==
>> -qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
>> -qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
>> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>
> This is an expected change. Previously squashed the real GNUTLS error
> into ECONNABORTED:
>
> -        case GNUTLS_E_PREMATURE_TERMINATION:
> -            errno = ECONNABORTED;
> -            break;
>
>
> now we report the original gnutls root cause.
>
> IOW, we need to update the expected output files.

Has this been done?

Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs

[Thomas Huth]

On 27/08/2024 09.05, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Mon, Aug 12, 2024 at 05:38:41PM +0200, Thomas Huth wrote:
>>> On 24/07/2024 11.47, Daniel P. Berrangé wrote:
>>>> The current TLS session I/O APIs just return a synthetic errno
>>>> value on error, which has been translated from a gnutls error
>>>> value. This looses a large amount of valuable information that
>>>> distinguishes different scenarios.
>>>>
>>>> Pushing population of the "Error *errp" object into the TLS
>>>> session I/O APIs gives more detailed error information.
>>>>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>>> ---
>>>
>>>   Hi Daniel!
>>>
>>> iotest 233 is failing for me with -raw now, and bisection
>>> points to this commit. Output is:
>>>
>>> --- .../qemu/tests/qemu-iotests/233.out
>>> +++ /tmp/qemu/tests/qemu-iotests/scratch/raw-file-233/233.out.bad
>>> @@ -69,8 +69,8 @@
>>>   1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>
>>>   == check TLS with authorization ==
>>> -qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
>>> -qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
>>> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>>> +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.
>>
>> This is an expected change. Previously squashed the real GNUTLS error
>> into ECONNABORTED:
>>
>> -        case GNUTLS_E_PREMATURE_TERMINATION:
>> -            errno = ECONNABORTED;
>> -            break;
>>
>>
>> now we report the original gnutls root cause.
>>
>> IOW, we need to update the expected output files.
> 
> Has this been done?

No, I think the problem still persists.

  Thomas

Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs

[Daniel P. Berrangé]

On Wed, Aug 28, 2024 at 10:32:15AM +0200, Thomas Huth wrote:
> On 27/08/2024 09.05, Markus Armbruster wrote:
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> > 
> > > On Mon, Aug 12, 2024 at 05:38:41PM +0200, Thomas Huth wrote:
> > > > On 24/07/2024 11.47, Daniel P. Berrangé wrote:
> > > > > The current TLS session I/O APIs just return a synthetic errno
> > > > > value on error, which has been translated from a gnutls error
> > > > > value. This looses a large amount of valuable information that
> > > > > distinguishes different scenarios.
> > > > > 
> > > > > Pushing population of the "Error *errp" object into the TLS
> > > > > session I/O APIs gives more detailed error information.
> > > > > 
> > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > ---
> > > > 
> > > >   Hi Daniel!
> > > > 
> > > > iotest 233 is failing for me with -raw now, and bisection
> > > > points to this commit. Output is:
> > > > 
> > > > --- .../qemu/tests/qemu-iotests/233.out
> > > > +++ /tmp/qemu/tests/qemu-iotests/scratch/raw-file-233/233.out.bad
> > > > @@ -69,8 +69,8 @@
> > > >   1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > > 
> > > >   == check TLS with authorization ==
> > > > -qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
> > > > -qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: Software caused connection abort
> > > > +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> > > > +qemu-img: Could not open 'driver=nbd,host=127.0.0.1,port=PORT,tls-creds=tls0': Failed to read option reply: Cannot read from TLS channel: The TLS connection was non-properly terminated.
> > > 
> > > This is an expected change. Previously squashed the real GNUTLS error
> > > into ECONNABORTED:
> > > 
> > > -        case GNUTLS_E_PREMATURE_TERMINATION:
> > > -            errno = ECONNABORTED;
> > > -            break;
> > > 
> > > 
> > > now we report the original gnutls root cause.
> > > 
> > > IOW, we need to update the expected output files.
> > 
> > Has this been done?
> 
> No, I think the problem still persists.

I've just cc'd you both on a patch that fixes this.

With 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 :|