[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[Leo Famulari]

We could also make it warn about use of the HTTP protocol (as opposed to
HTTPS). Your thoughts?

* guix/lint.scm (check-git-protocol): New procedure.
(%local-checkers): Add 'git-protocol' checker.
* doc/guix.texi (Invoking guix lint): Document it.
---
 doc/guix.texi |  6 +++++-
 guix/lint.scm | 25 ++++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index ff9e8da2e0..d17e2f2e96 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -28,7 +28,7 @@ Copyright @copyright{} 2014, 2015, 2016 Alex Kost@*
 Copyright @copyright{} 2015, 2016 Mathieu Lirzin@*
 Copyright @copyright{} 2014 Pierre-Antoine Rault@*
 Copyright @copyright{} 2015 Taylan Ulrich Bayırlı/Kammer@*
-Copyright @copyright{} 2015, 2016, 2017, 2019, 2020 Leo Famulari@*
+Copyright @copyright{} 2015, 2016, 2017, 2019, 2020, 2021 Leo Famulari@*
 Copyright @copyright{} 2015, 2016, 2017, 2018, 2019, 2020 Ricardo Wurmus@*
 Copyright @copyright{} 2016 Ben Woodcroft@*
 Copyright @copyright{} 2016, 2017, 2018 Chris Marusich@*
@@ -11736,6 +11736,10 @@ Parse the @code{source} URL to determine if a tarball from GitHub is
 autogenerated or if it is a release tarball.  Unfortunately GitHub's
 autogenerated tarballs are sometimes regenerated.
 
+@item git-protocol
+Check if the package's source code is fetched using the insecure @code{git://}
+protocol.
+
 @item derivation
 Check that the derivation of the given packages can be successfully
 computed for all the supported systems (@pxref{Derivations}).
diff --git a/guix/lint.scm b/guix/lint.scm
index 311bc94cc3..5a609b0454 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -11,6 +11,7 @@
 ;;; Copyright © 2018, 2019 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2020 Chris Marusich <cmmarusich@gmail.com>
 ;;; Copyright © 2020 Timothy Sample <samplet@ngyro.com>
+;;; Copyright © 2021 Leo Famulari <leo@famulari.name>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -51,7 +52,7 @@
   #:use-module (guix gnu-maintenance)
   #:use-module (guix cve)
   #:use-module ((guix swh) #:hide (origin?))
-  #:autoload   (guix git-download) (git-reference?
+  #:autoload   (guix git-download) (git-reference? git-fetch
                                     git-reference-url git-reference-commit)
   #:use-module (guix import stackage)
   #:use-module (ice-9 match)
@@ -84,6 +85,7 @@
             check-source
             check-source-file-name
             check-source-unstable-tarball
+            check-git-protocol
             check-mirror-url
             check-github-url
             check-license
@@ -918,6 +920,23 @@ descriptions maintained upstream."
                     (origin-uris origin))
         '())))
 
+(define (check-git-protocol package)
+  "Emit a warning if PACKAGE's source URI protocol is 'git://'."
+  (define (check-source-uri-scheme uri)
+    (if (eqv? (uri-scheme uri) 'git)
+        (list
+         (make-warning package
+                       (G_ "the source URI should not use the git:// protocol")
+                       #:field 'source))
+        '()))
+
+  (let ((origin (package-source package)))
+    (if (and (origin? origin)
+             (eqv? (origin-method origin) git-fetch))
+        (check-source-uri-scheme
+          (string->uri (git-reference-url (origin-uri origin))))
+        '())))
+
 (define (check-mirror-url package)
   "Check whether PACKAGE uses source URLs that should be 'mirror://'."
   (define (check-mirror-uri uri)                  ;XXX: could be optimized
@@ -1476,6 +1495,10 @@ or a list thereof")
      (name        'source-unstable-tarball)
      (description "Check for autogenerated tarballs")
      (check       check-source-unstable-tarball))
+   (lint-checker
+     (name        'git-protocol)
+     (description "Check for use of the git:// protocol")
+     (check       check-git-protocol))
    (lint-checker
      (name            'derivation)
      (description     "Report failure to compile a package to a derivation")
-- 
2.30.0

[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[zimoun]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

Hi Leo,

Giving a look to the bug tracker for the next release, I see this
bug. :-)


On Fri, 29 Jan 2021 at 20:04, Leo Famulari <leo@famulari.name> wrote:
> We could also make it warn about use of the HTTP protocol (as opposed to
> HTTPS). Your thoughts?
>
> * guix/lint.scm (check-git-protocol): New procedure.
> (%local-checkers): Add 'git-protocol' checker.
> * doc/guix.texi (Invoking guix lint): Document it.

The doc/ does not apply anymore.


Instead of these ’eqv?’

> +(define (check-git-protocol package)
> +  "Emit a warning if PACKAGE's source URI protocol is 'git://'."
> +  (define (check-source-uri-scheme uri)
> +    (if (eqv? (uri-scheme uri) 'git)

[...]

> +  (let ((origin (package-source package)))
> +    (if (and (origin? origin)
> +             (eqv? (origin-method origin) git-fetch))
> +        (check-source-uri-scheme
> +          (string->uri (git-reference-url (origin-uri origin))))
> +        '())))

I propose ’match’ which is more coherent with the Guix style.  Well,
from my understanding. :-)

Patch attached.  Well, it could be nice to add a test in
tests/guix-lint.sh for that.  WDYT?


Cheers,
simon


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: lint-git-protocol.patch --]
[-- Type: text/x-diff, Size: 2155 bytes --]

diff --git a/guix/lint.scm b/guix/lint.scm
index 311bc94cc3..980f77c736 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -51,7 +51,7 @@
   #:use-module (guix gnu-maintenance)
   #:use-module (guix cve)
   #:use-module ((guix swh) #:hide (origin?))
-  #:autoload   (guix git-download) (git-reference?
+  #:autoload   (guix git-download) (git-reference? git-fetch
                                     git-reference-url git-reference-commit)
   #:use-module (guix import stackage)
   #:use-module (ice-9 match)
@@ -84,6 +84,7 @@
             check-source
             check-source-file-name
             check-source-unstable-tarball
+            check-git-protocol
             check-mirror-url
             check-github-url
             check-license
@@ -918,6 +919,26 @@ descriptions maintained upstream."
                     (origin-uris origin))
         '())))
 
+(define (check-git-protocol package)
+  "Emit a warning if PACKAGE's source URI protocol is 'git://'."
+  (define (check-source-uri-scheme uri)
+    (match (uri-scheme uri)
+      ('git
+       (list
+         (make-warning package
+                       (G_ "the source URI should not use the git:// protocol")
+                       #:field 'source)))
+      (_ '())))
+
+  (match (package-source package)
+    ((? origin? origin)
+     (match (origin-method origin)
+       (git-fetch
+        (check-source-uri-scheme
+         (string->uri (git-reference-url (origin-uri origin)))))
+       (_ '())))
+    (_ '())))
+
 (define (check-mirror-url package)
   "Check whether PACKAGE uses source URLs that should be 'mirror://'."
   (define (check-mirror-uri uri)                  ;XXX: could be optimized
@@ -1476,6 +1497,10 @@ or a list thereof")
      (name        'source-unstable-tarball)
      (description "Check for autogenerated tarballs")
      (check       check-source-unstable-tarball))
+   (lint-checker
+     (name        'git-protocol)
+     (description "Check for use of the git:// protocol")
+     (check       check-git-protocol))
    (lint-checker
      (name            'derivation)
      (description     "Report failure to compile a package to a derivation")

[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[Leo Famulari]

On Thu, Mar 11, 2021 at 01:14:33AM +0100, zimoun wrote:
> I propose ’match’ which is more coherent with the Guix style.  Well,
> from my understanding. :-)

I have heard that before, but I don't know how to use it 🤷

If this new patch is working for you, please push!

[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[zimoun]

Hi Leo,

On Wed, 10 Mar 2021 at 20:46, Leo Famulari <leo@famulari.name> wrote:
> On Thu, Mar 11, 2021 at 01:14:33AM +0100, zimoun wrote:
>> I propose ’match’ which is more coherent with the Guix style.  Well,
>> from my understanding. :-)
>
> I have heard that before, but I don't know how to use it 🤷

The section [1] in the manual is worth to read because running and
playing with the examples gives a good feeling on how to use it. :-)

> If this new patch is working for you, please push!

I do not have this power. :-)


Cheers,
simon

[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[Ludovic Courtès]

Hi!

Leo Famulari <leo@famulari.name> skribis:

> We could also make it warn about use of the HTTP protocol (as opposed to
> HTTPS). Your thoughts?
>
> * guix/lint.scm (check-git-protocol): New procedure.
> (%local-checkers): Add 'git-protocol' checker.
> * doc/guix.texi (Invoking guix lint): Document it.

Nice!  I think it’s OK to use ‘eqv?’ here (‘eq?’, even).

One nit: it would be nice to add a positive and a negative test in
tests/lint.scm.  You can run “make check TESTS=tests/lint.scm” then.

Otherwise LGTM!

Thanks,
Ludo’.

[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[Maxim Cournoyer]

tags 46182 +moreinfo
thanks

Hello,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi!
>
> Leo Famulari <leo@famulari.name> skribis:
>
>> We could also make it warn about use of the HTTP protocol (as opposed to
>> HTTPS). Your thoughts?
>>
>> * guix/lint.scm (check-git-protocol): New procedure.
>> (%local-checkers): Add 'git-protocol' checker.
>> * doc/guix.texi (Invoking guix lint): Document it.
>
> Nice!  I think it’s OK to use ‘eqv?’ here (‘eq?’, even).
>
> One nit: it would be nice to add a positive and a negative test in
> tests/lint.scm.  You can run “make check TESTS=tests/lint.scm” then.
>
> Otherwise LGTM!

Gentle ping to Leo :-).

It looks near ready.

Thank you!

Maxim

[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[Maxim Cournoyer]

Hello,

zimoun <zimon.toutoune@gmail.com> writes:

> Hi Leo,
>
> On Wed, 10 Mar 2021 at 20:46, Leo Famulari <leo@famulari.name> wrote:
>> On Thu, Mar 11, 2021 at 01:14:33AM +0100, zimoun wrote:
>>> I propose ’match’ which is more coherent with the Guix style.  Well,
>>> from my understanding. :-)
>>
>> I have heard that before, but I don't know how to use it 🤷
>
> The section [1] in the manual is worth to read because running and
> playing with the examples gives a good feeling on how to use it. :-)
>
>> If this new patch is working for you, please push!
>
> I do not have this power. :-)

No longer true ;-).

Thinking about this change though; why is it bad to fetch from git
places?  There may be repos out there where it's the only offered way,
and as long as we're talking fixed output derivations, it seems moot
whether you use HTTPS, HTTP or X to retrieve the files (unless you are
worried about your traffic being monitored, but that's not in scope, I'd
say).

-- 
Thanks,
Maxim

[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[Simon Tournier]

Hi Maxim,

On Thu, 19 Oct 2023 at 22:22, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> Thinking about this change though; why is it bad to fetch from git
> places?  There may be repos out there where it's the only offered way,
> and as long as we're talking fixed output derivations, it seems moot
> whether you use HTTPS, HTTP or X to retrieve the files (unless you are
> worried about your traffic being monitored, but that's not in scope, I'd
> say).

Why would not it be in scope?

Being able to strongly verify (sha256) that the content you fetch is the
data you expect does not imply that the protocol for communicating
cannot be exploited for other means.

Well, git:// protocol is not supported by well-known forges.  Quoting
Pro Git book:

        The Cons

        Due to the lack of TLS or other cryptography, cloning over
        git:// might lead to an arbitrary code execution vulnerability,
        and should therefore be avoided unless you know what you are
        doing.

        https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols

And I do not have enough imagination to find a way to exploit the git://
protocol.  However, it appears to me a good practise to warn when this
protocol is used.  Somehow, a lint message is a recommendation – a good
practise – and not an absolute truth. :-)

In short, from my point of view, the general rule reads: avoid git://
protocol if you can.  Obviously, if you cannot because it is the only
offered way by some repositories, then let make an exception; but it
does mean that’s a good practise.

Cheers,
simon

[bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.

[Maxim Cournoyer]

Hi,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Hi Maxim,
>
> On Thu, 19 Oct 2023 at 22:22, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>> Thinking about this change though; why is it bad to fetch from git
>> places?  There may be repos out there where it's the only offered way,
>> and as long as we're talking fixed output derivations, it seems moot
>> whether you use HTTPS, HTTP or X to retrieve the files (unless you are
>> worried about your traffic being monitored, but that's not in scope, I'd
>> say).
>
> Why would not it be in scope?
>
> Being able to strongly verify (sha256) that the content you fetch is the
> data you expect does not imply that the protocol for communicating
> cannot be exploited for other means.
>
> Well, git:// protocol is not supported by well-known forges.  Quoting
> Pro Git book:
>
>         The Cons
>
>         Due to the lack of TLS or other cryptography, cloning over
>         git:// might lead to an arbitrary code execution vulnerability,
>         and should therefore be avoided unless you know what you are
>         doing.
>
>         https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols
>
> And I do not have enough imagination to find a way to exploit the git://
> protocol.  However, it appears to me a good practise to warn when this
> protocol is used.  Somehow, a lint message is a recommendation – a good
> practise – and not an absolute truth. :-)
>
> In short, from my point of view, the general rule reads: avoid git://
> protocol if you can.  Obviously, if you cannot because it is the only
> offered way by some repositories, then let make an exception; but it
> does mean that’s a good practise.

OK, fair.  I remove my objection, but I dislike warnings when they
cannot be acted upon (e.g. 'no coverage in software heritage' -- OK
neat, but I can't do anything about it, and it may not even support that
tarball ingestion yet).

-- 
Thanks,
Maxim