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

["Daniel P. Berrangé" <berrange@redhat.com>]

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