bug#43501: Whitelisting/blacklisting transitive package dependencies

[Danny Milosavljevic]

Hi,

On Fri, 18 Sep 2020 23:40:13 -0400
Mark H Weaver <mhw@netris.org> wrote:

> I think it's important that Guix core functionality should be usable
> without installing a collection of patented media codecs.  Those plugins
> should be purely optional.  In my opinion, we should find a way to
> eliminate those dependencies.

I agree that it would be good to prevent weird dependencies from creeping 
in--for
your stated reasons, but also for a lot of other reasons, chief of which is that
the most secure source code is the source code that has been eliminated.

Also, Guix sometimes pulls in transitive dependencies for the weirdest things.

The "*-minimal" packages we have make it less bad.

Still, it would nice to also have something that automatically checks whether
there are weird transitive inputs of a package, for each package (*especially*
in order to use that for "-minimal" packages).

I sometimes add #:disallowed-references after tracking down problems of
unintended transitive inputs (for example see f2fs-tools/static).  But even
that disallows just one specific reference (one package version).

What I want is to disallow any package of that name entirely in the dependency
graph--or even disallow references to specific source files (or other groups of
packages) entirely.
And I want it to keep disallowing it mechanically without me having to
remember it.

Guix lint already does something like I want, but for direct (non-transitive)
inputs.

It should be possible to add a "guix lint" check that also checks transitive
inputs of packages for suspicious packages.

The maintenance of a transitive-whitelist/-blacklist per package would then
probably be have to be done inside guix lint, though.  It could be nicer if
there were package fields for those for it eventually.  But for now, I guess
inside guix lint is good enough.

That said, for practicality one has to find some kind of groups of packages,
in order to keep the whitelist/blacklist from ballooning.  For now, I assume
that each group has an extra source file--which we know is not true in Guix
right now.  But we could make it true.

I started to add something to guix lint (possible procedures to use:
package-transitive-inputs, package-transitive-propagated-inputs,
package-transitive-native-inputs)--see patch below.  But note that it just
complains about everything now--we would still have to specify what is
"bad".

I would suggest to have a whitelist (of file names) and a blacklist
(of file names), and the following:
If a package has a whitelist and a transitive dependency is not on the
whitelist, complain.  If a package does not have a whitelist but does
have a blacklist and a transitive dependency is on the blacklist, complain.

I still find it illuminating as it is now.  Try:

$ guix lint qemu
[...]
gnu/packages/virtualization.scm:260:5: qemu@5.0.0: 'gnu/packages/dbm.scm' 
should probably not be referred to (but it is--because of packages (gdbm))
[...]
gnu/packages/virtualization.scm:260:5: qemu@5.0.0: 'gnu/packages/spice.scm' 
should probably not be referred to (but it is by packages (libcacard spice 
usbredir virglrenderer spice-protocol))
gnu/packages/virtualization.scm:260:5: qemu@5.0.0: 'gnu/packages/gl.scm' should 
probably not be referred to (but it is by packages (libepoxy mesa))
[...]
gnu/packages/virtualization.scm:260:5: qemu@5.0.0: 'gnu/packages/dbm.scm' 
should probably not be referred to (but it is by packages (gdbm))
[...]
gnu/packages/virtualization.scm:260:5: qemu@5.0.0: 
'gnu/packages/pulseaudio.scm' should probably not be referred to (but it is 
because of packages (pulseaudio))

WTF!

Also, for the special case where no package in a source file A should refer to
any package in a source file B, it could be enough to establish a convention
of commenting out the respective "#:use-module (...)" in source file A (and
adding a "DO NOT USE" text to it), and never deleting that comment.

That way, once somebody had found what module one should not ever import, he
could document that fact.

diff --git a/guix/lint.scm b/guix/lint.scm
index ec43a4dcad..d65ac34441 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -72,6 +72,7 @@
   #:export (check-description-style
             check-inputs-should-be-native
             check-inputs-should-not-be-an-input-at-all
+            check-transitive-input-sanity
             check-patch-file-names
             check-synopsis-style
             check-derivation
@@ -287,6 +288,38 @@ of a package, and INPUT-NAMES, a list of package 
specifications such as
                                input))))
                  packages outputs))))
 
+(define (check-transitive-input-sanity package)
+  (let* ((examined-package-name (package-name package))
+         (examined-package-location (package-location package))
+         (examined-package-source-file-name (location-file 
examined-package-location))
+         (examined-package-dependency-source-file-names
+          (delete examined-package-source-file-name
+           (delete-duplicates
+            (map (match-lambda
+                  ((key dependency . rest) (location-file (package-location 
dependency))))
+             (package-transitive-target-inputs package))))))
+    (map (lambda (source-file-name)
+           (let ((packages-in-source-file
+                  (filter (match-lambda
+                            ((key dependency . rest) (string=? source-file-name
+                                                               (location-file
+                                                                
(package-location dependency)))))
+                          (package-transitive-target-inputs package))))
+             (make-warning package
+              (G_ "'~a' should probably not be referred to (but it is--because 
of packages ~a)")
+              (list source-file-name (map (match-lambda
+                                           ((key dependency . rest)
+                                            (package-name dependency)))
+                                          packages-in-source-file))
+               #:field 'inputs)))
+         examined-package-dependency-source-file-names)))
+          (delete examined-package-source-file-name
+           (delete-duplicates
+            (map (match-lambda
+                  ((key dependency . rest) (location-file (package-location 
dependency))))
+             (package-transitive-target-inputs package))))))
+    (map (lambda (source-file-name)
+           (let ((packages-in-source-file
+                  (filter (match-lambda
+                            ((key dependency . rest) (string=? source-file-name
+                                                               (location-file
+                                                                
(package-location dependency)))))
+                          (package-transitive-target-inputs package))))
+             (make-warning package
+              (G_ "'~a' should probably not be referred to (but it is--because 
of packages ~a)")
+              (list source-file-name (map (match-lambda
+                                           ((key dependency . rest)
+                                            (package-name dependency)))
+                                          packages-in-source-file))
+               #:field 'inputs)))
+         examined-package-dependency-source-file-names)))
+
+    ;; if examined-package-name like '%qemu%':
+    ;;   (package-name (map <car cdr> package-transitive-inputs)) no 
gstreamer; source file not
+    ;;   gstreamer.scm, gtk.scm.
+    ;;   allowed references to location-files
+  ;; TODO: gstreamer should not be anywhere in any transitive inputs of any 
qemu
+
 (define (check-inputs-should-be-native package)
   ;; Emit a warning if some inputs of PACKAGE are likely to belong to its
   ;; native inputs.
@@ -1378,6 +1411,10 @@ them for PACKAGE."
 
 (define %local-checkers
   (list
+   (lint-checker
+     (name        'transitive-inputs)
+     (description "Checks transitive inputs")
+     (check check-transitive-input-sanity))
    (lint-checker
      (name        'description)
      (description "Validate package descriptions")

pgpMU9RYCVeea.pgp
Description: OpenPGP digital signature