Re: Emacs core TLS support

[Stefan Monnier <monnier@iro.umontreal.ca>]

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 <gnutls/gnutls.h>
> +
> +#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