bug#24075: tls/https support in Guile (through r6rs binary ports?)

[Ludovic Courtès]

Christopher Allan Webber <address@hidden> skribis:

> Ludovic Courtès writes:
>
>>> +(define (ensure-gnutls)
>>> +  (if (not (force gnutls-module))
>>> +      (throw 'gnutls-not-available "(gnutls) module not available")))
>>
>> I wonder if this is the right exception, but I can’t think of anything
>> better (there’s no generic “not supported” exception I think; (throw
>> 'system-error … ENOSYS) would do that but it’s too vague.)
>
> I don't know... it's hard for me to tell when to use what exception
> symbol in Guile!
>
> I prefer specific exceptions when a more general exception

Yes, I agree.  I was just wondering out loud whether there might be
another exception.  Anyway, this one’s fine!

>> What about leaving the ‘ensure-gnutls’ call and then simply use the
>> GnuTLS symbols directly and rely on autoloading, as in (guix build
>> download)?
>>
>> --8<---------------cut here---------------start------------->8---
>> ;; Autoload GnuTLS so that this module can be used even when GnuTLS is
>> ;; not available.  At compile time, this yields "possibly unbound
>> ;; variable" warnings, but these are OK: we know that the variables will
>> ;; be bound if we need them, because (guix download) adds GnuTLS as an
>> ;; input in that case.
>>
>> ;; XXX: Use this hack instead of #:autoload to avoid compilation errors.
>> ;; See <http://bugs.gnu.org/12202>.
>> (module-autoload! (current-module)
>>                   '(gnutls) '(make-session connection-end/client))
>> --8<---------------cut here---------------end--------------->8---
>>
>> That would lead more concise and slightly more efficient code, and I
>> think it would still work as expected in the absence of (gnutls).
>>
>> WDYT?
>
> So there was this converstaion on #guile:
>
>   <civodul> mark_weaver: the autoload hack fails gracelessly when GnuTLS is   
>     
>             missing
>   <civodul> that's fine in the context of Guix, but maybe not in a more 
> general   
>             context
>   <paron_remote> oh :)
>   <paron_remote> civodul: what approach would you suggest then?
>   <mark_weaver> civodul: could we make it more graceful?
>   <civodul> yeah maybe with some explicit module hackery
>   <civodul> an explicit resolve-interface + module-ref
>   <civodul> something like that
>   <mark_weaver> sounds doable
>
> So... that's what lead me to change it.
>
> Admittedly I'm not totally clear what was meant by "the autoload hack
> fails gracelessly", and what would be more graceful.  Would it be
> because it's trying to utilize a symbol that's not bound to anything?
>
> Which leads to the next question: if I did the autoload hack, what would
> (ensure-gnutls) look like?

Sorry for the confusing statements.  :-)

I think ‘ensure-gnutls’ would be exactly as in this patch, and the
autoload hack would be exactly as shown above.

Here it would fail “gracefully” in the sense that ‘ensure-gnutls’ would
catch a potential problem early on and raise the ‘gnutls-not-available’
exception (you’d have to double-check that this is indeed the behavior
we get, but I’m quite confident ;-)).

In (guix build download) there’s no such protection so when (gnutls) is
missing, users get an unbound variable right in the middle of the code.

>>> +      (define (read! bv start count)
>>> +        (define read-bv (get-bytevector-n record count))
>>> +        (define read-bv-len (bytevector-length read-bv))
>>> +        (bytevector-copy! read-bv 0 bv 0 read-bv-len)
>>> +        read-bv-len)
>>
>> Beware: ‘get-bytevector-n’ can return the EOF object instead of a
>> number, so you need to check for that.  (Conversely, ‘read!’ needs to
>> return 0 to indicate EOF.)
>
> So that would look like this?
>
>       (define (read! bv start count)
>         (define read-bv (get-bytevector-n record count))
>         (if (eof-object? read-bv)
>             0
>             (let ((read-bv-len (bytevector-length read-bv)))
>               (bytevector-copy! read-bv 0 bv 0 read-bv-len)
>               read-bv-len)))

Exactly.

>>> +  (define (open-socket)
>>> +    (let loop ((addresses addresses))
>>
>> Or just “(define sock …”.
>
> Hm, is that a good idea?  Does this need to happen before or within the
> with-https-proxy?

Oh you’re right, sorry.

> From 91c0a4a728ca4bf2e9468cdc849c350dd3f7380f Mon Sep 17 00:00:00 2001
> From: Christopher Allan Webber <address@hidden>
> Date: Thu, 17 Sep 2015 15:14:54 -0500
> Subject: [PATCH] web: Add https support through gnutls.
>
> Since importing gnutls directly would result in a dependency cycle,
> we load gnutls lazily.
>
> This uses code originally written for Guix by Ludovic
>
> * module/web/client.scm: (%http-receive-buffer-size)
>   (warn-no-gnutls-return-false, gnutls-module, ensure-gnutls)
>   (gnutls-ref, tls-wrap): New variables.
>   (open-socket-for-uri): Wrap in tls when uri scheme is https.
> * doc/ref/web.texi (open-socket-for-uri): Document gnutls usage.

[...]

>  @deffn {Scheme Procedure} open-socket-for-uri uri
> -Return an open input/output port for a connection to URI.
> +Return an open input/output port for a connection to URI.  Guile
> +dynamically loads gnutls for https support; for more information, see
> address@hidden Preparations,
> +how to install the GnuTLS bindings for Guile,, gnutls-guile,
> +GnuTLS-Guile}.

@xref generates a “See” for the beginning of a sentence, so it should
be:

  … support.  @xref{…}, for more information.

Also, “HTTPS” and “GnuTLS”.  :-)

The rest is all good for me, so the only remaining bits are the autoload
thing and maybe the bug you observed?

Thanks!

Ludo’.