Re: [PATCH] GnuTLS support on Woe32
Ted Zlatanov
>> + hostname = SSDATA (Fsymbol_value (intern_c_string ("gnutls-hostname")));
SM> C is not Lisp, it does not perform dynamic type checks for you, you have
SM> to do them by hand: the above code will lead to crashes if someone sets
SM> gnutls-hostname to something else than a string, so you need to
SM> CHECK_STRING or something like that.
Added CHECK_STRING.
SM> Also further down you define Qgnutls_hostname but never use it, but here
SM> would be a good place to use it (otherwise, don't define it).
SM> Finally, if you want to avoid Fsymbol_value, you can use DEFVAR_LISP to
SM> define Vgnutls_hostname so you can then just do SSDATA (Vgnutls_hostname).
Fixed. I wanted to define that variable in gnutls.el so I can make it
buffer-local there too (right before it's used). If you think that's
better in gnutls.c, I'll change it.
SM> You do not need the braces if there's only one instruction in the block.
Fixed, except in these two places:
if (peer_verification != 0)
{
if (NILP (verify_hostname_error))
{
message ("Certificate validation failed for %s, verification code %d",
hostname, peer_verification);
}
else
{
error ("Certificate validation failed for %s, verification code %d",
hostname, peer_verification);
}
}
...
if (!gnutls_x509_crt_check_hostname (gnutls_verify_cert, hostname))
{
if (NILP (verify_hostname_error))
{
message ("GnuTLS warning: the certificate's hostname does not match gnutls-hostname \"%s\"", hostname);
}
else
{
gnutls_x509_crt_deinit (gnutls_verify_cert);
error ("The certificate's hostname does not match gnutls-hostname \"%s\"", hostname);
}
}
where I thought removing the braces looked confusing and ugly because of
the nesting.
>> +#ifdef HAVE_GNUTLS
>> + /* GnuTLS buffers data internally. In lowat mode it leaves some data
SM> Shouldn't that be "Iowait"?
No, see gnutls_transport_set_lowat() for instance.
SM> Also please put 2 spaces after a ".".
I fixed the comments, I think.
>> + && gnutls_record_check_pending(wait_proc->gnutls_state) > 0)
Fixed.
>> + sc = select (fd + 1, &fdset, (SELECT_TYPE *)0, (SELECT_TYPE *)0, &timeout);
Fixed.
>> + /* translate WSAEWOULDBLOCK alias
>> + EWOULDBLOCK to EAGAIN for
>> + GnuTLS */
Fixed (and the other such comment in w32.c).
>> +extern ssize_t emacs_gnutls_pull(gnutls_transport_ptr_t p,
>> + void* buf, size_t sz);
>> +extern ssize_t emacs_gnutls_push(gnutls_transport_ptr_t p,
>> + const void* buf, size_t sz);
Fixed, and the call in w32.c too.
I've attached an updated patch. Sorry if I have missed anything. It
would be nice to have an automatic way to catch these formatting issues.
SM> As far as functionality goes, I don't know what this is trying to do nor
SM> why it needs to do it this way, so I can't really judge. The key
SM> validation code seems to be "very" complex, in the sense that we would
SM> probably want to move some of that complexity to Elisp at some point.
Unfortunately the validation is tightly coupled to the C-level GnuTLS
functions so it would require writing a lot of glue code. All the
session data initialization and certificate validation are done with
GnuTLS C functions and the data passed around has to be at the C level.
Breaking up the validation into chunks could help but then more
intermediate results have to be stored in each buffer and the
error-handling logic would get even more complicated.
I am excited that this patch finally achieves the base functionality
Emacs needs to do SSL and TLS connections without helper applications on
most platforms we support. So I hope I can make it acceptable soon :)
Thanks for looking at it.
Ted
Ted Zlatanov
SM> BTW, I had not noticed this part in gnutls.el, which seems like an
SM> error: why would you want it to be buffer-local? Gnutls is about
SM> processes, so binding this var to buffers makes no sense to me.
...
SM> Now that I look at it, I don't understand what this gnutls-hostname
SM> variable is about. Why isn't it an additional keyword argument instead?
SM> It needs better documentation than the current "Remote hostname.".
Because of the way SSL and TLS work, the connection may start out
unencrypted and the upgrade is sort of opportunistic. So we don't know
in advance if we'll need the `gnutls-hostname'. Also the
`gnutls-hostname' is not necessarily the actual host we connect to, so
we can't keep it as a per-process property. And finally, making it a
keyword parameter means the piece that *upgrades* the connection to TLS
has to know the original hostname of the connection. I thought it was
cleaner to separate them, so upgrading a connection is easier to do
opportunistically.
Emacs doesn't have per-process variables at the ELisp level so I had to
associate it with the buffer and making it buffer-local seemed
sensible. How would you do it?
>> where I thought removing the braces looked confusing and ugly because of
>> the nesting.
SM> Fine (I personally prefer this code without the internal braces, but
SM> it's no big deal). I'm not opposed to braces, but in the previous code
SM> there was a lot of them around repetitive and "simple" code which lead
SM> to the code being much too diluted.
Yes, you were absolutely right to note those. Thanks.
SM> We could come up with some font-lock rules to highlight "offending"
SM> code, but I'm not sure it's worth the trouble.
It would make my life easier. And maybe help other contributors.
>> +:verify-flags is a bitset as per gnutls_certificate_set_verify_flags().
SM> In the GNU system we use the convention that "funname()" is a function
SM> call and denotes the result of calling that function, rather than the
SM> function itself. To refer to the function, just say "funname".
Fixed. It's a habit for me :)
>> +:verify-hostname-error determines if a hostname mismatch is a warning
>> +or an error.
SM> Try to use the form "if non-nil blabla", so it's clear which value gives
SM> you which behavior.
Fixed.
>> + (set (intern "gnutls-hostname") host))
SM> Yuck!!
SM> This should say "(setq gnutls-hostname host)": more efficient, more
SM> concise, more understandable (also for the compiler), ...
I was trying to shut up the byte-compilation warnings. proto-stream.el
does some funky loading and I didn't know a better way (there's no
`declare-variable'). If you have a better approach, please tell...
Sorry this patch is getting so large. I'll try to fix all the issues
ASAP. We need Claudio Bley's papers too, right?
Ted