qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Thomas Huth
Subject: Re: [PULL 10/11] crypto: push error reporting into TLS session I/O APIs
Date: Mon, 12 Aug 2024 17:38:41 +0200
User-agent: Mozilla Thunderbird

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;




reply via email to

[Prev in Thread] Current Thread [Next in Thread]