From aab42bcb212698bc1f61beb9f321ffbd751f36f5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo@gnu.org>
Date: Fri, 15 Dec 2017 09:57:04 +0100
Subject: [PATCH 1/2] substitute: Always allow substitutes for fixed-output
 derivation results.

Fixes <https://bugs.gnu.org/28659>.

* guix/scripts/substitute.scm (content-addressed-item?): New procedure.
(valid-narinfo?): Use it.
* nix/libstore/build.cc (DerivationGoal::haveDerivation): Always make a
substitution goal when 'fixedOutput' is true.
* tests/substitute.scm ("query unsigned narinfo for content-addressed
item"): New test.
---
 guix/scripts/substitute.scm | 31 ++++++++++++++++++++++++++++++-
 nix/libstore/build.cc       |  6 ++++--
 tests/substitute.scm        | 24 +++++++++++++++++++++++-
 3 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index 2fd2bf810..670a9b4dd 100755
--- a/guix/scripts/substitute.scm
+++ b/guix/scripts/substitute.scm
@@ -25,6 +25,9 @@
   #:use-module (guix config)
   #:use-module (guix records)
   #:use-module ((guix serialization) #:select (restore-file))
+  #:use-module ((guix derivations)
+                #:select (read-derivation-from-file
+                          fixed-output-derivation?))
   #:use-module (guix hash)
   #:use-module (guix base32)
   #:use-module (guix base64)
@@ -406,10 +409,36 @@ No authentication and authorization checks are performed here!"
        (let ((above-signature (string-take contents index)))
          (sha256 (string->utf8 above-signature)))))))
 
+(define* (content-addressed-item? item)
+  "Return true if ITEM is content-addressed---i.e., if ITEM is the result of a
+fixed-output derivation."
+  (guard (c ((nix-connection-error? c)
+             ;; We failed to connect, maybe because we have the wrong
+             ;; GUIX_DAEMON_SOCKET?  Let's conservatively assume that
+             ;; nothing's content-addressed.
+             #f))
+    (with-store store
+      (match (valid-derivers store item)
+        (()
+         ;; If there are no valid derivers it's most likely because ITEM is a
+         ;; source (added with 'add-to-store' or similar).  Nevertheless,
+         ;; since we can't be certain, return #f.
+         #f)
+        ((drv . _)
+         (fixed-output-derivation?
+          (read-derivation-from-file drv)))))))
+
 (define* (valid-narinfo? narinfo #:optional (acl (current-acl))
                          #:key verbose?)
-  "Return #t if NARINFO's signature is not valid."
+  "Return #t if NARINFO is \"valid\"---signed by an authorized key, or
+designating a content-addressed item."
   (or %allow-unauthenticated-substitutes?
+
+      ;; If NARINFO designates a content-addressed item, there's no point
+      ;; authenticating it.  Don't explicitly check 'narinfo-hash' for
+      ;; integrity: this will be done by the daemon once we've downloaded it.
+      (content-addressed-item? (narinfo-path narinfo))
+
       (let ((hash      (narinfo-sha256 narinfo))
             (signature (narinfo-signature narinfo))
             (uri       (uri->string (narinfo-uri narinfo))))
diff --git a/nix/libstore/build.cc b/nix/libstore/build.cc
index d68e8b2bc..03a8f5080 100644
--- a/nix/libstore/build.cc
+++ b/nix/libstore/build.cc
@@ -1034,8 +1034,10 @@ void DerivationGoal::haveDerivation()
 
     /* We are first going to try to create the invalid output paths
        through substitutes.  If that doesn't work, we'll build
-       them. */
-    if (settings.useSubstitutes && substitutesAllowed(drv))
+       them.  Always enable substitutes for fixed-output derivations to
+       protect against disappearing files and in-place modifications on
+       upstream sites.  */
+    if ((fixedOutput || settings.useSubstitutes) && substitutesAllowed(drv))
         foreach (PathSet::iterator, i, invalidOutputs)
             addWaitee(worker.makeSubstitutionGoal(*i, buildMode == bmRepair));
 
diff --git a/tests/substitute.scm b/tests/substitute.scm
index 0ad624795..03579b9f1 100644
--- a/tests/substitute.scm
+++ b/tests/substitute.scm
@@ -21,15 +21,17 @@
   #:use-module (guix scripts substitute)
   #:use-module (guix base64)
   #:use-module (guix hash)
+  #:use-module (guix derivations)
   #:use-module (guix serialization)
   #:use-module (guix pk-crypto)
   #:use-module (guix pki)
   #:use-module (guix config)
   #:use-module (guix base32)
-  #:use-module ((guix store) #:select (%store-prefix))
+  #:use-module ((guix store) #:select (%store-prefix with-store))
   #:use-module ((guix ui) #:select (guix-warning-port))
   #:use-module ((guix build utils)
                 #:select (mkdir-p delete-file-recursively))
+  #:use-module (guix tests)
   #:use-module (guix tests http)
   #:use-module (rnrs bytevectors)
   #:use-module (rnrs io ports)
@@ -241,6 +243,26 @@ a file for NARINFO."
            (lambda ()
              (guix-substitute "--query"))))))))
 
+(test-assert "query unsigned narinfo for content-addressed item"
+  (with-store store
+    (let* ((hash (sha256 (random-bytevector 128)))
+           (drv  (derivation store "content-addressed"
+                             "builtin:download" '()
+                             #:hash-algo 'sha256 #:hash hash)))
+      (define output
+        (with-output-to-string
+          (lambda ()
+            (with-derivation-narinfo drv (sha256 => hash)
+              (with-input-from-string (string-append "have "
+                                                     (derivation->output-path drv))
+                (lambda ()
+                  (set! (@@ (guix scripts substitute)
+                            %allow-unauthenticated-substitutes?)
+                    #f)
+                  (guix-substitute "--query")))))))
+
+      (string=? (string-trim-both output) (derivation->output-path drv)))))
+
 (test-quit "substitute, no signature"
     "no valid substitute"
   (with-narinfo %narinfo
-- 
2.15.1