From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37523) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUyFV-0003jy-A0 for qemu-devel@nongnu.org; Thu, 27 Aug 2015 10:33:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUyFR-0000k9-11 for qemu-devel@nongnu.org; Thu, 27 Aug 2015 10:33:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39713) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUyFQ-0000k5-Ny for qemu-devel@nongnu.org; Thu, 27 Aug 2015 10:33:48 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 794EB8CF5B for ; Thu, 27 Aug 2015 14:33:48 +0000 (UTC) References: <1440601524-30316-1-git-send-email-berrange@redhat.com> <1440601524-30316-8-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <55DF1FC7.3000506@redhat.com> Date: Thu, 27 Aug 2015 08:33:43 -0600 MIME-Version: 1.0 In-Reply-To: <1440601524-30316-8-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="QWgFTX9FQTluqw9D4tVatnIbsToW8Vv7w" Subject: Re: [Qemu-devel] [PATCH v5 7/9] crypto: introduce new module for handling TLS sessions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Paolo Bonzini , Gerd Hoffmann This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --QWgFTX9FQTluqw9D4tVatnIbsToW8Vv7w Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > Signed-off-by: Daniel P. Berrange > --- > 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 >=20 > +++ 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 =3D 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_DE= R) < 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 =3D=3D 0) { > + size_t dnameSize =3D 1024; > + session->peername =3D g_malloc(dnameSize); > + requery: > + ret =3D gnutls_x509_crt_get_dn(cert, session->peername, &d= nameSize); > + if (ret < 0) { > + if (ret =3D=3D GNUTLS_E_SHORT_MEMORY_BUFFER) { > + session->peername =3D g_realloc(session->peername,= > + dnameSize); Indentation is off. > +++ b/include/crypto/tlssession.h > + * sess =3D qcrypto_tls_session_new(creds, > + * "vnc.example.com", > + * NULL, > + * QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT= , > + * errp); > + * if (sess =3D=3D 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) =3D=3D 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). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --QWgFTX9FQTluqw9D4tVatnIbsToW8Vv7w Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV3x/HAAoJEKeha0olJ0Nqn6cIAKnkCKRV9IxXVCjM8WYjgzoQ rSIJqeuiIUSYkoTwEBqpdk2v424PhgZQlMOeeWHffT/nGDdyNChyWxgopQOVZuYk 9cxCaMS+P6d52cAhvzXLFM/xOaqXPAYsSAevDHTo8vHkYpJzYwEneiYPnZ1YbAu4 LZlnTZnEsspw/akVWQ+AvwIF0LAmQ1EY/mPyYOJmQfm2QiTk8fjeuoI19x1dXiiX 6fnInhJVsh+1OdjGvwDOHHckXxwg7oqa9c0hw9SnE7PdVdcAy6KB6S3rvXReLYM7 rGfw6GRdraow7iQCYFzBxaD+lQ2/AJwq0WUQhHe2YsDOzIyvdmuqoSRa+jUQZsE= =nkGJ -----END PGP SIGNATURE----- --QWgFTX9FQTluqw9D4tVatnIbsToW8Vv7w--