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 <berra...@redhat.com> > --- > 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
