[PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[Jookia]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4680 bytes --]

When downloading a repository through SVN over HTTP, do it using a proxy if
possible. This is especially useful for people who use Tor to do all their
downloading. This doesn't work with svn:// repositories to my knowledge.

* guix/build/svn.scm (svn-fetch): Pass the "servers:global:http-proxy-host"
  and "servers:global:http-proxy-port" configuration options to SVN if
  current-http-proxy is set. Bail if unable to parse the proxy to avoid leaks.
* guix/svn-download.scm (svn-fetch): Leak the http_proxy environment variable.
---
 guix/build/svn.scm    | 48 +++++++++++++++++++++++++++++++++++-------------
 guix/svn-download.scm |  2 ++
 2 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/guix/build/svn.scm b/guix/build/svn.scm
index 74fe084..2de5abc 100644
--- a/guix/build/svn.scm
+++ b/guix/build/svn.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2014 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2014 Sree Harsha Totakura <sreeharsha@totakura.in>
+;;; Copyright © 2016 Jookia <166291@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -19,6 +20,10 @@
 
 (define-module (guix build svn)
   #:use-module (guix build utils)
+  #:use-module (ice-9 format)
+  #:use-module (srfi srfi-2)
+  #:use-module (web uri)
+  #:use-module (web client)
   #:export (svn-fetch))
 
 ;;; Commentary:
@@ -32,18 +37,35 @@
                     #:key (svn-command "svn"))
   "Fetch REVISION from URL into DIRECTORY.  REVISION must be an integer, and a
 valid Subversion revision.  Return #t on success, #f otherwise."
-  (and (zero? (system* svn-command "checkout" "--non-interactive"
-                       ;; Trust the server certificate.  This is OK as we
-                       ;; verify the checksum later.  This can be removed when
-                       ;; ca-certificates package is added.
-                       "--trust-server-cert" "-r" (number->string revision)
-                       url directory))
-       (with-directory-excursion directory
-         (begin
-           ;; The contents of '.svn' vary as a function of the current status
-           ;; of the repo.  Since we want a fixed output, this directory needs
-           ;; to be taken out.
-           (delete-file-recursively ".svn")
-           #t))))
+  (define proxy-config
+    (if (current-http-proxy)
+      (and-let* ((proxy-uri  (string->uri (current-http-proxy)))
+                 (proxy-host (uri-host proxy-uri))
+                 (proxy-port (number->string (uri-port proxy-uri)))
+                 (config-host "servers:global:http-proxy-host=")
+                 (config-port "servers:global:http-proxy-port="))
+        `("--config-option" ,(string-append config-host proxy-host)
+          "--config-option" ,(string-append config-port proxy-port)))
+      '()))
+
+    (if proxy-config
+        (and (zero? (apply system* (append
+                      `(,svn-command "checkout")
+                      proxy-config
+                      `("--non-interactive"
+                      ;; Trust the server certificate.  This is OK as we
+                      ;; verify the checksum later.  This can be removed when
+                      ;; ca-certificates package is added.
+                      "--trust-server-cert" "-r" ,(number->string revision)
+                      ,url ,directory))))
+          (with-directory-excursion directory
+            (begin
+              ;; The contents of '.svn' vary as a function of the current status
+              ;; of the repo.  Since we want a fixed output, this directory needs
+              ;; to be taken out.
+              (delete-file-recursively ".svn")
+              #t)))
+        (format (current-error-port)
+              "Unable to parse current-http-proxy: ~s~%" (current-http-proxy))))
 
 ;;; svn.scm ends here
diff --git a/guix/svn-download.scm b/guix/svn-download.scm
index d6853ca..fbc96df 100644
--- a/guix/svn-download.scm
+++ b/guix/svn-download.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2014, 2015 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2014 Sree Harsha Totakura <sreeharsha@totakura.in>
+;;; Copyright © 2016 Jookia <166291@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -72,6 +73,7 @@ HASH-ALGO (a symbol).  Use NAME as the file name, or a generic name if #f."
                       #:recursive? #t
                       #:modules '((guix build svn)
                                   (guix build utils))
+                      #:leaked-env-vars '("http_proxy")
                       #:guile-for-build guile
                       #:local-build? #t)))
 
-- 
2.7.0

Re: [PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[Ludovic Courtès]

Jookia <166291@gmail.com> skribis:

> When downloading a repository through SVN over HTTP, do it using a proxy if
> possible. This is especially useful for people who use Tor to do all their
> downloading. This doesn't work with svn:// repositories to my knowledge.
>
> * guix/build/svn.scm (svn-fetch): Pass the "servers:global:http-proxy-host"
>   and "servers:global:http-proxy-port" configuration options to SVN if
>   current-http-proxy is set. Bail if unable to parse the proxy to avoid leaks.
> * guix/svn-download.scm (svn-fetch): Leak the http_proxy environment variable.

Sorry for the delay, and thanks for the useful patch!

I gather ‘svn’ doesn’t honor ‘http_proxy’ in the first place, right?
That would have simplified things.

Also, what about ‘https_proxy’?  (This could be left for a subsequent
patch, if you prefer.)

[...]

> +  (define proxy-config
> +    (if (current-http-proxy)
> +      (and-let* ((proxy-uri  (string->uri (current-http-proxy)))
> +                 (proxy-host (uri-host proxy-uri))
> +                 (proxy-port (number->string (uri-port proxy-uri)))
> +                 (config-host "servers:global:http-proxy-host=")
> +                 (config-port "servers:global:http-proxy-port="))
> +        `("--config-option" ,(string-append config-host proxy-host)
> +          "--config-option" ,(string-append config-port proxy-port)))
> +      '()))

I would suggest using an explicit (getenv "http_proxy") instead of
(current-http-proxy) since the latter is specific to the (web …)
modules, which are not involved here.

Also, I’m not a fan of SRFI-2.  :-)  So I’d write it as:

  (match (getenv "http_proxy")
    (#f '())
    (uri-string
     (let* ((uri  (string->uri uri-string))
            (host (uri-host uri))
            (port (uri-port uri)))
       (list (string-append
              "--config-option=servers:global:http-proxy-host="
              host)
             (string-append
              "--config-option=servers:global:http-proxy-port="
              (number->string port))))))

Probably with something like:

  (unless uri
    (format (current-error-port) "invalid HTTP proxy URI: ~s~%"
            uri-string)
    (exit 1))

Could you send an updated patch?

Thanks!

Ludo’.

Re: [PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[Jookia]

On Sun, Feb 28, 2016 at 05:44:18PM +0100, Ludovic Courtès wrote:
> Sorry for the delay, and thanks for the useful patch!

No problem. :)

> I gather ‘svn’ doesn’t honor ‘http_proxy’ in the first place, right?
> That would have simplified things.

It really would, but for some reason that's not a feature. Well, it was for one
or two releases but that was unintentional and fixed.

> Also, what about ‘https_proxy’?  (This could be left for a subsequent
> patch, if you prefer.)

I'd have to parse the subversion URL and figure out if its scheme is HTTPS as I
don't think Subverion has a https-proxy-* set of variables.

> > +  (define proxy-config
> > +    (if (current-http-proxy)
> > +      (and-let* ((proxy-uri  (string->uri (current-http-proxy)))
> > +                 (proxy-host (uri-host proxy-uri))
> > +                 (proxy-port (number->string (uri-port proxy-uri)))
> > +                 (config-host "servers:global:http-proxy-host=")
> > +                 (config-port "servers:global:http-proxy-port="))
> > +        `("--config-option" ,(string-append config-host proxy-host)
> > +          "--config-option" ,(string-append config-port proxy-port)))
> > +      '()))
> 
> I would suggest using an explicit (getenv "http_proxy") instead of
> (current-http-proxy) since the latter is specific to the (web …)
> modules, which are not involved here.
> 
> Also, I’m not a fan of SRFI-2.  :-)  So I’d write it as:
> 
>   (match (getenv "http_proxy")
>     (#f '())
>     (uri-string
>      (let* ((uri  (string->uri uri-string))
>             (host (uri-host uri))
>             (port (uri-port uri)))
>        (list (string-append
>               "--config-option=servers:global:http-proxy-host="
>               host)
>              (string-append
>               "--config-option=servers:global:http-proxy-port="
>               (number->string port))))))
> 
> Probably with something like:
> 
>   (unless uri
>     (format (current-error-port) "invalid HTTP proxy URI: ~s~%"
>             uri-string)
>     (exit 1))

Interesting!

> Could you send an updated patch?

Sure, I'll do it hopefully this week.

> Thanks!
>
> Ludo’.

Cheers,
Jookia.

Re: [PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[levenson]

Hi Jookia, Ludo

Was this patch merged? I haven't found it in guix-patches and commits =(.  I came across the same problem today by just doing guix pull. In addition, a package (texlive-hyph-utf8) uses svn:// scheme which I am not sure does respect those http-proxy variables. 

It would be also nice to have a http/https fallback in svn-download for such cases.

WDYT?

-- 
Alexey

Re: [PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[Jookia]

Hi Alexey,

I don't contribute to Guix and abandoned this patch.

Please untag me,
Jookia.

On Sun, Mar 15, 2020 at 12:22:08PM +0100, levenson@mmer.org wrote:
> Hi Jookia, Ludo
> 
> Was this patch merged? I haven't found it in guix-patches and commits =(.  I came across the same problem today by just doing guix pull. In addition, a package (texlive-hyph-utf8) uses svn:// scheme which I am not sure does respect those http-proxy variables. 
> 
> It would be also nice to have a http/https fallback in svn-download for such cases.
> 
> WDYT?
> 
> -- 
> Alexey

Re: [PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[Leo Famulari]

On Sun, Mar 15, 2020 at 12:22:08PM +0100, levenson@mmer.org wrote:
> Was this patch merged? I haven't found it in guix-patches and commits
> =(.  I came across the same problem today by just doing guix pull. In
> addition, a package (texlive-hyph-utf8) uses svn:// scheme which I am
> not sure does respect those http-proxy variables. 
> 
> It would be also nice to have a http/https fallback in svn-download
> for such cases.

If you can't find it in the commit log, then I doubt the problem was
ever addressed, since SVN and HTTP proxy usage are both somewhat rare.

If you are having this issue, then we need someone to finish the patch,
addressing Ludo's feedback.

Re: [PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[Jookia]

On Sun, Feb 28, 2016 at 05:44:18PM +0100, Ludovic Courtès wrote:
> > +  (define proxy-config
> > +    (if (current-http-proxy)
> > +      (and-let* ((proxy-uri  (string->uri (current-http-proxy)))
> > +                 (proxy-host (uri-host proxy-uri))
> > +                 (proxy-port (number->string (uri-port proxy-uri)))
> > +                 (config-host "servers:global:http-proxy-host=")
> > +                 (config-port "servers:global:http-proxy-port="))
> > +        `("--config-option" ,(string-append config-host proxy-host)
> > +          "--config-option" ,(string-append config-port proxy-port)))
> > +      '()))
> 
> I would suggest using an explicit (getenv "http_proxy") instead of
> (current-http-proxy) since the latter is specific to the (web …)
> modules, which are not involved here.

Sorry for the reply, now that I have an RFC for the coreboot patches I've looked
back at this. The URI module is also in the web module, and I'm not sure how I
feel about rewriting a URI parser to get the host and port out of http_proxy.

> Thanks!
> 
> Ludo’.

Thoughts?
Jookia.

Re: [PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[Ludovic Courtès]

Jookia <166291@gmail.com> skribis:

> On Sun, Feb 28, 2016 at 05:44:18PM +0100, Ludovic Courtès wrote:
>> > +  (define proxy-config
>> > +    (if (current-http-proxy)
>> > +      (and-let* ((proxy-uri  (string->uri (current-http-proxy)))
>> > +                 (proxy-host (uri-host proxy-uri))
>> > +                 (proxy-port (number->string (uri-port proxy-uri)))
>> > +                 (config-host "servers:global:http-proxy-host=")
>> > +                 (config-port "servers:global:http-proxy-port="))
>> > +        `("--config-option" ,(string-append config-host proxy-host)
>> > +          "--config-option" ,(string-append config-port proxy-port)))
>> > +      '()))
>> 
>> I would suggest using an explicit (getenv "http_proxy") instead of
>> (current-http-proxy) since the latter is specific to the (web …)
>> modules, which are not involved here.
>
> Sorry for the reply, now that I have an RFC for the coreboot patches I've looked
> back at this. The URI module is also in the web module, and I'm not sure how I
> feel about rewriting a URI parser to get the host and port out of http_proxy.

Of course you should keep using ‘string->uri’, no problem with that.

I’m just saying that the code should explicitly check for the
‘http_proxy’ environment variable instead of using the
‘current-http-proxy’ SRFI-9 parameter.

WDYT?

Ludo’.

Re: [PATCH 1/2] svn-download: Respect current-http-proxy when downloading.

[Jookia]

On Thu, Mar 03, 2016 at 05:39:21PM +0100, Ludovic Courtès wrote:
> Of course you should keep using ‘string->uri’, no problem with that.
> 
> I’m just saying that the code should explicitly check for the
> ‘http_proxy’ environment variable instead of using the
> ‘current-http-proxy’ SRFI-9 parameter.
> 
> WDYT?

I think I know what you mean, I'll do a patch up when I can. :)

> Ludo’.

Jookia.