[PATCH] package.el: check tarball signature

[PATCH] package.el: check tarball signature

[Daiki Ueno]

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

Well, I still don't understand why this is advertised as such a
difficult problem, particularly why package.el would need sign operation
with Emacs.  Am I missing something?

Perhaps it might make sense to discuss with some code.  Here it is.

The code verifies a detached signature NAME-VERSION.tar.sig with a
trusted keyring located under ~/.emacs.d/elpa/gnupg/.  That's it.

For uploading packages, we could simply use the same mechanism as
gnupload in Gnulib.

It's actually a 10-minute work at an airport lobby and tested only with
the local package archive.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: package-signature.patch --]
[-- Type: text/x-patch, Size: 1887 bytes --]

=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el	2013-08-03 02:34:22 +0000
+++ lisp/emacs-lisp/package.el	2013-09-30 16:50:40 +0000
@@ -739,13 +739,44 @@
       (error "Error during download request:%s"
 	     (buffer-substring-no-properties (point) (line-end-position))))))
 
+(declare-function epg-make-context "epg"
+		  (&optional protocol armor textmode include-certs
+			     cipher-algorithm
+			     digest-algorithm
+			     compress-algorithm))
+(declare-function epg-context-set-home-directory "epg" (context directory))
+(declare-function epg-verify-file "epg" (context signature
+						 &optional signed-text plain))
+
+(defun package--check-signature (pkg-desc)
+  "Check signature of a package.
+GnuPG keyring is located under \"gnupg\" in `package-user-dir'."
+  (let* ((location (package-archive-base pkg-desc))
+	 (sig-file (concat (package-desc-full-name pkg-desc)
+			   (package-desc-suffix pkg-desc)
+			   ".sig"))
+	 (signature (package--with-work-buffer location sig-file
+		      (buffer-string)))
+	 (context (epg-make-context 'OpenPGP)))
+    (epg-context-set-home-directory context
+				    (expand-file-name "gnupg" package-user-dir))
+    (epg-verify-file context signature (buffer-string))))
+
 (defun package-install-from-archive (pkg-desc)
   "Download and install a tar package."
   (let ((location (package-archive-base pkg-desc))
 	(file (concat (package-desc-full-name pkg-desc)
                       (package-desc-suffix pkg-desc))))
     (package--with-work-buffer location file
-      (package-unpack pkg-desc))))
+      (if (condition-case nil
+	      (progn
+		(package--check-signature pkg-desc)
+		t)
+	    (error (y-or-n-p
+		    (format "Cannot verify signature of `%s'; \
+install it anyway? "
+			    (package-desc-name pkg-desc)))))
+	  (package-unpack pkg-desc)))))
 
 (defvar package--initialized nil)
 


[-- Attachment #3: Type: text/plain, Size: 25 bytes --]


Regards,
-- 
Daiki Ueno

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

[Eli Zaretskii]

> From: Daiki Ueno <ueno@gnu.org>
> Date: Mon, 30 Sep 2013 15:48:16 -0400
> 
> Well, I still don't understand why this is advertised as such a
> difficult problem, particularly why package.el would need sign operation
> with Emacs.  Am I missing something?
> 
> Perhaps it might make sense to discuss with some code.  Here it is.
> 
> The code verifies a detached signature NAME-VERSION.tar.sig with a
> trusted keyring located under ~/.emacs.d/elpa/gnupg/.  That's it.
> 
> For uploading packages, we could simply use the same mechanism as
> gnupload in Gnulib.
> 
> It's actually a 10-minute work at an airport lobby and tested only with
> the local package archive.

Thanks, but please add a defcustom to disable this check (e.g.,
because gnupg isn't installed, and isn't going to be).

In general, I think .sig files are there for those who want to verify
the packages, but users should not be forced to do that as a
prerequisite for downloading.  (And no, the y-or-n-p question doesn't
cut it: it's a nuisance to have to answer that question every time.)

[PATCHv2] package.el: check tarball signature

[Daiki Ueno]

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

Hi,

Thanks for the suggestion.

(Sorry for the delay, I'm just back from Boston ;-)

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but please add a defcustom to disable this check (e.g.,
> because gnupg isn't installed, and isn't going to be).

Done.  Now it has package-check-signature option, which can be set
either: nil (no signature verification), t (always check signature), or
allow-unsigned (skip signature verification if no .sig file is provided,
default).

Actually I wondered whether it should be a per-archive option rather
than a global option.  But I'd leave it as global, for simplicity.

> In general, I think .sig files are there for those who want to verify
> the packages, but users should not be forced to do that as a
> prerequisite for downloading.  (And no, the y-or-n-p question doesn't
> cut it: it's a nuisance to have to answer that question every time.)

Agreed.  Removed the y-or-n-p question.

Other than those, I changed a bit:

* display "unsigned" status on the package listing and the description
  buffer.

* fixed the verification logic.  The .sig file might contain multiple
  signatures and it should be considered as verified when one of those
  is good.

* import the default keyring from <data-directory>/package-keyring.gpg.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: package-signature.patch --]
[-- Type: text/x-diff, Size: 10519 bytes --]

=== modified file 'lisp/emacs-lisp/package.el'
--- lisp/emacs-lisp/package.el	2013-10-01 15:52:53 +0000
+++ lisp/emacs-lisp/package.el	2013-10-02 05:48:13 +0000
@@ -206,6 +206,7 @@
 (defvar Info-directory-list)
 (declare-function info-initialize "info" ())
 (declare-function url-http-parse-response "url-http" ())
+(declare-function url-http-file-exists-p "url-http" (url))
 (declare-function lm-header "lisp-mnt" (header))
 (declare-function lm-commentary "lisp-mnt" (&optional file))
 (defvar url-http-end-of-headers)
@@ -285,6 +286,15 @@
   :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")
+
 (defvar package--default-summary "No description available.")
 
 (cl-defstruct (package-desc
@@ -340,7 +350,9 @@
 `dir'	The directory where the package is installed (if installed),
 	`builtin' if it is built-in, or nil otherwise.
 
-`extras' Optional alist of additional keyword-value pairs."
+`extras' Optional alist of additional keyword-value pairs.
+
+`signed' Flag to indicate that the package is signed by provider."
   name
   version
   (summary package--default-summary)
@@ -348,7 +360,8 @@
   kind
   archive
   dir
-  extras)
+  extras
+  signed)
 
 ;; Pseudo fields.
 (defun package-desc-full-name (pkg-desc)
@@ -428,7 +441,8 @@
 (defun package-load-descriptor (pkg-dir)
   "Load the description file in directory PKG-DIR."
   (let ((pkg-file (expand-file-name (package--description-file pkg-dir)
-                                    pkg-dir)))
+                                    pkg-dir))
+	(signed-file (concat pkg-dir ".signed")))
     (when (file-exists-p pkg-file)
       (with-temp-buffer
         (insert-file-contents pkg-file)
@@ -436,6 +450,8 @@
         (let ((pkg-desc (package-process-define-package
                          (read (current-buffer)) pkg-file)))
           (setf (package-desc-dir pkg-desc) pkg-dir)
+	  (if (file-exists-p signed-file)
+	      (setf (package-desc-signed pkg-desc) t))
           pkg-desc)))))
 
 (defun package-load-all-descriptors ()
@@ -766,13 +782,90 @@
       (error "Error during download request:%s"
 	     (buffer-substring-no-properties (point) (line-end-position))))))
 
+(defun package--archive-file-exists-p (location file)
+  (let ((http (string-match "\\`https?:" location)))
+    (if http
+	(progn
+	  (require 'url-http)
+	  (url-http-file-exists-p (concat location file)))
+      (file-exists-p (expand-file-name location file)))))
+
+(declare-function epg-make-context "epg"
+		  (&optional protocol armor textmode include-certs
+			     cipher-algorithm
+			     digest-algorithm
+			     compress-algorithm))
+(declare-function epg-context-set-home-directory "epg" (context directory))
+(declare-function epg-verify-string "epg" (context signature
+						   &optional signed-text))
+(declare-function epg-context-result-for "epg" (context name))
+(declare-function epg-signature-status "epg" (signature))
+(declare-function epg-signature-to-string "epg" (signature))
+
+(defun package--check-signature (pkg-desc)
+  "Check signature of a package.
+GnuPG keyring is located under \"gnupg\" in `package-user-dir'."
+  (let ((location (package-archive-base pkg-desc))
+	(context (epg-make-context 'OpenPGP))
+	(homedir (expand-file-name "gnupg" package-user-dir))
+	(sig-file (concat (package-desc-full-name pkg-desc)
+			  (package-desc-suffix pkg-desc)
+			  ".sig"))
+	sig-content
+	good-signatures)
+    (condition-case-unless-debug error
+	(setq sig-content (package--with-work-buffer location sig-file
+			    (buffer-string)))
+      (error "Failed to download %s: %S" sig-file (cdr error)))
+    (epg-context-set-home-directory context homedir)
+    (epg-verify-string context sig-content (buffer-string))
+    ;; The .sig file may contain multiple signatures.  Success if one
+    ;; of the signatures is good.
+    (setq good-signatures
+	  (delq nil (mapcar (lambda (sig)
+			      (if (eq (epg-signature-status sig) 'good)
+				  sig))
+			    (epg-context-result-for context 'verify))))
+    (if (null good-signatures)
+	(error "Failed to verify signature %s: %S"
+	       sig-file
+	       (mapcar #'epg-signature-to-string
+		       (epg-context-result-for context 'verify))))))
+
 (defun package-install-from-archive (pkg-desc)
   "Download and install a tar package."
   (let ((location (package-archive-base pkg-desc))
 	(file (concat (package-desc-full-name pkg-desc)
-                      (package-desc-suffix pkg-desc))))
+                      (package-desc-suffix pkg-desc)))
+	(sig-file (concat (package-desc-full-name pkg-desc)
+			  (package-desc-suffix pkg-desc)
+			  ".sig"))
+	signed pkg-descs)
     (package--with-work-buffer location file
-      (package-unpack pkg-desc))))
+      (if package-check-signature
+	  (if (package--archive-file-exists-p location sig-file)
+	      (progn
+		(package--check-signature pkg-desc)
+		(setq signed t))
+	    (unless (eq package-check-signature '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.
+    (when signed
+      ;; Create an empty NAME-VERSION.signed file, which indicates the
+      ;; signature of the package was verified on installation.
+      (write-region "" nil (expand-file-name
+			    (concat (package-desc-full-name pkg-desc)
+				    ".signed")
+			    package-user-dir))
+      ;; Update the old pkg-desc which will be shown on the description buffer.
+      (setf (package-desc-signed pkg-desc) t)
+      ;; Update the new (activated) pkg-desc as well.
+      (setq pkg-descs (cdr (assq (package-desc-name pkg-desc) package-alist)))
+      (if pkg-descs
+	  (setf (package-desc-signed (car pkg-descs)) t)))))
 
 (defvar package--initialized nil)
 
@@ -1145,6 +1238,21 @@
 		      (car archive)))))
   (package-read-all-archive-contents))
 
+(declare-function epg-check-configuration "epg-config"
+		  (config &optional minimum-version))
+(declare-function epg-configuration "epg-config" ())
+(declare-function epg-import-keys-from-file "epg" (context keys))
+
+(defun package--import-default-keyring ()
+  (let* ((context (epg-make-context 'OpenPGP))
+	 (homedir (expand-file-name "gnupg" package-user-dir))
+	 (default-keyring (expand-file-name "package-keyring.gpg"
+					    data-directory)))
+    (when (file-exists-p default-keyring)
+      (make-directory homedir t)
+      (epg-context-set-home-directory context homedir)
+      (epg-import-keys-from-file context default-keyring))))
+
 ;;;###autoload
 (defun package-initialize (&optional no-activate)
   "Load Emacs Lisp packages, and activate them.
@@ -1157,6 +1265,11 @@
   (unless no-activate
     (dolist (elt package-alist)
       (package-activate (car elt))))
+  (condition-case-unless-debug nil
+      (progn
+	(epg-check-configuration (epg-configuration))
+	(package--import-default-keyring))
+    (error (message "Cannot import default keyring")))
   (setq package--initialized t))
 
 \f
@@ -1209,7 +1322,8 @@
          (homepage (if desc (cdr (assoc :url (package-desc-extras desc)))))
          (built-in (eq pkg-dir 'builtin))
          (installable (and archive (not built-in)))
-         (status (if desc (package-desc-status desc) "orphan")))
+         (status (if desc (package-desc-status desc) "orphan"))
+         (signed (if desc (package-desc-signed desc))))
     (prin1 name)
     (princ " is ")
     (princ (if (memq (aref status 0) '(?a ?e ?i ?o ?u)) "an " "a "))
@@ -1233,9 +1347,11 @@
                     (not (package-built-in-p name version)))
 	       (insert "',\n             shadowing a "
 		       (propertize "built-in package"
-				   'font-lock-face 'font-lock-builtin-face)
-		       ".")
-	     (insert "'.")))
+				   'font-lock-face 'font-lock-builtin-face))
+	     (insert "'"))
+	   (if signed
+	       (insert ".")
+	     (insert " (unsigned).")))
 	  (installable
            (insert (capitalize status))
 	   (insert " from " (format "%s" archive))
@@ -1449,7 +1565,8 @@
          (dir (package-desc-dir pkg-desc))
          (lle (assq name package-load-list))
          (held (cadr lle))
-         (version (package-desc-version pkg-desc)))
+         (version (package-desc-version pkg-desc))
+         (signed (package-desc-signed pkg-desc)))
     (cond
      ((eq dir 'builtin) "built-in")
      ((and lle (null held)) "disabled")
@@ -1463,7 +1580,9 @@
      (dir                               ;One of the installed packages.
       (cond
        ((not (file-exists-p (package-desc-dir pkg-desc))) "deleted")
-       ((eq pkg-desc (cadr (assq name package-alist))) "installed")
+       ((eq pkg-desc (cadr (assq name package-alist))) (if signed
+							   "installed"
+							 "unsigned"))
        (t "obsolete")))
      (t
       (let* ((ins (cadr (assq name package-alist)))
@@ -1473,7 +1592,9 @@
           (if (memq name package-menu--new-package-list)
               "new" "available"))
          ((version-list-< version ins-v) "obsolete")
-         ((version-list-= version ins-v) "installed")))))))
+         ((version-list-= version ins-v) (if signed
+					     "installed"
+					   "unsigned"))))))))
 
 (defun package-menu--refresh (&optional packages)
   "Re-populate the `tabulated-list-entries'.
@@ -1532,6 +1653,7 @@
 		(`"held"      'font-lock-constant-face)
 		(`"disabled"  'font-lock-warning-face)
 		(`"installed" 'font-lock-comment-face)
+		(`"unsigned"  'font-lock-warning-face)
 		(_            'font-lock-warning-face)))) ; obsolete.
     (list pkg-desc
 	  (vector (list (symbol-name (package-desc-name pkg-desc))
@@ -1570,7 +1692,7 @@
 (defun package-menu-mark-delete (&optional _num)
   "Mark a package for deletion and move to the next line."
   (interactive "p")
-  (if (member (package-menu-get-status) '("installed" "obsolete"))
+  (if (member (package-menu-get-status) '("installed" "obsolete" "unsigned"))
       (tabulated-list-put-tag "D" t)
     (forward-line)))
 
@@ -1738,6 +1860,8 @@
 	  ((string= sB "available") nil)
 	  ((string= sA "installed") t)
 	  ((string= sB "installed") nil)
+	  ((string= sA "unsigned") t)
+	  ((string= sB "unsigned") nil)
 	  ((string= sA "held") t)
 	  ((string= sB "held") nil)
 	  ((string= sA "built-in") t)


[-- Attachment #3: Type: text/plain, Size: 25 bytes --]


Regards,
-- 
Daiki Ueno

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

[Ted Zlatanov]

This looks terrific, but please make signing an option per archive, not
per package or global.  The GNU ELPA will be signed; others may not.

Ted

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

[Ted Zlatanov]

On Mon, 30 Sep 2013 15:48:16 -0400 Daiki Ueno <ueno@gnu.org> wrote: 

DU> Well, I still don't understand why this is advertised as such a
DU> difficult problem, particularly why package.el would need sign operation
DU> with Emacs.  Am I missing something?

Yes, I think so.  Checking package signatures in general was mostly
resolved back in June 2013, I simply didn't have time to work on it
until just now.  When I wanted to play with it over the weekend, the
GnuPG 2.0.20 behavior annoyed me enough that I complained about it and
am planning to expose the libnettle functions ASAP so we don't have to
depend on GnuPG.

The difficult part has been specifying the desired behavior, not
implementing it.

Perhaps you can look at
http://thread.gmane.org/gmane.emacs.devel/155400/focus=160631 and look
at my patch there and the surrounding discussion for background.  Stefan
participated and advised me on most of the desired features.

DU> Perhaps it might make sense to discuss with some code.  Here it is.

DU> The code verifies a detached signature NAME-VERSION.tar.sig with a
DU> trusted keyring located under ~/.emacs.d/elpa/gnupg/.  That's it.

The signed/unsigned status needs to be shown in the package listing.
Some archives are signed, some aren't.  Any file from an archive, not
just a package tarball, should be signed (especially the package index).

The management of the special gnupg keychain needs to be abstracted.
Signatures should be generated from inside Emacs.

In addition I started on the EPG interaction you've finished, so you can
probably start with my patch and fix the EPG-related pieces and any
other issues instead of writing your own.

DU> For uploading packages, we could simply use the same mechanism as
DU> gnupload in Gnulib.

DU> It's actually a 10-minute work at an airport lobby and tested only with
DU> the local package archive.

Your help is very welcome.

Ted

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

[Stefan Monnier]

DU> Well, I still don't understand why this is advertised as such a
DU> difficult problem, particularly why package.el would need sign operation
DU> with Emacs.  Am I missing something?
> Yes, I think so.  Checking package signatures in general was mostly
> resolved back in June 2013, I simply didn't have time to work on it
> until just now.

But the feature provided with the simple code of Daiki would already be
very useful.  I know you had similar code working back then.  Why not
install that already?


        Stefan

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

[Ted Zlatanov]

On Mon, 30 Sep 2013 18:56:18 -0400 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

DU> Well, I still don't understand why this is advertised as such a
DU> difficult problem, particularly why package.el would need sign operation
DU> with Emacs.  Am I missing something?
>> Yes, I think so.  Checking package signatures in general was mostly
>> resolved back in June 2013, I simply didn't have time to work on it
>> until just now.

SM> But the feature provided with the simple code of Daiki would already be
SM> very useful.  I know you had similar code working back then.  Why not
SM> install that already?

Daiki Ueno has moved on with his v2 patch and it provides most of the
desired functionality.  It still lacks two things I consider important:
per-archive signing and the verification of any downloaded file in a
signed archive.  Please let us know if you consider them important too
and I'll collaborate with him to implement them.

Meanwhile I've got all the Nettle hash functions and the ECB modes for
the ciphers working, and am making progress on exposing the CBC and CTR
ciphers, exposing RSA and DSA and ECC crypto, and writing ERT tests for
the whole.  I hope to have that work ready for review soon.

Thanks
Ted

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

[Daiki Ueno]

Ted Zlatanov <tzz@lifelogs.com> writes:

> Perhaps you can look at
> http://thread.gmane.org/gmane.emacs.devel/155400/focus=160631 and look
> at my patch there and the surrounding discussion for background.  Stefan
> participated and advised me on most of the desired features.
>
> DU> Perhaps it might make sense to discuss with some code.  Here it is.
>
> DU> The code verifies a detached signature NAME-VERSION.tar.sig with a
> DU> trusted keyring located under ~/.emacs.d/elpa/gnupg/.  That's it.
>
> The signed/unsigned status needs to be shown in the package listing.
> Some archives are signed, some aren't.  Any file from an archive, not
> just a package tarball, should be signed (especially the package index).

Done in my latest patch.

> The management of the special gnupg keychain needs to be abstracted.
> Signatures should be generated from inside Emacs.

I've read the discussion and patches, but it's still unclear to me.
Your latest(?) patch (package-archive-signed-3.patch) has
package--create-detached-signature, but nobody calls it.  For what
purpose would you need signature generation?

Perhaps you wanted to sign locally to toggle "unsigned" status to
"signed" status?  Then why it's not sufficient to just mark the package
as "unsigned" and ask package creaters to sign and upload?

Or, perhaps you wanted to develop a user interface to upload tarballs
with signature?  Then it should be go into package-x.el instead of
package.el, I suppose.

Anyway, I'm a bit surprised that there are few researches of existing
packaging systems which already utilize GPG signature, such as Debian
and Fedora.  AFAIK, those systems do not require signing operation in
their installer UI.

> In addition I started on the EPG interaction you've finished, so you can
> probably start with my patch and fix the EPG-related pieces and any
> other issues instead of writing your own.

I'm sorry, I couldn't find anything I can reuse in your patch.  It even
succeeds signature verification when GPG reports bad signatures.  Also,
why did you choose ".gpgsig" extension rather than ".sig", which has
already been used on ftp.gnu.org for a decade?  And I think it's too
much to modify package--with-work-buffer to check signatures of all
files downloaded.

Regards,
-- 
Daiki Ueno

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

[Ted Zlatanov]

On Wed, 02 Oct 2013 16:16:04 +0900 Daiki Ueno <ueno@gnu.org> wrote: 

DU> I've read the discussion and patches, but it's still unclear to me.
DU> Your latest(?) patch (package-archive-signed-3.patch) has
DU> package--create-detached-signature, but nobody calls it.  For what
DU> purpose would you need signature generation?

So the maintainer can create a signature from Emacs instead of
externally.  The signer is intended to be a maintainer after review, not
a package creator.

DU> Or, perhaps you wanted to develop a user interface to upload tarballs
DU> with signature?  Then it should be go into package-x.el instead of
DU> package.el, I suppose.

It's something you would run on the ELPA server, not at upload time.

I thought it belonged nicely in package.el.

DU> Anyway, I'm a bit surprised that there are few researches of existing
DU> packaging systems which already utilize GPG signature, such as Debian
DU> and Fedora.  AFAIK, those systems do not require signing operation in
DU> their installer UI.

package.el is not just an installer UI, it's a full package manager.

>> In addition I started on the EPG interaction you've finished, so you can
>> probably start with my patch and fix the EPG-related pieces and any
>> other issues instead of writing your own.

DU> I'm sorry, I couldn't find anything I can reuse in your patch.  It even
DU> succeeds signature verification when GPG reports bad signatures.

That's one of the EPG-related pieces I mentioned need fixing.  But at
this point your v2 patch has done the work so there's no point in arguing.

DU> Also, why did you choose ".gpgsig" extension rather than ".sig",
DU> which has already been used on ftp.gnu.org for a decade?

I think the extension name is not that important, but here specifically
I wanted to indicate it's generated by GPG.  .sig will obviously work
exactly the same way.

DU> And I think it's too much to modify package--with-work-buffer to
DU> check signatures of all files downloaded.

I disagree, but please implement what you believe will do the work of
checking the signatures for packages (tarballs and individual) and the
package index.  That's the goal; the implementation details don't
matter too much.

Ted

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

[Daiki Ueno]

Ted Zlatanov <tzz@lifelogs.com> writes:

> DU> For what purpose would you need signature generation?
>
> So the maintainer can create a signature from Emacs instead of
> externally.  The signer is intended to be a maintainer after review, not
> a package creator.

I'm fine with signing with dput for Debian and gnupload for GNU, who
else of you really wants that feature.  Reference?

> It's something you would run on the ELPA server, not at upload time.

I'd rather use other scripting language to do such a batch job.

> package.el is not just an installer UI, it's a full package manager.

Why the uploading part is separated into package-x.el then?

> DU> I'm sorry, I couldn't find anything I can reuse in your patch.  It even
> DU> succeeds signature verification when GPG reports bad signatures.
>
> That's one of the EPG-related pieces I mentioned need fixing.  But at
> this point your v2 patch has done the work so there's no point in arguing.

Thanks for understanding.  I should have been involved in this earlier.
What I'm really surprised is no progress on this for almost one year.

> DU> Also, why did you choose ".gpgsig" extension rather than ".sig",
> DU> which has already been used on ftp.gnu.org for a decade?
>
> I think the extension name is not that important, but here specifically
> I wanted to indicate it's generated by GPG.  .sig will obviously work
> exactly the same way.

It's important, if we would like to use common tools like gnupload too.

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

[Ted Zlatanov]

On Wed, 02 Oct 2013 21:22:53 +0900 Daiki Ueno <ueno@gnu.org> wrote: 

DU> Ted Zlatanov <tzz@lifelogs.com> writes:
DU> For what purpose would you need signature generation?
>> 
>> So the maintainer can create a signature from Emacs instead of
>> externally.  The signer is intended to be a maintainer after review, not
>> a package creator.

DU> I'm fine with signing with dput for Debian and gnupload for GNU, who
DU> else of you really wants that feature.  Reference?

I want it.

If we move to a branch-pull request-merge model, this will be much less
important since the signing will happen at the time of the merge on the
server; the reviewer never needs to manually sign anything.  But at
least for now we need interactive tools to automate that process and
gnupload would certainly fill that need.  So please don't dwell on this.

>> It's something you would run on the ELPA server, not at upload time.

DU> I'd rather use other scripting language to do such a batch job.

OK, I think there's room for both views.  Let's assume I will implement
it if I need it, and it shouldn't stop you.  Note I didn't mention it in
my "wishlist" for your v2 patch, so I don't consider it essential like
per-archive signing.

>> package.el is not just an installer UI, it's a full package manager.

DU> Why the uploading part is separated into package-x.el then?

Good point, I think you're right.  Thanks for the digging.  If I add
signing from Emacs I'll put it in package-x.el.

DU> I'm sorry, I couldn't find anything I can reuse in your patch.  It even
DU> succeeds signature verification when GPG reports bad signatures.
>> 
>> That's one of the EPG-related pieces I mentioned need fixing.  But at
>> this point your v2 patch has done the work so there's no point in arguing.

DU> Thanks for understanding.  I should have been involved in this earlier.
DU> What I'm really surprised is no progress on this for almost one
DU> year.

Yes, I know.  I was part of the problem: extremely busy with work and
"almost done" all the time.  Let's make an effort together and get it
done now.  I think it's an important part of Emacs' future.

DU> Also, why did you choose ".gpgsig" extension rather than ".sig",
DU> which has already been used on ftp.gnu.org for a decade?
>> 
>> I think the extension name is not that important, but here specifically
>> I wanted to indicate it's generated by GPG.  .sig will obviously work
>> exactly the same way.

DU> It's important, if we would like to use common tools like gnupload too.

OK with me, please consider me in favor of .sig.

Ted

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

[Stefan Monnier]

DU> I'm fine with signing with dput for Debian and gnupload for GNU, who
DU> else of you really wants that feature.  Reference?

I don't want that feature in package.el where it does not belong.
If someone wants it in package-x.el, that's fine, of course.

> If we move to a branch-pull request-merge model, this will be much less
> important since the signing will happen at the time of the merge on the
> server; the reviewer never needs to manually sign anything.  But at
> least for now we need interactive tools to automate that process and
> gnupload would certainly fill that need.  So please don't dwell on this.

Much too hypothetical for me.  If/when we start supporting signatures
provided by the author, we'll try and figure out how that can work
(which is not obvious at all, since the signature should be applied to
the tarball, but the tarball is generated later on by a batch process).


        Stefan

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

[Thien-Thi Nguyen]

[-- Attachment #1.1: Type: text/plain, Size: 223 bytes --]

() Daiki Ueno <ueno@gnu.org>
() Wed, 02 Oct 2013 16:16:04 +0900

   Or, perhaps you wanted to develop a user interface to upload tarballs
   with signature?

For this, i wrote gnupload.el a few months back, attached here:


[-- Attachment #1.2: gnupload.el --]
[-- Type: application/emacs-lisp, Size: 6186 bytes --]

[-- Attachment #1.3: Type: text/plain, Size: 424 bytes --]


and have used it personally for several GNU packages i maintain (seems
to work fine, so far).  Posted as FYI-FWIW-HTH-HAND, but critique more
than welcome.  Thanks for reminding me to share!

-- 
Thien-Thi Nguyen
   GPG key: 4C807502
   (if you're human and you know it)
      read my lisp: (responsep (questions 'technical)
                               (not (via 'mailing-list)))
                     => nil

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Stefan Monnier]

>    Or, perhaps you wanted to develop a user interface to upload tarballs
>    with signature?
> For this, i wrote gnupload.el a few months back, attached here:

GNU ELPA packages are auto-generated from the `elpa' git repository, so
there's no "upload".  The signing should take place in
elpa/admin/update-archive.sh.


        Stefan

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

[Stefan Monnier]

> Done in my latest patch.

Please install it.


        Stefan

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

[Daiki Ueno]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Please install it.

Pushed.

Ted Zlatanov <tzz@lifelogs.com> writes:

> This looks terrific, but please make signing an option per archive, not
> per package or global.  The GNU ELPA will be signed; others may not.

Ported package-unsigned-archives from your patch.  Also, added a check
of archive-contents signature.  Thanks for the suggestions.

Regards,
-- 
Daiki Ueno

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

[Ted Zlatanov]

On Thu, 03 Oct 2013 16:18:46 +0900 Daiki Ueno <ueno@gnu.org> wrote: 

DU> Ted Zlatanov <tzz@lifelogs.com> writes:

>> This looks terrific, but please make signing an option per archive, not
>> per package or global.  The GNU ELPA will be signed; others may not.

DU> Ported package-unsigned-archives from your patch.  Also, added a check
DU> of archive-contents signature.  Thanks for the suggestions.

Wonderful.  Needs documentation, though... especially the new defcustoms.

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?

The tests look great, too.

Ted

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

[Stefan Monnier]

> +(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?

Actually, let's wait.  If all turn out well, most/all ELPA archives will
start providing signatures in the not too distant future and there'll be
no need for per-archive settings (and we can change the default to t).


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Thu, 03 Oct 2013 11:01:43 -0400
> 
> > +(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?
> 
> Actually, let's wait.  If all turn out well, most/all ELPA archives will
> start providing signatures in the not too distant future and there'll be
> no need for per-archive settings (and we can change the default to t).

Are you saying that verification will not need gpg be installed?

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

[Ted Zlatanov]

On Fri, 04 Oct 2013 22:23:06 +0300 Eli Zaretskii <eliz@gnu.org> wrote: 

>> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
>> Date: Thu, 03 Oct 2013 11:01:43 -0400
>> 
>> > +(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?
>> 
>> Actually, let's wait.  If all turn out well, most/all ELPA archives will
>> start providing signatures in the not too distant future and there'll be
>> no need for per-archive settings (and we can change the default to t).

EZ> Are you saying that verification will not need gpg be installed?

If my work with libnettle progresses well, I think we'll be able to at
least verify GPG signatures without calling out to GnuPG or other tools
on all the platforms that have libnettle+libhogweed (any platforms with
GnuTLS support AFAIK).

I can put up my current patch for review but I still have HMAC, maybe
UMAC, and RSA+DSA+ECC crypto to finish.  The hashing methods and the
ciphers in ECB, CBC, and CTR modes are done with tests.  Should I make a
Bazaar branch for that work?  Is anyone interested in reviewing it?

Ted

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

[Daiki Ueno]

Ted Zlatanov <tzz@lifelogs.com> writes:

> I can put up my current patch for review but I still have HMAC, maybe
> UMAC, and RSA+DSA+ECC crypto to finish.  The hashing methods and the
> ciphers in ECB, CBC, and CTR modes are done with tests.  Should I make a
> Bazaar branch for that work?  Is anyone interested in reviewing it?

Probably I should shut up, but...

Does that mean all the package signatures will be signed/verified with
your own "Emacs internal" signature format, and all the packagers will
need to use your tool and Emacs, instead of GPG, right?

That is what I opposed again and again and suggested to use a standard
format.

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

[Stephen J. Turnbull]

Daiki Ueno writes:
 > Ted Zlatanov <tzz@lifelogs.com> writes:
 > 
 > > I can put up my current patch for review but I still have HMAC, maybe
 > > UMAC, and RSA+DSA+ECC crypto to finish.  The hashing methods and the
 > > ciphers in ECB, CBC, and CTR modes are done with tests.  Should I make a
 > > Bazaar branch for that work?  Is anyone interested in reviewing it?
 > 
 > Probably I should shut up, but...

Please don't.  You seem to be the only sane voice[1] in the crowd.
Not that I agree 100% with everything you've written, but at least you
have the security mindset.  Everybody else seems to think this is like
fixing any other bug.

 > Does that mean all the package signatures will be signed/verified with
 > your own "Emacs internal" signature format, and all the packagers will
 > need to use your tool and Emacs, instead of GPG, right?

He has suggested that, but AFAIK he doesn't insist on it.

Still, the whole idea worries me; there's no reason to suppose it will
increase security, and Ted never has seemed to grasp that security is
not a SMOP, nor that security is inherently inconvenient.  Quis
custodiat ipsos custodes?  Do you really want to put a possible fox in
charge of the security check at the henhouse door?

 > That is what I opposed again and again and suggested to use a standard
 > format.

+1


Footnotes: 
[1]  I don't understand security well enough to claim to be a sane
voice, but at least I know how little I know.

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

[Ted Zlatanov]

On Sat, 05 Oct 2013 14:40:46 +0900 "Stephen J. Turnbull" <stephen@xemacs.org> wrote: 

SJT> Daiki Ueno writes:
>> Ted Zlatanov <tzz@lifelogs.com> writes:
>> 
>> > I can put up my current patch for review but I still have HMAC, maybe
>> > UMAC, and RSA+DSA+ECC crypto to finish.  The hashing methods and the
>> > ciphers in ECB, CBC, and CTR modes are done with tests.  Should I make a
>> > Bazaar branch for that work?  Is anyone interested in reviewing it?
...
SJT> Still, the whole idea worries me; there's no reason to suppose it will
SJT> increase security, and Ted never has seemed to grasp that security is
SJT> not a SMOP, nor that security is inherently inconvenient.

I find that mildly offensive.

SJT> Quis custodiat ipsos custodes?  Do you really want to put a
SJT> possible fox in charge of the security check at the henhouse door?

My work will be open for review before it goes in.  I suggest you
criticize the work, not me or my motivations.

Ted

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

[Stephen J. Turnbull]

Ted Zlatanov writes:
 > On Sat, 05 Oct 2013 14:40:46 +0900 "Stephen J. Turnbull" <stephen@xemacs.org> wrote: 

 > SJT> Still, the whole idea worries me; there's no reason to suppose
 > SJT> it will increase security, and Ted never has seemed to grasp
 > SJT> that security is not a SMOP, nor that security is inherently
 > SJT> inconvenient.
 > 
 > I find that mildly offensive.

I'm sorry you do.  It's not something that can be phrased without
risking offense, though.

 > My work will be open for review before it goes in.  I suggest you
 > criticize the work, not me

You miss the point (and trimmed the relevant footnote, to boot).  I am
not qualified to criticize the C-level programming, because I assume
there are some subtle issues in handling crypto beyond using strncat
instead of strcat.

Therefore, I'm in a position where I have to delegate the work to you.
That being so, and being unable to verify that you can dive like a
duck, I'd at least like to hear you quack like one.

 > or my motivations.

Where did I criticize your motivations?  I'd like to correct my words
if I gave that impression.  I certainly didn't intend that.

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

[Ted Zlatanov]

On Sun, 06 Oct 2013 00:07:53 +0900 "Stephen J. Turnbull" <stephen@xemacs.org> wrote: 

SJT> That being so, and being unable to verify that you can dive like a
SJT> duck, I'd at least like to hear you quack like one.

Quack.

Ted

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

[Ted Zlatanov]

On Sat, 05 Oct 2013 09:34:52 +0900 Daiki Ueno <ueno@gnu.org> wrote: 

DU> Ted Zlatanov <tzz@lifelogs.com> writes:
>> I can put up my current patch for review but I still have HMAC, maybe
>> UMAC, and RSA+DSA+ECC crypto to finish.  The hashing methods and the
>> ciphers in ECB, CBC, and CTR modes are done with tests.  Should I make a
>> Bazaar branch for that work?  Is anyone interested in reviewing it?

DU> Probably I should shut up, but...

DU> Does that mean all the package signatures will be signed/verified with
DU> your own "Emacs internal" signature format, and all the packagers will
DU> need to use your tool and Emacs, instead of GPG, right?

No.  After I have the basic needs done, I will start on an OpenPGP
protocol emulation.

DU> That is what I opposed again and again and suggested to use a standard
DU> format.

Well, OpenPGP is fairly standard, although there are many versions.
I'll do my best to be GnuPG-compatible, but my primary goal will be
OpenPGP compatibility as in RFC 4880.

Ted

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

[Eli Zaretskii]

> From: Ted Zlatanov <tzz@lifelogs.com>
> Date: Fri, 04 Oct 2013 17:14:44 -0400
> 
> >> Actually, let's wait.  If all turn out well, most/all ELPA archives will
> >> start providing signatures in the not too distant future and there'll be
> >> no need for per-archive settings (and we can change the default to t).
> 
> EZ> Are you saying that verification will not need gpg be installed?
> 
> If my work with libnettle progresses well, I think we'll be able to at
> least verify GPG signatures without calling out to GnuPG or other tools
> on all the platforms that have libnettle+libhogweed (any platforms with
> GnuTLS support AFAIK).

And what about users whose Emacs doesn't have GnuTLS?  Are we saying
they will not be able to install packages from ELPA without being
annoyed by prompts and error messages?

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

[Ted Zlatanov]

On Sat, 05 Oct 2013 10:09:25 +0300 Eli Zaretskii <eliz@gnu.org> wrote: 

>> From: Ted Zlatanov <tzz@lifelogs.com>
>> Date: Fri, 04 Oct 2013 17:14:44 -0400
>> 
>> >> Actually, let's wait.  If all turn out well, most/all ELPA archives will
>> >> start providing signatures in the not too distant future and there'll be
>> >> no need for per-archive settings (and we can change the default to t).
>> 
EZ> Are you saying that verification will not need gpg be installed?
>> 
>> If my work with libnettle progresses well, I think we'll be able to at
>> least verify GPG signatures without calling out to GnuPG or other tools
>> on all the platforms that have libnettle+libhogweed (any platforms with
>> GnuTLS support AFAIK).

EZ> And what about users whose Emacs doesn't have GnuTLS?  Are we saying
EZ> they will not be able to install packages from ELPA without being
EZ> annoyed by prompts and error messages?

No one has said that AFAIK.

My suggestion was to give users the choice (per archive) to always,
maybe, or never verify packages.  Currently this choice is global in
Daiki Ueno's changes that were committed recently, but it's still a
choice.

The signature verification method will be anything that can take a .sig
file and check that it's been signed by a maintainer's key, according to
the OpenPGP protocol in RFC 4880.  Daiki Ueno has implemented a
epg.el-backed verification method using calls to the external GnuPG that
works today.  I will (perhaps with others' help) try to implement a
libnettle+libhogweed-backed verification method that only requires those
libraries and makes no external calls.  I expect the user will be able
to decide which one to use, or even both if they wish.

Ted

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

[Eli Zaretskii]

> From: Ted Zlatanov <tzz@lifelogs.com>
> Date: Sat, 05 Oct 2013 06:11:51 -0400
> 
> On Sat, 05 Oct 2013 10:09:25 +0300 Eli Zaretskii <eliz@gnu.org> wrote: 
> 
> >> From: Ted Zlatanov <tzz@lifelogs.com>
> >> Date: Fri, 04 Oct 2013 17:14:44 -0400
> >> 
> >> >> Actually, let's wait.  If all turn out well, most/all ELPA archives will
> >> >> start providing signatures in the not too distant future and there'll be
> >> >> no need for per-archive settings (and we can change the default to t).
> >> 
> EZ> Are you saying that verification will not need gpg be installed?
> >> 
> >> If my work with libnettle progresses well, I think we'll be able to at
> >> least verify GPG signatures without calling out to GnuPG or other tools
> >> on all the platforms that have libnettle+libhogweed (any platforms with
> >> GnuTLS support AFAIK).
> 
> EZ> And what about users whose Emacs doesn't have GnuTLS?  Are we saying
> EZ> they will not be able to install packages from ELPA without being
> EZ> annoyed by prompts and error messages?
> 
> No one has said that AFAIK.

Stefan did, see above.

> My suggestion was to give users the choice (per archive) to always,
> maybe, or never verify packages.  Currently this choice is global in
> Daiki Ueno's changes that were committed recently, but it's still a
> choice.

Daiki's default is not t.

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

[Stefan Monnier]

>> >> >> Actually, let's wait.  If all turn out well, most/all ELPA
>> >> >> archives will start providing signatures in the not too distant
>> >> >> future and there'll be no need for per-archive settings (and we
>> >> >> can change the default to t).
[...]
>> No one has said that AFAIK.
> Stefan did, see above.

No, I was talking about "per archive settings".  I.e. the fact that we
probably won't need to worry about whether an archive provides
signatures.  Of course, we'll still need to worry about whether or not
the user's install has the tools needed to check signatures.

Of course we'll probably want to output a warning message if we can't check
signatures for lack of local tools, so the user is aware of that fact.
But there should be no prompts nor error messages.


        Stefan

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

[Daiki Ueno]

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

Ted Zlatanov <tzz@lifelogs.com> 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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: package-allow-unsigned.patch --]
[-- Type: text/x-patch, Size: 6060 bytes --]

=== 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


[-- Attachment #3: Type: text/plain, Size: 26 bytes --]


Regards,
-- 
Daiki Ueno

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

[Ted Zlatanov]

On Fri, 04 Oct 2013 11:46:30 +0900 Daiki Ueno <ueno@gnu.org> wrote: 

DU> Ted Zlatanov <tzz@lifelogs.com> 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?

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

DU> * remove the package-check-signature option, and

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

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

I think it's a good direction.  Maybe archives should have trust levels
that the user can provide when adding them, instead of managing
`package-{signed,unsigned}-archives' as external lists:

- signed (always check for .sig and verify it)
- optionally signed (always check for .sig but allow it may not exist)
- not signed (never check for .sig, avoiding extra network requests)

The default trust level would be "signed."  Does that work?

The user may also want a keyring per archive, if that could be a
property.  I would want it.  But it may be expensive to implement.

Ted