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

[Thomas Huth <thuth@redhat.com>]

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;