Re: [PATCH 1/3] crypto: Add qcrypto_tls_shutdown()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Gnutls documents that applications that want to distinguish between a
> clean end-of-communication and a malicious client abruptly tearing the
> underlying transport out of under our feet need to use gnutls_bye().
> Our channel code is already set up to allow shutdown requests, but we
> weren't forwarding those to gnutls.  To make that work, we first need
> a new entry point that can isolate the rest of our code from the
> gnutls interface.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  qapi/crypto.json            | 15 +++++++++++++++
>  include/crypto/tlssession.h | 24 ++++++++++++++++++++++++
>  crypto/tlssession.c         | 27 +++++++++++++++++++++++++++
>  3 files changed, 66 insertions(+)
>
> diff --git a/qapi/crypto.json b/qapi/crypto.json
> index b2a4cff683ff..1df0f4502885 100644
> --- a/qapi/crypto.json
> +++ b/qapi/crypto.json
> @@ -119,6 +119,21 @@
>    'prefix': 'QCRYPTO_IVGEN_ALG',
>    'data': ['plain', 'plain64', 'essiv']}
>
> +##
> +# @QCryptoShutdownMode:
> +#
> +# The supported modes for requesting shutdown of a crypto
> +# communication channel.
> +#
> +# @shut-wr: No more writes will be sent, but the remote end can still send
> +#           data to be read.
> +# @shut-rdwr: No more reads or writes should occur.
> +# Since: 5.1
> +##
> +{ 'enum': 'QCryptoShutdownMode',
> +  'prefix': 'QCRYPTO',
> +  'data': ['shut-wr', 'shut-rdwr']}
> +
>  ##
>  # @QCryptoBlockFormat:
>  #
> diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
> index 15b9cef086cc..10c670e3b6a2 100644
> --- a/include/crypto/tlssession.h
> +++ b/include/crypto/tlssession.h
> @@ -321,4 +321,28 @@ int qcrypto_tls_session_get_key_size(QCryptoTLSSession 
> *sess,
>   */
>  char *qcrypto_tls_session_get_peer_name(QCryptoTLSSession *sess);
>
> +/**
> + * qcrypto_tls_shutdown:
> + * @sess: the TLS session object
> + * @how: the desired shutdown mode
> + *
> + * Prepare to terminate the session.  If @how is QCRYPTO_SHUT_WR, this
> + * side will no longer write data, but should still process reads
> + * until EOF; if @how is QCRYPTO_SHUT_RDWR, then the entire session
> + * should shut down.  Use of this function is optional, since closing
> + * the session implies QCRYPTO_SHUT_RDWR.  However, using this
> + * function prior to terminating the underlying transport layer makes
> + * it possible for the remote endpoint to distinguish between a
> + * malicious party prematurely terminating the the connection and
> + * normal termination.
> + *
> + * This function should only be used after a successful
> + * qcrypto_tls_session_handshake().
> + *
> + * Returns: 0 for success, or -EAGAIN if more underlying I/O is
> + * required to finish proper session shutdown.
> + */
> +int qcrypto_tls_session_shutdown(QCryptoTLSSession *sess,
> +                                 QCryptoShutdownMode how);
> +
>  #endif /* QCRYPTO_TLSSESSION_H */
> diff --git a/crypto/tlssession.c b/crypto/tlssession.c
> index 33203e8ca711..903301189069 100644
> --- a/crypto/tlssession.c
> +++ b/crypto/tlssession.c
> @@ -521,6 +521,33 @@ 
> qcrypto_tls_session_get_handshake_status(QCryptoTLSSession *session)
>  }
>
>
> +int qcrypto_tls_session_shutdown(QCryptoTLSSession *session,
> +                                 QCryptoShutdownMode how)
> +{
> +    gnutls_close_request_t mode;
> +    int ret;
> +
> +    assert(session->handshakeComplete);

Suggest a blank line here.

> +    switch (how) {
> +    case QCRYPTO_SHUT_WR:
> +        mode = GNUTLS_SHUT_WR;
> +        break;
> +    case QCRYPTO_SHUT_RDWR:
> +        mode = GNUTLS_SHUT_RDWR;
> +        break;
> +    default:
> +        abort();
> +    }
> +
> +    ret = gnutls_bye(session->handle, mode);
> +    if (ret == GNUTLS_E_INTERRUPTED ||
> +        ret == GNUTLS_E_AGAIN) {
> +        return -EAGAIN;
> +    }
> +    return 0;
> +}
> +
> +
>  int
>  qcrypto_tls_session_get_key_size(QCryptoTLSSession *session,
>                                   Error **errp)

This is a thin wrapper around gnutls_bye().  I understand this is an
abstraction layer backed by GnuTLS.  Not sure abstracting from just one
concrete thing is a good idea, but that's way out of scope here.

In scope: why do you need QCryptoShutdownMode be a QAPI type?