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

  • Open
  • quality assurance status badge
Details
4 participants
  • Leo Famulari
  • Ludovic Courtès
  • Maxim Cournoyer
  • zimoun
Owner
unassigned
Submitted by
Leo Famulari
Severity
normal
L
L
Leo Famulari wrote on 30 Jan 2021 02:04
(address . guix-patches@gnu.org)
f9137838eca39b768e49f4ee7852dd32edce7e8c.1611968623.git.leo@famulari.name
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(-)

Toggle diff (90 lines)
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
Z
Z
zimoun wrote on 11 Mar 2021 01:14
(name . Leo Famulari)(address . leo@famulari.name)(address . 46182@debbugs.gnu.org)
86a6rabl7a.fsf@gmail.com
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:
Toggle quote (7 lines)
> 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?’

Toggle quote (5 lines)
> +(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)

[...]

Toggle quote (7 lines)
> + (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
Toggle diff (59 lines)
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")
L
L
Leo Famulari wrote on 11 Mar 2021 02:46
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 46182@debbugs.gnu.org)
YEl2fHXnDm6+GzFZ@jasmine.lan
On Thu, Mar 11, 2021 at 01:14:33AM +0100, zimoun wrote:
Toggle quote (3 lines)
> 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!
Z
Z
zimoun wrote on 11 Mar 2021 10:44
(name . Leo Famulari)(address . leo@famulari.name)(address . 46182@debbugs.gnu.org)
86k0qe9g8u.fsf@gmail.com
Hi Leo,

On Wed, 10 Mar 2021 at 20:46, Leo Famulari <leo@famulari.name> wrote:
Toggle quote (6 lines)
> 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. :-)

Toggle quote (2 lines)
> If this new patch is working for you, please push!

I do not have this power. :-)


Cheers,
simon
L
L
Ludovic Courtès wrote on 11 Mar 2021 23:29
Re: bug#46182: [PATCH] lint: Add 'check-git-protocol' checker.
(name . Leo Famulari)(address . leo@famulari.name)(address . 46182@debbugs.gnu.org)
87zgz9uxwu.fsf@gnu.org
Hi!

Leo Famulari <leo@famulari.name> skribis:

Toggle quote (7 lines)
> 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’.
M
M
Maxim Cournoyer wrote on 22 May 2022 06:15
(name . Leo Famulari)(address . leo@famulari.name)
87sfp26w4m.fsf_-_@gmail.com
tags 46182 +moreinfo
thanks

Hello,

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

Toggle quote (18 lines)
> 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
M
M
Maxim Cournoyer wrote on 20 Oct 2023 04:22
Re: [bug#46182] [PATCH] lint: Add 'check-git-protocol' checker.
(name . zimoun)(address . zimon.toutoune@gmail.com)
87pm1am3rt.fsf@gmail.com
Hello,

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

Toggle quote (16 lines)
> 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
S
S
Simon Tournier wrote on 20 Oct 2023 14:45
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87v8b1mph6.fsf@gmail.com
Hi Maxim,

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

Toggle quote (7 lines)
> 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.


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
M
M
Maxim Cournoyer wrote on 20 Oct 2023 17:37
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
877cnhi9t7.fsf@gmail.com
Hi,

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

Toggle quote (39 lines)
> 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
?