* [PULL 01/11] crypto: Remove 'crypto-tls-x509-helpers.h' from crypto-tls-psk-helpers.c
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
@ 2024-07-24 9:46 ` Daniel P. Berrangé
2024-07-24 9:46 ` [PULL 02/11] crypto: Restrict pkix_asn1_tab[] to crypto-tls-x509-helpers.c Daniel P. Berrangé
` (10 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PULL 02/11] crypto: Restrict pkix_asn1_tab[] to crypto-tls-x509-helpers.c
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
2024-07-24 9:46 ` [PULL 01/11] crypto: Remove 'crypto-tls-x509-helpers.h' from crypto-tls-psk-helpers.c Daniel P. Berrangé
@ 2024-07-24 9:46 ` Daniel P. Berrangé
2024-07-24 9:46 ` [PULL 03/11] crypto: Allow building with GnuTLS but without Libtasn1 Daniel P. Berrangé
` (9 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PULL 03/11] crypto: Allow building with GnuTLS but without Libtasn1
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
2024-07-24 9:46 ` [PULL 01/11] crypto: Remove 'crypto-tls-x509-helpers.h' from crypto-tls-psk-helpers.c Daniel P. Berrangé
2024-07-24 9:46 ` [PULL 02/11] crypto: Restrict pkix_asn1_tab[] to crypto-tls-x509-helpers.c Daniel P. Berrangé
@ 2024-07-24 9:46 ` Daniel P. Berrangé
2024-07-24 9:46 ` [PULL 04/11] docs/devel: Add introduction to LUKS volume with detached header Daniel P. Berrangé
` (8 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PULL 04/11] docs/devel: Add introduction to LUKS volume with detached header
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
` (2 preceding siblings ...)
2024-07-24 9:46 ` [PULL 03/11] crypto: Allow building with GnuTLS but without Libtasn1 Daniel P. Berrangé
@ 2024-07-24 9:46 ` Daniel P. Berrangé
2024-07-24 9:47 ` [PULL 05/11] meson.build: fix libgcrypt detection on system without libgcrypt-config Daniel P. Berrangé
` (7 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:46 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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 +
| 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
--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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PULL 05/11] meson.build: fix libgcrypt detection on system without libgcrypt-config
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
` (3 preceding siblings ...)
2024-07-24 9:46 ` [PULL 04/11] docs/devel: Add introduction to LUKS volume with detached header Daniel P. Berrangé
@ 2024-07-24 9:47 ` Daniel P. Berrangé
2024-07-24 9:47 ` [PULL 06/11] qapi: drop unused QCryptoBlockCreateOptionsLUKS.detached-header Daniel P. Berrangé
` (6 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:47 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang, Yao Zi
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PULL 06/11] qapi: drop unused QCryptoBlockCreateOptionsLUKS.detached-header
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
` (4 preceding siblings ...)
2024-07-24 9:47 ` [PULL 05/11] meson.build: fix libgcrypt detection on system without libgcrypt-config Daniel P. Berrangé
@ 2024-07-24 9:47 ` Daniel P. Berrangé
2024-07-24 9:47 ` [PULL 07/11] meson: build chardev trace files when have_block Daniel P. Berrangé
` (5 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:47 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PULL 07/11] meson: build chardev trace files when have_block
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
` (5 preceding siblings ...)
2024-07-24 9:47 ` [PULL 06/11] qapi: drop unused QCryptoBlockCreateOptionsLUKS.detached-header Daniel P. Berrangé
@ 2024-07-24 9:47 ` Daniel P. Berrangé
2024-07-24 9:47 ` [PULL 08/11] chardev: add tracing of socket error conditions Daniel P. Berrangé
` (4 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:47 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PULL 08/11] chardev: add tracing of socket error conditions
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
` (6 preceding siblings ...)
2024-07-24 9:47 ` [PULL 07/11] meson: build chardev trace files when have_block Daniel P. Berrangé
@ 2024-07-24 9:47 ` Daniel P. Berrangé
2024-07-24 9:47 ` [PULL 09/11] crypto: drop gnutls debug logging support Daniel P. Berrangé
` (3 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:47 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PULL 09/11] crypto: drop gnutls debug logging support
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
` (7 preceding siblings ...)
2024-07-24 9:47 ` [PULL 08/11] chardev: add tracing of socket error conditions Daniel P. Berrangé
@ 2024-07-24 9:47 ` Daniel P. Berrangé
2024-07-24 9:47 ` [PULL 10/11] crypto: push error reporting into TLS session I/O APIs Daniel P. Berrangé
` (2 subsequent siblings)
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:47 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PULL 10/11] crypto: push error reporting into TLS session I/O APIs
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
` (8 preceding siblings ...)
2024-07-24 9:47 ` [PULL 09/11] crypto: drop gnutls debug logging support Daniel P. Berrangé
@ 2024-07-24 9:47 ` Daniel P. Berrangé
2024-08-12 15:38 ` Thomas Huth
2024-07-24 9:47 ` [PULL 11/11] crypto: propagate errors from TLS session I/O callbacks Daniel P. Berrangé
2024-07-24 23:53 ` [PULL 00/11] Crypto patches Richard Henderson
11 siblings, 1 reply; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:47 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs
2024-07-24 9:47 ` [PULL 10/11] crypto: push error reporting into TLS session I/O APIs Daniel P. Berrangé
@ 2024-08-12 15:38 ` Thomas Huth
2024-08-12 15:42 ` Daniel P. Berrangé
0 siblings, 1 reply; 18+ messages in thread
From: Thomas Huth @ 2024-08-12 15:38 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Paolo Bonzini, Laurent Vivier, Markus Armbruster,
Philippe Mathieu-Daudé, Eric Blake, Marc-André Lureau,
Hyman Huang, Qemu-block, Kevin Wolf, Hanna Reitz
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;
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs
2024-08-12 15:38 ` Thomas Huth
@ 2024-08-12 15:42 ` Daniel P. Berrangé
2024-08-27 7:05 ` Markus Armbruster
0 siblings, 1 reply; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-08-12 15:42 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-devel, Paolo Bonzini, Laurent Vivier, Markus Armbruster,
Philippe Mathieu-Daudé, Eric Blake, Marc-André Lureau,
Hyman Huang, Qemu-block, Kevin Wolf, Hanna Reitz
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 :|
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs
2024-08-12 15:42 ` Daniel P. Berrangé
@ 2024-08-27 7:05 ` Markus Armbruster
2024-08-28 8:32 ` Thomas Huth
0 siblings, 1 reply; 18+ messages in thread
From: Markus Armbruster @ 2024-08-27 7:05 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Thomas Huth, qemu-devel, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Eric Blake, Marc-André Lureau,
Hyman Huang, Qemu-block, Kevin Wolf, Hanna Reitz
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?
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs
2024-08-27 7:05 ` Markus Armbruster
@ 2024-08-28 8:32 ` Thomas Huth
2024-08-29 11:03 ` Daniel P. Berrangé
0 siblings, 1 reply; 18+ messages in thread
From: Thomas Huth @ 2024-08-28 8:32 UTC (permalink / raw)
To: Markus Armbruster, Daniel P. Berrangé
Cc: qemu-devel, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Eric Blake, Marc-André Lureau,
Hyman Huang, Qemu-block, Kevin Wolf, Hanna Reitz
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs
2024-08-28 8:32 ` Thomas Huth
@ 2024-08-29 11:03 ` Daniel P. Berrangé
0 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-08-29 11:03 UTC (permalink / raw)
To: Thomas Huth
Cc: Markus Armbruster, qemu-devel, Paolo Bonzini, Laurent Vivier,
Philippe Mathieu-Daudé, Eric Blake, Marc-André Lureau,
Hyman Huang, Qemu-block, Kevin Wolf, Hanna Reitz
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 :|
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PULL 11/11] crypto: propagate errors from TLS session I/O callbacks
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
` (9 preceding siblings ...)
2024-07-24 9:47 ` [PULL 10/11] crypto: push error reporting into TLS session I/O APIs Daniel P. Berrangé
@ 2024-07-24 9:47 ` Daniel P. Berrangé
2024-07-24 23:53 ` [PULL 00/11] Crypto patches Richard Henderson
11 siblings, 0 replies; 18+ messages in thread
From: Daniel P. Berrangé @ 2024-07-24 9:47 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Daniel P. Berrangé, Philippe Mathieu-Daudé, Eric Blake,
Marc-André Lureau, Hyman Huang
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PULL 00/11] Crypto patches
2024-07-24 9:46 [PULL 00/11] Crypto patches Daniel P. Berrangé
` (10 preceding siblings ...)
2024-07-24 9:47 ` [PULL 11/11] crypto: propagate errors from TLS session I/O callbacks Daniel P. Berrangé
@ 2024-07-24 23:53 ` Richard Henderson
11 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2024-07-24 23:53 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Paolo Bonzini, Thomas Huth, Laurent Vivier, Markus Armbruster,
Philippe Mathieu-Daudé, Eric Blake, Marc-André Lureau,
Hyman Huang
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~
^ permalink raw reply [flat|nested] 18+ messages in thread