Re: [Qemu-devel] [PATCH v5 7/9] crypto: introduce new module for handling TLS sessions

[Eric Blake]

On 08/26/2015 09:05 AM, Daniel P. Berrange wrote:
> Introduce a QCryptoTLSSession object that will encapsulate
> all the code for setting up and using a client/sever TLS
> session. This isolates the code which depends on the gnutls
> library, avoiding #ifdefs in the rest of the codebase, as
> well as facilitating any possible future port to other TLS
> libraries, if desired. It makes use of the previously
> defined QCryptoTLSCreds object to access credentials to
> use with the session. It also includes further unit tests
> to validate the correctness of the TLS session handshake
> and certificate validation. This is functionally equivalent
> to the current TLS session handling code embedded in the
> VNC server, and will obsolete it.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  crypto/Makefile.objs           |   1 +
>  crypto/tlssession.c            | 583 
> +++++++++++++++++++++++++++++++++++++++++
>  include/crypto/tlssession.h    | 322 +++++++++++++++++++++++
>  tests/.gitignore               |   4 +
>  tests/Makefile                 |   3 +
>  tests/test-crypto-tlssession.c | 534 +++++++++++++++++++++++++++++++++++++
>  6 files changed, 1447 insertions(+)
>  create mode 100644 crypto/tlssession.c
>  create mode 100644 include/crypto/tlssession.h
>  create mode 100644 tests/test-crypto-tlssession.c
> 

> +++ b/crypto/tlssession.c

> +
> +struct _QCryptoTLSSession {

Why the leading underscore before a capital?  This collides with the
namespace reserved to the compiler/library toolchain.


> +
> +void
> +qcrypto_tls_session_free(QCryptoTLSSession *session)

qemu coding style generally puts the return type and function name on
the same line; but if checkpatch.pl isn't complaining, I won't insist.
(I actually like the return type on a separate line, as emacs handles it
nicer)


> +
> +static int
> +qcrypto_tls_session_check_certificate(QCryptoTLSSession *session,
> +                                      Error **errp)
> +{
> +    int ret;

> +
> +    for (i = 0 ; i < nCerts ; i++) {

No space before ';' (twice)

> +        gnutls_x509_crt_t cert;
> +
> +        if (gnutls_x509_crt_init(&cert) < 0) {
> +            return -1;
> +        }
> +
> +        if (gnutls_x509_crt_import(cert, &certs[i], GNUTLS_X509_FMT_DER) < 
> 0) {
> +            gnutls_x509_crt_deinit(cert);
> +            return -1;
> +        }
> +
> +        if (gnutls_x509_crt_get_expiration_time(cert) < now) {
> +            error_setg(errp, "The certificate has expired");
> +            gnutls_x509_crt_deinit(cert);
> +            return -1;

Lots of common cleanup; worth consolidating it into an out: label?

> +        if (i == 0) {
> +            size_t dnameSize = 1024;
> +            session->peername = g_malloc(dnameSize);
> +        requery:
> +            ret = gnutls_x509_crt_get_dn(cert, session->peername, 
> &dnameSize);
> +            if (ret < 0) {
> +                if (ret == GNUTLS_E_SHORT_MEMORY_BUFFER) {
> +                    session->peername = g_realloc(session->peername,
> +                                                   dnameSize);

Indentation is off.

> +++ b/include/crypto/tlssession.h

> + *    sess = qcrypto_tls_session_new(creds,
> + *                                   "vnc.example.com",
> + *                                   NULL,
> + *                                   QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT,
> + *                                   errp);
> + *    if (sess == NULL) {
> + *           return -1;
> + *    }

Indentation is off

> + *
> + *    qcrypto_tls_session_set_callbacks(sess,
> + *                                      mysock_send,
> + *                                      mysock_recv
> + *                                      GINT_TO_POINTER(fd));
> + *
> + *    while (1) {
> + *       if (qcrypto_tls_session_handshake(sess, errp) < 0) {
> + *           qcrypto_tls_session_free(sess);
> + *           return -1;
> + *       }
> + *
> + *       switch(qcrypto_tls_session_get_handshake_status(sess)) {
> + *         case QCRYPTO_TLS_HANDSHAKE_COMPLETE:
> + *            if (qcrypto_tls_session_check_credentials(sess, errp) < )) {

Unusual indentation

> +
> +typedef struct _QCryptoTLSSession QCryptoTLSSession;

Naming the struct without a leading _ should be fine.

> +
> +
> +/**
> + * qcrypto_tls_session_new:

> + * The @aclname parameter (optionally) specifies the name
> + * of an access controll list that will be used to validate

s/controll/control/


> +/**
> + * qcrypto_tls_session_set_callbacks:

> + *
> + * The @readFunc callback will be passed a pointer to fill
> + * with encrypted data received fro mthe remote peer

s/fro mthe/from the/


> +/**
> + * qcrypto_tls_session_read:
> + * @sess: the TLS session object
> + * @buf: to fill with plain text received
> + * @len: the length of @buf
> + *
> + * Receive upto @len bytes of data from the remote peer

s/upto/up to/


> +++ b/tests/test-crypto-tlssession.c

> +
> +/*
> + * This tests validation checking of peer certificates
> + *
> + * This is replicating the checks that are done for an
> + * active TLS session after handshake completes. To
> + * simulate that we create our TLS contexts, skipping
> + * sanity checks. When then get a socketpair, and

s/When/We/

> + * initiate a TLS session across them. Finally do
> + * do actual cert validation tests
> + */
> +static void test_crypto_tls_session(const void *opaque)

> +    /* We'll use this for our fake client-server connection */
> +    g_assert(socketpair(AF_UNIX, SOCK_STREAM, 0, channel) == 0);

Evil to stick side-effects in a g_assert() (not as evil as doing it in
assert(), but still something you should hoist out separately).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature