From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37593) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZVJUR-0002PX-VO for qemu-devel@nongnu.org; Fri, 28 Aug 2015 09:14:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZVJUO-0001hu-7v for qemu-devel@nongnu.org; Fri, 28 Aug 2015 09:14:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40976) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZVJUO-0001hc-0K for qemu-devel@nongnu.org; Fri, 28 Aug 2015 09:14:40 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 80FD6344EAF for ; Fri, 28 Aug 2015 13:14:39 +0000 (UTC) Date: Fri, 28 Aug 2015 14:14:35 +0100 From: "Daniel P. Berrange" Message-ID: <20150828131435.GP28526@redhat.com> References: <1440601524-30316-1-git-send-email-berrange@redhat.com> <1440601524-30316-8-git-send-email-berrange@redhat.com> <55DF1FC7.3000506@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <55DF1FC7.3000506@redhat.com> Subject: Re: [Qemu-devel] [PATCH v5 7/9] crypto: introduce new module for handling TLS sessions Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Paolo Bonzini , qemu-devel@nongnu.org, Gerd Hoffmann On Thu, Aug 27, 2015 at 08:33:43AM -0600, Eric Blake wrote: > 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 > > --- > > 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. Just left over from my conversion from libvirt code, so will remove that. > > + > > +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) I ended up putting the return type on a separate line because in several places it helped keep under the 80 character limit. > > +++ 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 Hehe, reviewing code examples in the comments is nice :-) > > + /* 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). yep, will separate. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|