The custom input/output port wrapping the TLS session record port would introduce overhead, and it would also prevent its uses in a non-blocking context--e.g., with Fibers. The port close mechanism added in GnuTLS 3.7.7 allows us to get rid of that wrapper. * guix/build/download.scm (wrap-record-port-for-gnutls<3.7.7): New procedure, with code formerly in 'tls-wrap'. (tls-wrap): Check for 'set-session-record-port-close!' and use it when available; otherwise call 'wrap-record-port-for-gnutls<3.7.7'. --- guix/build/download.scm | 102 +++++++++++++++++++++------------------- 1 file changed, 54 insertions(+), 48 deletions(-) Hello! I'll land a similar change in Guile's (web client) module afterwards if there are no objections. Ludo'. diff --git a/guix/build/download.scm b/guix/build/download.scm index 41583e8143..de094890b3 100644 --- a/guix/build/download.scm +++ b/guix/build/download.scm @@ -245,6 +245,54 @@ (define (print-tls-certificate-error port key args default-printer) (set-exception-printer! 'tls-certificate-error print-tls-certificate-error) +(define (wrap-record-port-for-gnutls<3.7.7 record port) + "Return a port that wraps RECORD to ensure that closing it also closes PORT, +the actual socket port, and its file descriptor. Make sure it does not +introduce extra buffering (custom ports are buffered by default as of Guile +3.0.5). + +This wrapper is unnecessary with GnuTLS >= 3.7.7, which can automatically +close SESSION's file descriptor when RECORD is closed." + (define (read! bv start count) + (define read + (catch 'gnutls-error + (lambda () + (get-bytevector-n! record bv start count)) + (lambda (key err proc . rest) + ;; When responding to "Connection: close" requests, some servers + ;; close the connection abruptly after sending the response body, + ;; without doing a proper TLS connection termination. Treat it as + ;; EOF. This is fixed in GnuTLS 3.7.7. + (if (eq? err error/premature-termination) + the-eof-object + (apply throw key err proc rest))))) + + (if (eof-object? read) + 0 + read)) + (define (write! bv start count) + (put-bytevector record bv start count) + (force-output record) + count) + (define (get-position) + (port-position record)) + (define (set-position! new-position) + (set-port-position! record new-position)) + (define (close) + (unless (port-closed? port) + (close-port port)) + (unless (port-closed? record) + (close-port record))) + + (define (unbuffered port) + (setvbuf port 'none) + port) + + (unbuffered + (make-custom-binary-input/output-port "gnutls wrapped port" read! write! + get-position set-position! + close))) + (define* (tls-wrap port server #:key (verify-certificate? #t)) "Return PORT wrapped in a TLS connection to SERVER. SERVER must be a DNS host name without trailing dot." @@ -317,55 +365,13 @@ (define (log level str) (apply throw args)))) (let ((record (session-record-port session))) - (define (read! bv start count) - (define read - (catch 'gnutls-error - (lambda () - (get-bytevector-n! record bv start count)) - (lambda (key err proc . rest) - ;; When responding to "Connection: close" requests, some - ;; servers close the connection abruptly after sending the - ;; response body, without doing a proper TLS connection - ;; termination. Treat it as EOF. - (if (eq? err error/premature-termination) - the-eof-object - (apply throw key err proc rest))))) - - (if (eof-object? read) - 0 - read)) - (define (write! bv start count) - (put-bytevector record bv start count) - (force-output record) - count) - (define (get-position) - (port-position record)) - (define (set-position! new-position) - (set-port-position! record new-position)) - (define (close) - (unless (port-closed? port) - (close-port port)) - (unless (port-closed? record) - (close-port record))) - - (define (unbuffered port) - (setvbuf port 'none) - port) - (setvbuf record 'block) - - ;; Return a port that wraps RECORD to ensure that closing it also - ;; closes PORT, the actual socket port, and its file descriptor. - ;; Make sure it does not introduce extra buffering (custom ports - ;; are buffered by default as of Guile 3.0.5). - ;; XXX: This wrapper would be unnecessary if GnuTLS could - ;; automatically close SESSION's file descriptor when RECORD is - ;; closed, but that doesn't seem to be possible currently (as of - ;; 3.6.9). - (unbuffered - (make-custom-binary-input/output-port "gnutls wrapped port" read! write! - get-position set-position! - close))))) + (if (module-defined? (resolve-interface '(gnutls)) + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 + (let ((close-wrapped-port (lambda (_) (close-port port)))) + (set-session-record-port-close! record close-wrapped-port) + record) + (wrap-record-port-for-gnutls<3.7.7 record port))))) (define (ensure-uri uri-or-string) ;XXX: copied from (web http) (cond base-commit: ab59155c5a38dda7efaceb47c7528578fcf0def4 -- 2.37.1
[-- Attachment #1: Type: text/plain, Size: 521 bytes --] Ludovic Courtès <ludo@gnu.org> skribis: > The custom input/output port wrapping the TLS session record port would > introduce overhead, and it would also prevent its uses in a non-blocking > context--e.g., with Fibers. The port close mechanism added in GnuTLS > 3.7.7 allows us to get rid of that wrapper. And here’s the GnuTLS 3.7.7 package to test it; you need to make sure to have 3.7.7 on your load path, for instance by running: ./pre-inst-env guix shell -D guix guile gnutls@3.7.7 Ludo’. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Type: text/x-patch, Size: 990 bytes --] diff --git a/gnu/packages/tls.scm b/gnu/packages/tls.scm index 1ee5400a9c..33c93b7a5b 100644 --- a/gnu/packages/tls.scm +++ b/gnu/packages/tls.scm @@ -329,6 +329,21 @@ (define-public gnutls (properties '((ftp-server . "ftp.gnutls.org") (ftp-directory . "/gcrypt/gnutls"))))) +(define-public gnutls-latest + (package + (inherit gnutls) + (version "3.7.7") + (source (origin + (method url-fetch) + (uri (string-append "mirror://gnupg/gnutls/v" + (version-major+minor version) + "/gnutls-" version ".tar.xz")) + (patches (search-patches "gnutls-skip-trust-store-test.patch" + "gnutls-cross.patch")) + (sha256 + (base32 + "01i1gl15k6qwvxmxx0by1mn9nlmcmym18wdpm7dn9awfsp8474dy")))))) + (define-public gnutls/guile-2.0 ;; GnuTLS for Guile 2.0. (package/inherit gnutls
[-- Attachment #1.1.1: Type: text/plain, Size: 2414 bytes --] Some objections on error handling (I don't know much about the wrapping) On 01-08-2022 11:07, Ludovic Courtès wrote: > [...] > Hello! > > I'll land a similar change in Guile's (web client) module afterwards > if there are no objections. > > Ludo'. > > diff --git a/guix/build/download.scm b/guix/build/download.scm > index 41583e8143..de094890b3 100644 > --- a/guix/build/download.scm > +++ b/guix/build/download.scm > @@ -245,6 +245,54 @@ (define (print-tls-certificate-error port key args default-printer) > (set-exception-printer! 'tls-certificate-error > print-tls-certificate-error) > > +(define (wrap-record-port-for-gnutls<3.7.7 record port) > + "Return a port that wraps RECORD to ensure that closing it also closes PORT, > +the actual socket port, and its file descriptor. Make sure it does not > +introduce extra buffering (custom ports are buffered by default as of Guile > +3.0.5). > + > +This wrapper is unnecessary with GnuTLS >= 3.7.7, which can automatically > +close SESSION's file descriptor when RECORD is closed." > + (define (read! bv start count) > + (define read > + (catch 'gnutls-error > + (lambda () > + (get-bytevector-n! record bv start count)) > + (lambda (key err proc . rest) > + ;; When responding to "Connection: close" requests, some servers > + ;; close the connection abruptly after sending the response body, > + ;; without doing a proper TLS connection termination. Treat it as > + ;; EOF. This is fixed in GnuTLS 3.7.7. > + (if (eq? err error/premature-termination) > + the-eof-object > + (apply throw key err proc rest))))) Objection: 'catch' makes the backtrace part happening inside the 'get-bytevector-n!' disappear, because it is unwinding, as has been noted a few times (in different contexts) by Attila Lendvai and me. Maybe use 'guard' with an appropriate condition instead? > + (if (module-defined? (resolve-interface '(gnutls)) > + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 resolve-module (and presumably also sets #:ensure #t by default, which sometimes causes 'module not found' messages to be replaced by 'unbound variable', which I don't think is useful behaviour, can #:ensure be set to #false? Greetings, Maxime [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 929 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 236 bytes --]
Hi, Maxime Devos <maximedevos@telenet.be> skribis: > On 01-08-2022 11:07, Ludovic Courtès wrote: [...] >> + (define (read! bv start count) >> + (define read >> + (catch 'gnutls-error >> + (lambda () >> + (get-bytevector-n! record bv start count)) >> + (lambda (key err proc . rest) >> + ;; When responding to "Connection: close" requests, some servers >> + ;; close the connection abruptly after sending the response body, >> + ;; without doing a proper TLS connection termination. Treat it as >> + ;; EOF. This is fixed in GnuTLS 3.7.7. >> + (if (eq? err error/premature-termination) >> + the-eof-object >> + (apply throw key err proc rest))))) > > Objection: 'catch' makes the backtrace part happening inside the > 'get-bytevector-n!' disappear, because it is unwinding, as has been > noted a few times (in different contexts) by Attila Lendvai and me. > Maybe use 'guard' with an appropriate condition instead? This code was already there and has just been moved around. (It’s also code that will no longer be used going forward.) >> + (if (module-defined? (resolve-interface '(gnutls)) >> + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 > > resolve-module (and presumably also sets #:ensure #t by default, which > sometimes causes 'module not found' messages to be replaced by > 'unbound variable', which I don't think is useful behaviour, can > #:ensure be set to #false? This is unnecessary: see the ‘load-gnutls’ mechanism there. The idiom above is already used in a couple of places. Thanks for your feedback! Ludo’.
Ludovic Courtès <ludo@gnu.org> skribis:
> The custom input/output port wrapping the TLS session record port would
> introduce overhead, and it would also prevent its uses in a non-blocking
> context--e.g., with Fibers. The port close mechanism added in GnuTLS
> 3.7.7 allows us to get rid of that wrapper.
>
> * guix/build/download.scm (wrap-record-port-for-gnutls<3.7.7): New
> procedure, with code formerly in 'tls-wrap'.
> (tls-wrap): Check for 'set-session-record-port-close!' and use it when
> available; otherwise call 'wrap-record-port-for-gnutls<3.7.7'.
> ---
> guix/build/download.scm | 102 +++++++++++++++++++++-------------------
> 1 file changed, 54 insertions(+), 48 deletions(-)
Pushed as Guix commit dd573ceea73295c7a872088ecd91e5f0fd74bf2b.
Ludo’.
[-- Attachment #1: Type: text/plain, Size: 1953 bytes --] Ludovic Courtès <ludo@gnu.org> skribis: > The custom input/output port wrapping the TLS session record port would > introduce overhead, and it would also prevent its uses in a non-blocking > context--e.g., with Fibers. The port close mechanism added in GnuTLS > 3.7.7 allows us to get rid of that wrapper. > > * guix/build/download.scm (wrap-record-port-for-gnutls<3.7.7): New > procedure, with code formerly in 'tls-wrap'. > (tls-wrap): Check for 'set-session-record-port-close!' and use it when > available; otherwise call 'wrap-record-port-for-gnutls<3.7.7'. I synchronized Guile's copy of this code: 317b06bf8 web: 'tls-wrap' retries handshake upon non-fatal errors. c01ca10b3 web: Do not wrap TLS port on GnuTLS >= 3.7.7. I realized that’s not enough to make it possible to use non-blocking ports though. First, I noticed that GnuTLS doesn’t implement ‘write_wait_fd’, only ‘read_wait_fd’ (not sure how problematic that is): --8<---------------cut here---------------start------------->8--- scheme@(guile-user)> ,use(web client) scheme@(guile-user)> (define p (open-socket-for-uri "https://guix.gnu.org")) scheme@(guile-user)> ((@@ (ice-9 suspendable-ports) wait-for-writable) p) ice-9/boot-9.scm:1685:16: In procedure raise-exception: In procedure write_wait_fd: unimplemented Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue. scheme@(guile-user) [1]> ,q scheme@(guile-user)> ,use(gnutls) scheme@(guile-user)> (gnutls-version) $1 = "3.7.7" scheme@(guile-user)> ((@@ (ice-9 suspendable-ports) wait-for-readable) p) $2 = 1 --8<---------------cut here---------------end--------------->8--- Second, ‘open-socket-for-uri’ creates a blocking socket and uses that as the backing file descriptor of the TLS session. We’d need a way to pass flags for the ‘socket’ call made by ‘open-socket-for-uri’ so we can pass O_NONBLOCK, maybe as show below: [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Type: text/x-patch, Size: 1488 bytes --] diff --git a/module/web/client.scm b/module/web/client.scm index a08c4203c..9273a45ad 100644 --- a/module/web/client.scm +++ b/module/web/client.scm @@ -320,7 +320,8 @@ host name without trailing dot." (read-response port)) (define* (open-socket-for-uri uri-or-string - #:key (verify-certificate? #t)) + #:key (verify-certificate? #t) + (flags 0)) "Return an open input/output port for a connection to URI-OR-STRING. When VERIFY-CERTIFICATE? is true, verify HTTPS server certificates." (define uri @@ -373,10 +374,18 @@ When VERIFY-CERTIFICATE? is true, verify HTTPS server certificates." (when (and https? (current-https-proxy)) (setup-http-tunnel s uri)) - (if https? - (tls-wrap s (uri-host uri) - #:verify-certificate? verify-certificate?) - s))) + (let ((port (if https? + (tls-wrap s (uri-host uri) + #:verify-certificate? verify-certificate?) + s))) + (unless (zero? flags) + ;; FLAGS might contain O_NONBLOCK. Thus, set it as a last step + ;; because 'handshake' otherwise throws an exception for + ;; GNUTLS_E_AGAIN. + (let ((initial-flags (fcntl s F_GETFL))) + (fcntl s F_SETFL (logior initial-flags flags)))) + + port))) (define (extend-request r k v . additional) (let ((r (set-field r (request-headers) [-- Attachment #3: Type: text/plain, Size: 392 bytes --] … which lets us do that: --8<---------------cut here---------------start------------->8--- scheme@(guile-user)> ,use(web client) scheme@(guile-user)> (define p (open-socket-for-uri "https://guix.gnu.org" #:flags O_NONBLOCK)) scheme@(guile-user)> (http-get "https://guix.gnu.org" #:port p) --8<---------------cut here---------------end--------------->8--- Thoughts? Ludo’.
Hello Ludo,
I don't have any comment/insight on what you're doing in general, except
about one of your points below:
Ludovic Courtès <ludo@gnu.org> writes:
> First, I noticed that GnuTLS doesn’t implement ‘write_wait_fd’, only
> ‘read_wait_fd’ (not sure how problematic that is):
>
> scheme@(guile-user)> ,use(web client)
> scheme@(guile-user)> (define p (open-socket-for-uri "https://guix.gnu.org"))
> scheme@(guile-user)> ((@@ (ice-9 suspendable-ports) wait-for-writable) p)
> ice-9/boot-9.scm:1685:16: In procedure raise-exception:
> In procedure write_wait_fd: unimplemented
>
> Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue.
> scheme@(guile-user) [1]> ,q
> scheme@(guile-user)> ,use(gnutls)
> scheme@(guile-user)> (gnutls-version)
> $1 = "3.7.7"
> scheme@(guile-user)> ((@@ (ice-9 suspendable-ports) wait-for-readable) p)
> $2 = 1
This occasionally causes problems when fetching substitutes, as can be
seen in bug #56005 (during substitution: write_wait_fd: unimplemented).
--
Thanks
Thiago
Hi,
Thiago Jung Bauermann <bauermann@kolabnow.com> skribis:
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> First, I noticed that GnuTLS doesn’t implement ‘write_wait_fd’, only
>> ‘read_wait_fd’ (not sure how problematic that is):
>>
>> scheme@(guile-user)> ,use(web client)
>> scheme@(guile-user)> (define p (open-socket-for-uri "https://guix.gnu.org"))
>> scheme@(guile-user)> ((@@ (ice-9 suspendable-ports) wait-for-writable) p)
>> ice-9/boot-9.scm:1685:16: In procedure raise-exception:
>> In procedure write_wait_fd: unimplemented
>>
>> Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue.
>> scheme@(guile-user) [1]> ,q
>> scheme@(guile-user)> ,use(gnutls)
>> scheme@(guile-user)> (gnutls-version)
>> $1 = "3.7.7"
>> scheme@(guile-user)> ((@@ (ice-9 suspendable-ports) wait-for-readable) p)
>> $2 = 1
>
> This occasionally causes problems when fetching substitutes, as can be
> seen in bug #56005 (during substitution: write_wait_fd: unimplemented).
Oh, I have not seen it but it’s weird: (guix scripts substitute) doesn’t
use O_NONBLOCK sockets, so I don’t get how it can hit that. Needs
investigation…
Thanks,
Ludo’.
On Thu, 04 Aug 2022 16:20:12 +0200 Ludovic Courtès <ludo@gnu.org> wrote: > Ludovic Courtès <ludo@gnu.org> skribis: > > > The custom input/output port wrapping the TLS session record port would > > introduce overhead, and it would also prevent its uses in a non-blocking > > context--e.g., with Fibers. The port close mechanism added in GnuTLS > > 3.7.7 allows us to get rid of that wrapper. > > > > * guix/build/download.scm (wrap-record-port-for-gnutls<3.7.7): New > > procedure, with code formerly in 'tls-wrap'. > > (tls-wrap): Check for 'set-session-record-port-close!' and use it when > > available; otherwise call 'wrap-record-port-for-gnutls<3.7.7'. > > I synchronized Guile's copy of this code: > > 317b06bf8 web: 'tls-wrap' retries handshake upon non-fatal errors. > c01ca10b3 web: Do not wrap TLS port on GnuTLS >= 3.7.7. > > I realized that’s not enough to make it possible to use non-blocking > ports though. [snip revised patch] > Thoughts? I have not been following these patches and this may be completely irrelevant, but if the context of these patches is the use of gnutls-guile, then when I last looked (which was some time ago) gnutls-guile ports were not suspendable. So a non-blocking gnutls-guile port will still block when used with guile's suspendable ports. If I have missed the point please ignore this.
[-- Attachment #1.1.1.1: Type: text/plain, Size: 1793 bytes --] On 02-08-2022 09:59, Ludovic Courtès wrote: >>> + (if (module-defined? (resolve-interface '(gnutls)) >>> + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 >> resolve-module (and presumably also sets #:ensure #t by default, which >> sometimes causes 'module not found' messages to be replaced by >> 'unbound variable', which I don't think is useful behaviour, can >> #:ensure be set to #false? > This is unnecessary: see the ‘load-gnutls’ mechanism there. The idiom > above is already used in a couple of places. I have looked at the 'load-gnutls' procedure, but I do not see how it avoids the issue I mentioned (*). I have also seen this idiom (resolve-interface and friends with #:ensure #t) before, in other places, but that doesn't make the idiom correct -- in fact, _because_ I've seen the idiom elsewhere causing problems, I recommend avoiding the same mistake here (and preferably also eliminating it elsewhere). More generally, the second sentence is a logical fallacy, a variant of "ad populum" -- the prevalency of a mistake does not make it correct and does not invalidate evidence of it being a mistake. To be clear, I am not referring to the existence/absence of compilation errors when compiling the Guix package without gnutls in the build environment, but to the confusing _contents_ of the error message and the odd semantics of #:ensure #t, and not only at compilation time but also at runtime. (*) The autoloading of gnutls in load-gnutls avoids compilation errors when gnutls is absent, but by the way it does it, it causes the module to be registered as 'it exists' even when it doesn't, so the information in the module system of Guix becomes incorrect. Greetings, Maxime. [-- Attachment #1.1.1.2: Type: text/html, Size: 2552 bytes --] [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 929 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 236 bytes --]
Hi, Maxime Devos <maximedevos@telenet.be> skribis: > On 02-08-2022 09:59, Ludovic Courtès wrote: >>>> + (if (module-defined? (resolve-interface '(gnutls)) >>>> + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 >>> resolve-module (and presumably also sets #:ensure #t by default, which >>> sometimes causes 'module not found' messages to be replaced by >>> 'unbound variable', which I don't think is useful behaviour, can >>> #:ensure be set to #false? >> This is unnecessary: see the ‘load-gnutls’ mechanism there. The idiom >> above is already used in a couple of places. > > I have looked at the 'load-gnutls' procedure, but I do not see how it > avoids the issue I mentioned (*). [...] > (*) The autoloading of gnutls in load-gnutls avoids compilation errors > when gnutls is absent, but by the way it does it, it causes the module > to be registered as 'it exists' even when it doesn't, so the > information in the module system of Guix becomes incorrect. I understand what you’re saying (I’m quite familiar with Guile’s module system :-) and I do agree that #:ensure #t can lead to bad surprises), but I don’t think this is correct: --8<---------------cut here---------------start------------->8--- scheme@(guile-user)> (resolve-interface '(xxx)) ice-9/boot-9.scm:1685:16: In procedure raise-exception: no code for module (xxx) Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue. scheme@(guile-user) [1]> ,q scheme@(guile-user)> (resolve-module '(xxx) #f #:ensure #f) $1 = #f --8<---------------cut here---------------end--------------->8--- This is because ‘resolve-interface’ does (resolve-module … #:ensure #f). Does that make sense? Thanks, Ludo’.
[-- Attachment #1.1.1: Type: text/plain, Size: 1129 bytes --] On 05-08-2022 10:31, Ludovic Courtès wrote: > I understand what you’re saying (I’m quite familiar with Guile’s module > system :-) and I do agree that #:ensure #t can lead to bad surprises), > but I don’t think this is correct: > > --8<---------------cut here---------------start------------->8--- > scheme@(guile-user)> (resolve-interface '(xxx)) > ice-9/boot-9.scm:1685:16: In procedure raise-exception: > no code for module (xxx) > > Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue. > scheme@(guile-user) [1]> ,q > scheme@(guile-user)> (resolve-module '(xxx) #f #:ensure #f) > $1 = #f > --8<---------------cut here---------------end--------------->8--- > > This is because ‘resolve-interface’ does (resolve-module … #:ensure #f). > > Does that make sense? Oops, I thought the #:ensure #f was universal to all the resolve-... interfaces, but apparently not for resole-interface! In that case, no problem, though I'd like to eventually make some changes to the Guile docs for clarity (and maybe change the default #:ensure #t -> #f) Greetings, Maxime. [-- Attachment #1.1.2: OpenPGP public key --] [-- Type: application/pgp-keys, Size: 929 bytes --] [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 236 bytes --]