Re: [PATCH] package.el: check tarball signature

[Daiki Ueno]

Ted Zlatanov <address@hidden> writes:

> Just one code comment:
>
> +(defcustom package-check-signature 'allow-unsigned
> +  "Whether to check package signatures when installing."
> +  :type '(choice (const nil :tag "Never")
> +                (const allow-unsigned :tag "Allow unsigned")
> +                (const t :tag "Check always"))
> +  :risky t
> +  :group 'package
> +  :version "24.1")
>
> IMHO this should be per archive, not global.  WDYT?

Yes, actually I was in doubt how to support that.  Given that most of
the archives will be eventually signed (as Stefan pointed[1]), I'm now
thinking of:

* remove the package-check-signature option, and

* even if an archive is listed in package-unsigned-archives, check
  signature if .sig file is provided (ignoring verification error)

How does this sound?  Here is a patch in this direction.

Footnotes: 
[1]  http://article.gmane.org/gmane.emacs.devel/160658

=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el  2013-10-03 07:11:27 +0000
+++ lisp/emacs-lisp/package.el  2013-10-04 02:45:27 +0000
@@ -286,17 +286,9 @@
   :group 'package
   :version "24.1")
 
-(defcustom package-check-signature 'allow-unsigned
-  "Whether to check package signatures when installing."
-  :type '(choice (const nil :tag "Never")
-                (const allow-unsigned :tag "Allow unsigned")
-                (const t :tag "Check always"))
-  :risky t
-  :group 'package
-  :version "24.1")
-
-(defcustom package-unsigned-archives nil
-  "A list of archives which do not use package signature."
+;; FIXME: This should be null once "gnu" switches to signed archive.
+(defcustom package-unsigned-archives '("gnu")
+  "A list of archives allowed to have unsigned packages."
   :type '(repeat (string :tag "Archive name"))
   :risky t
   :group 'package
@@ -809,7 +801,7 @@
 (declare-function epg-signature-status "epg" (signature))
 (declare-function epg-signature-to-string "epg" (signature))
 
-(defun package--check-signature (location file)
+(defun package--check-signature (location file allow-unsigned)
   "Check signature of the current buffer.
 GnuPG keyring is located under \"gnupg\" in `package-user-dir'."
   (let ((context (epg-make-context 'OpenPGP))
@@ -831,7 +823,8 @@
                                  sig))
                            (epg-context-result-for context 'verify))))
     (if (null good-signatures)
-       (error "Failed to verify signature %s: %S"
+       (apply (if allow-unsigned #'message #'error)
+              "Failed to verify signature %s: %S"
               sig-file
               (mapcar #'epg-signature-to-string
                       (epg-context-result-for context 'verify)))
@@ -843,16 +836,17 @@
         (file (concat (package-desc-full-name pkg-desc)
                       (package-desc-suffix pkg-desc)))
         (sig-file (concat file ".sig"))
+        (allow-unsigned (member (package-desc-archive pkg-desc)
+                                package-unsigned-archives))
         good-signatures pkg-descs)
     (package--with-work-buffer location file
-      (if (and package-check-signature
-              (not (member (package-desc-archive pkg-desc)
-                           package-unsigned-archives)))
-         (if (package--archive-file-exists-p location sig-file)
-             (setq good-signatures (package--check-signature location file))
-           (unless (eq package-check-signature 'allow-unsigned)
-             (error "Unsigned package: `%s'"
-                    (package-desc-name pkg-desc)))))
+      ;; Check signature of package if .sig file exists.
+      (if (package--archive-file-exists-p location sig-file)
+         (setq good-signatures (package--check-signature location
+                                                         file
+                                                         allow-unsigned))
+       (unless allow-unsigned
+         (error "Unsigned package: `%s'" (package-desc-name pkg-desc))))
       (package-unpack pkg-desc))
     ;; Here the package has been installed successfully, mark it as
     ;; signed if appropriate.
@@ -1222,17 +1216,17 @@
   (let ((dir (expand-file-name (format "archives/%s" (car archive))
                               package-user-dir))
        (sig-file (concat file ".sig"))
+       (allow-unsigned (member (car archive)
+                                package-unsigned-archives))
        good-signatures)
     (package--with-work-buffer (cdr archive) file
-      ;; Check signature of archive-contents, if desired.
-      (if (and package-check-signature
-              (not (member archive package-unsigned-archives)))
-         (if (package--archive-file-exists-p (cdr archive) sig-file)
-             (setq good-signatures (package--check-signature (cdr archive)
-                                                             file))
-           (unless (eq package-check-signature 'allow-unsigned)
-             (error "Unsigned archive `%s'"
-                    (car archive)))))
+      ;; Check signature of archive-contents if .sig file exists.
+      (if (package--archive-file-exists-p (cdr archive) sig-file)
+         (setq good-signatures (package--check-signature (cdr archive)
+                                                         file
+                                                         allow-unsigned))
+       (unless allow-unsigned
+         (error "Unsigned archive `%s'" (car archive))))
       ;; Read the retrieved buffer to make sure it is valid (e.g. it
       ;; may fetch a URL redirect page).
       (when (listp (read buffer))

=== modified file 'test/automated/package-test.el'
--- test/automated/package-test.el      2013-10-03 07:11:27 +0000
+++ test/automated/package-test.el      2013-10-04 02:40:31 +0000
@@ -347,8 +347,8 @@
      (should (search-forward "This is a bare-bones readme file for the 
multi-file"
                              nil t)))))
 
-(ert-deftest package-test-signed ()
-  "Test verifying package signature."
+(ert-deftest package-test-signed-good ()
+  "Test verifying package signature (good)."
   :expected-result (condition-case nil
                       (progn
                         (epg-check-configuration (epg-configuration))
@@ -361,14 +361,11 @@
       (package-initialize)
       (package-import-keyring keyring)
       (package-refresh-contents)
-      (should (package-install 'signed-good))
-      (should-error (package-install 'signed-bad))
-      ;; Check if the installed package status is updated.
+      (package-install 'signed-good)
       (let ((buf (package-list-packages)))
        (package-menu-refresh)
        (should (re-search-forward "^\\s-+signed-good\\s-+1\\.0\\s-+installed"
                                   nil t)))
-      ;; Check if the package description is updated.
       (with-fake-help-buffer
        (describe-package 'signed-good)
        (goto-char (point-min))
@@ -378,6 +375,24 @@
                        (expand-file-name "signed-good-1.0" package-user-dir))
                nil t))))))
 
+(ert-deftest package-test-signed-bad ()
+  "Test verifying package signature (bad)."
+  :expected-result (condition-case nil
+                      (progn
+                        (epg-check-configuration (epg-configuration))
+                        :passed)
+                    (error :failed))
+  (let* ((keyring (expand-file-name "key.pub" package-test-data-dir))
+        (package-test-data-dir
+          (expand-file-name "data/package/signed" package-test-file-dir))
+        ;; Disable unsigned packages.
+        package-unsigned-archives)
+    (with-package-test ()
+      (package-initialize)
+      (package-import-keyring keyring)
+      (package-refresh-contents)
+      (should-error (package-install 'signed-bad)))))
+
 (provide 'package-test)
 
 ;;; package-test.el ends here

Regards,
-- 
Daiki Ueno