From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel,gmane.comp.encryption.gpg.gnutls.devel Subject: Re: Emacs core TLS support Date: Mon, 06 Sep 2010 00:47:39 +0200 Message-ID: References: <878wc1vfh3.fsf@lifelogs.com> <87r5ptpnz2.fsf@stupidchicken.com> <871vhsvkut.fsf@lifelogs.com> <87d41csktn.fsf@lifelogs.com> <87k4v0n0m8.fsf@lifelogs.com> <87wrrvfnc4.fsf@lifelogs.com> <87r5i2d00q.fsf@lifelogs.com> <87zkwqijye.fsf@stupidchicken.com> <878w4actmg.fsf@lifelogs.com> <877hju123h.fsf@stupidchicken.com> <8762yklrdk.fsf@lifelogs.com> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: dough.gmane.org 1283727018 10720 80.91.229.12 (5 Sep 2010 22:50:18 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Sun, 5 Sep 2010 22:50:18 +0000 (UTC) Cc: gnutls-devel@gnu.org, emacs-devel@gnu.org To: Ted Zlatanov Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Sep 06 00:50:15 2010 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([199.232.76.165]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1OsO2O-0006W8-0l for ged-emacs-devel@m.gmane.org; Mon, 06 Sep 2010 00:50:12 +0200 Original-Received: from localhost ([127.0.0.1]:55917 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OsO2L-0002Kg-Bs for ged-emacs-devel@m.gmane.org; Sun, 05 Sep 2010 18:50:09 -0400 Original-Received: from [140.186.70.92] (port=40294 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OsO01-0001Ww-Nt for emacs-devel@gnu.org; Sun, 05 Sep 2010 18:47:46 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OsNzz-00025X-3n for emacs-devel@gnu.org; Sun, 05 Sep 2010 18:47:45 -0400 Original-Received: from impaqm1.telefonica.net ([213.4.138.1]:9141) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OsNzy-00025N-QR for emacs-devel@gnu.org; Sun, 05 Sep 2010 18:47:43 -0400 Original-Received: from IMPmailhost6.adm.correo ([10.20.102.127]) by IMPaqm1.telefonica.net with bizsmtp id 3ALa1f0192kvMAa01AnhoX; Mon, 06 Sep 2010 00:47:41 +0200 Original-Received: from ceviche.home ([83.61.51.178]) by IMPmailhost6.adm.correo with BIZ IMP id 3Anf1f0043qhCuj1mAngBy; Mon, 06 Sep 2010 00:47:40 +0200 X-Brightmail-Tracker: AAAAAA== X-TE-authinfo: authemail="monnier$movistar.es" |auth_email="monnier@movistar.es" X-TE-AcuTerraCos: auth_cuTerraCos="cosuitnetc01" Original-Received: by ceviche.home (Postfix, from userid 20848) id 8805B66115; Mon, 6 Sep 2010 00:47:39 +0200 (CEST) In-Reply-To: <8762yklrdk.fsf@lifelogs.com> (Ted Zlatanov's message of "Sat, 04 Sep 2010 23:57:11 -0500") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:129698 gmane.comp.encryption.gpg.gnutls.devel:4476 Archived-At: Overall, it looks good. Some comments on your code. > @@ -3682,6 +3690,7 @@ > echo " Does Emacs use -ldbus? ${HAVE_DBUS}" > echo " Does Emacs use -lgconf? ${HAVE_GCONF}" > echo " Does Emacs use -lselinux? ${HAVE_LIBSELINUX}" > +echo " Does Emacs use Gnu TLS? ${HAVE_GNUTLS}" For symmetry, I'd say "Does Emacs use -lgnutls?". > --- lisp/net/gnutls.el 1970-01-01 00:00:00 +0000 > +++ lisp/net/gnutls.el 2010-09-05 04:42:32 +0000 > @@ -0,0 +1,120 @@ > +;; By Simon Josefsson 2001-12-01 > +;; See http://josefsson.org/emacs-security/ Use C-u M-x checkdoc-current-buffer which will help you follow the usual coding conventions (e.g. inserting the GPL blurb). > +(defvar starttls-host nil) What is this for? It seems to only ever be set and never read. Making it global doesn't make sense, and making it buffer-local only makes sense if you presume there's never going to be more than a single TLS process per buffer, which can't guarantee. For that reason, any needed aux data should be kept in process properties, I think. > + (set (make-variable-buffer-local 'starttls-host) host))) Hpefully the byte-compiler flags this which should use `make-local-variable' instead (or move the (make-variable-buffer-local 'starttls-host) to the toplevel right after the defvar). Tho, as mentioned above, probably the real solution doesn't use such a variable. > +DEFUN ("gnutls-init", Fgnutls_init, Sgnutls_init, 2, 2, 0, > + doc: /* Initializes GNU TLS for process PROC for use as CONNECTION-END. > +CONNECTION-END is used to indicate if this process is as a server or > +client. Can be one of `gnutls-client' and `gnutls-server'. Currently > +only `gnutls-client' is supported. This formulation means that the symbols (rather than the value of the corresponding variables) `gnutls-client' and `gnutls-server' are the valid values. > +Processes must be initialized with this function before other GNU TLS > +functions are used. This function allocates resources which can only > +be deallocated by calling `gnutls-deinit'. Returns zero on success. */) > + (Lisp_Object proc, Lisp_Object connection_end) > +{ > + int ret; > + > + CHECK_PROCESS (proc); > + > + ret = gnutls_init((gnutls_session_t*)&(XPROCESS(proc)->gnutls_state), > + connection_end); I recommend you compile your Emacs with -DUSE_LISP_UNION_TYPE which will catch errors such as the one above: clearly gnutls_init doesn't take a Lisp_Object as second argument. You probably meant to add an XINT (...), and you'll want to add a CHECK_NUMBER for it beforehand as well. This said, while I understand the general desire to just bring the C API of GNU TLS into Elisp, as long as you do it by hand, you might as well use here a Lisp boolean for connection_end. > + return XINT(ret); -DUSE_LISP_UNION_TYPE will also catch this error. > + state = (gnutls_session_t) XPROCESS(proc)->gnutls_state; > + gnutls_deinit(state); Please always put a space before the open paren of a macro or function call. Applies to the rest of the code as well, of course. > + int ret; > + ret = gnutls_global_init(); Uninitialized variables are dangerous, so it's a good habit to initialize vars when you declare them, especially when it's trivial to do so. It's also more concise: int ret = gnutls_global_init(); > + XSETINT (lret, ret); > + return lret; > +} return make_number (lret); will save you the uninitialized lret as well. > +DEFUN ("gnutls-cert-set-x509-trust-file", > + Fgnutls_cert_set_x509_trust_file, > + Sgnutls_cert_set_x509_trust_file, 2, 2, 0, > + doc: /* Set X.509 client trust file for PROCESS > +CERTFILE is a PEM encoded file. Returns zero on success. > +*/) By convention we keep the closing */) at the end of the previous line. > + (Lisp_Object proc, Lisp_Object certfile) > +{ > + gnutls_session_t state; > + gnutls_certificate_credentials_t x509_cred; > + Lisp_Object lret; > + int ret; > + > + CHECK_STRING(certfile); > + > + CHECK_PROCESS (proc); > + state = (gnutls_session_t) XPROCESS(proc)->gnutls_state; > + > + x509_cred = (gnutls_certificate_credentials_t) XPROCESS(proc)->x509_cred; > + > + ret = gnutls_certificate_set_x509_trust_file (x509_cred, XSTRING (certfile)->data, GNUTLS_X509_FMT_PEM); > + > + XSETINT (lret, ret); > + return lret; > +} > + > +DEFUN ("gnutls-cred-set", Fgnutls_cred_set, > + Sgnutls_cred_set, 2, 2, 0, > + doc: /* Enables GNU TLS authentication for PROCESS. > +TYPE is an integer indicating the type of the credentials, either > +`gnutls-anon', `gnutls-srp' or `gnutls-x509pki'. Again, the above formulation means that the caller should pass those symbols rather than value associated with the corresponding variables. > + switch (XINT (type)) Here, you extract the integer value without having checked that `type' is indeed an integer. > + { > + case GNUTLS_CRD_CERTIFICATE: > + if (gnutls_certificate_allocate_credentials (&x509_cred) < 0) > + memory_full (); Can it really only mean "memory is full"? > === added file 'src/gnutls.h' > --- src/gnutls.h 1970-01-01 00:00:00 +0000 > +++ src/gnutls.h 2010-09-05 04:42:32 +0000 > @@ -0,0 +1,4 @@ > +#ifdef HAVE_GNUTLS > +#include > + > +#endif Why add this file? Doesn't seem worth the trouble. > +#ifdef HAVE_GNUTLS > +/* Defined in gnutls.c */ > +extern void syms_of_gnutls (void); > +#endif If you have a src/gnutls.h, then the above should be moved to there. > +#ifdef HAVE_GNUTLS > + /* XXX Store GNU TLS state and auth mechanisms in Lisp_Objects. */ > + Lisp_Object gnutls_state; > + Lisp_Object x509_cred, x509_callback; > + Lisp_Object anon_cred; > + Lisp_Object srp_cred; > +#endif Rather than hardcode variables in gnutls.el, an alternative could be to define those variables in gnutls.c so you can initialize them to the values taken from gnutls/gnutls.h. Stefan