diff mbox series

[RFC,1/4] crypto: Allow gracefully ending the TLS session

Message ID 20250206175824.22664-2-farosas@suse.de (mailing list archive)
State New
Headers show
Series crypto,io,migration: Add support to gnutls_bye() | expand

Commit Message

Fabiano Rosas Feb. 6, 2025, 5:58 p.m. UTC 
QEMU's TLS session code provides no way to call gnutls_bye() to
terminate a TLS session. Callers of qcrypto_tls_session_read() can
choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the
gracefulTermination argument.

The QIOChannelTLS ignores the premature termination error whenever
shutdown() has already been issued. This is not enough anymore for the
migration code due to changes [1] in the synchronization between
migration source and destination.

Add support for calling gnutls_bye() in the tlssession layer so users
of QIOChannelTLS can clearly identify the end of a TLS session.

1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete")

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 crypto/tlssession.c         | 40 +++++++++++++++++++++++++++++++++++++
 include/crypto/tlssession.h | 22 ++++++++++++++++++++
 2 files changed, 62 insertions(+)

Comments

Daniel P. Berrangé Feb. 6, 2025, 6:15 p.m. UTC | #1 
On Thu, Feb 06, 2025 at 02:58:21PM -0300, Fabiano Rosas wrote:
> QEMU's TLS session code provides no way to call gnutls_bye() to
> terminate a TLS session. Callers of qcrypto_tls_session_read() can
> choose to ignore a GNUTLS_E_PREMATURE_TERMINATION error by setting the
> gracefulTermination argument.
> 
> The QIOChannelTLS ignores the premature termination error whenever
> shutdown() has already been issued. This is not enough anymore for the
> migration code due to changes [1] in the synchronization between
> migration source and destination.
> 
> Add support for calling gnutls_bye() in the tlssession layer so users
> of QIOChannelTLS can clearly identify the end of a TLS session.
> 
> 1- 1d457daf86 ("migration/multifd: Further remove the SYNC on complete")
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  crypto/tlssession.c         | 40 +++++++++++++++++++++++++++++++++++++
>  include/crypto/tlssession.h | 22 ++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> index 77286e23f4..d52714a3f3 100644
> --- a/crypto/tlssession.c
> +++ b/crypto/tlssession.c
> @@ -585,6 +585,40 @@ qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *session)
>      }
>  }
>  
> +int
> +qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
> +{
> +    int ret;
> +
> +    if (!session->handshakeComplete) {
> +        return 0;
> +    }
> +
> +    ret = gnutls_bye(session->handle, GNUTLS_SHUT_WR);
> +
> +    if (!ret) {
> +        return QCRYPTO_TLS_BYE_COMPLETE;
> +    }
> +
> +    if (ret == GNUTLS_E_INTERRUPTED || ret == GNUTLS_E_AGAIN) {
> +        int direction = gnutls_record_get_direction(session->handle);
> +        return direction ? QCRYPTO_TLS_BYE_SENDING : QCRYPTO_TLS_BYE_RECVING;
> +    }
> +
> +    if (session->rerr || session->werr) {
> +        error_setg(errp, "TLS termination failed: %s: %s", gnutls_strerror(ret),
> +                   error_get_pretty(session->rerr ?
> +                                    session->rerr : session->werr));
> +    } else {
> +        error_setg(errp, "TLS termination failed: %s", gnutls_strerror(ret));
> +    }
> +
> +    error_free(session->rerr);
> +    error_free(session->werr);
> +    session->rerr = session->werr = NULL;
> +
> +    return -1;
> +}
>  
>  int
>  qcrypto_tls_session_get_key_size(QCryptoTLSSession *session,
> @@ -699,6 +733,12 @@ qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess)
>  }
>  
>  
> +int
> +qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
> +{
> +    return QCRYPTO_TLS_BYE_COMPLETE;
> +}
> +
>  int
>  qcrypto_tls_session_get_key_size(QCryptoTLSSession *sess,
>                                   Error **errp)
> diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
> index f694a5c3c5..c0f64ce989 100644
> --- a/include/crypto/tlssession.h
> +++ b/include/crypto/tlssession.h
> @@ -323,6 +323,28 @@ typedef enum {
>  QCryptoTLSSessionHandshakeStatus
>  qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess);
>  
> +typedef enum {
> +    QCRYPTO_TLS_BYE_COMPLETE,
> +    QCRYPTO_TLS_BYE_SENDING,
> +    QCRYPTO_TLS_BYE_RECVING,
> +} QCryptoTLSSessionByeStatus;
> +
> +/**
> + * qcrypto_tls_session_bye:
> + * @session: the TLS session object
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Start, or continue, a TLS termination sequence. If the underlying
> + * data channel is non-blocking, then this method may return control
> + * before the termination is complete. The return value will indicate
> + * whether the termination has completed, or is waiting to send or
> + * receive data. In the latter cases, the caller should setup an event
> + * loop watch and call this method again once the underlying data
> + * channel is ready to read or write again.
> + */
> +int
> +qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp);

For handshake I have separate methods

   qcrypto_tls_session_handshake
   qcrypto_tls_session_get_handshake_status

Essentially because I was missing gnutls which has the separate
method to get the record direction.

I would prefer if the handshake and bye code used the same design
pattern.

In retrospect the 2 separate methods for handshake appears redundant
as all uses of them invoke both from the same piece of code. So I'm
fine if you re-factor the existing handshake method to return the
status immediate and get rid of qcrypto_tls_session_get_handshake_status
to match you 'bye' design here.

With regards,
Daniel
diff mbox series

Patch

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 77286e23f4..d52714a3f3 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -585,6 +585,40 @@  qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *session)
     }
 }
 
+int
+qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
+{
+    int ret;
+
+    if (!session->handshakeComplete) {
+        return 0;
+    }
+
+    ret = gnutls_bye(session->handle, GNUTLS_SHUT_WR);
+
+    if (!ret) {
+        return QCRYPTO_TLS_BYE_COMPLETE;
+    }
+
+    if (ret == GNUTLS_E_INTERRUPTED || ret == GNUTLS_E_AGAIN) {
+        int direction = gnutls_record_get_direction(session->handle);
+        return direction ? QCRYPTO_TLS_BYE_SENDING : QCRYPTO_TLS_BYE_RECVING;
+    }
+
+    if (session->rerr || session->werr) {
+        error_setg(errp, "TLS termination failed: %s: %s", gnutls_strerror(ret),
+                   error_get_pretty(session->rerr ?
+                                    session->rerr : session->werr));
+    } else {
+        error_setg(errp, "TLS termination failed: %s", gnutls_strerror(ret));
+    }
+
+    error_free(session->rerr);
+    error_free(session->werr);
+    session->rerr = session->werr = NULL;
+
+    return -1;
+}
 
 int
 qcrypto_tls_session_get_key_size(QCryptoTLSSession *session,
@@ -699,6 +733,12 @@  qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess)
 }
 
 
+int
+qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
+{
+    return QCRYPTO_TLS_BYE_COMPLETE;
+}
+
 int
 qcrypto_tls_session_get_key_size(QCryptoTLSSession *sess,
                                  Error **errp)
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index f694a5c3c5..c0f64ce989 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -323,6 +323,28 @@  typedef enum {
 QCryptoTLSSessionHandshakeStatus
 qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *sess);
 
+typedef enum {
+    QCRYPTO_TLS_BYE_COMPLETE,
+    QCRYPTO_TLS_BYE_SENDING,
+    QCRYPTO_TLS_BYE_RECVING,
+} QCryptoTLSSessionByeStatus;
+
+/**
+ * qcrypto_tls_session_bye:
+ * @session: the TLS session object
+ * @errp: pointer to a NULL-initialized error object
+ *
+ * Start, or continue, a TLS termination sequence. If the underlying
+ * data channel is non-blocking, then this method may return control
+ * before the termination is complete. The return value will indicate
+ * whether the termination has completed, or is waiting to send or
+ * receive data. In the latter cases, the caller should setup an event
+ * loop watch and call this method again once the underlying data
+ * channel is ready to read or write again.
+ */
+int
+qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp);
+
 /**
  * qcrypto_tls_session_get_key_size:
  * @sess: the TLS session object