From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Daiki Ueno Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] package.el: check tarball signature Date: Fri, 04 Oct 2013 11:46:30 +0900 Message-ID: References: <874n92x9em.fsf@flea.lifelogs.com> <87fvsk9m8b.fsf-ueno@gnu.org> <877gdutp1l.fsf@flea.lifelogs.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: ger.gmane.org 1380854799 23161 80.91.229.3 (4 Oct 2013 02:46:39 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 4 Oct 2013 02:46:39 +0000 (UTC) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Oct 04 04:46:44 2013 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1VRvPe-0008IB-Ft for ged-emacs-devel@m.gmane.org; Fri, 04 Oct 2013 04:46:42 +0200 Original-Received: from localhost ([::1]:46038 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VRvPe-0006tA-4k for ged-emacs-devel@m.gmane.org; Thu, 03 Oct 2013 22:46:42 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:56450) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VRvPZ-0006ot-G4 for emacs-devel@gnu.org; Thu, 03 Oct 2013 22:46:39 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VRvPY-0004KV-4G for emacs-devel@gnu.org; Thu, 03 Oct 2013 22:46:37 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:50086) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VRvPY-0004KN-1F for emacs-devel@gnu.org; Thu, 03 Oct 2013 22:46:36 -0400 Original-Received: from du-a.org ([2001:e41:db5e:fb14::1]:37934 helo=localhost.localdomain) by fencepost.gnu.org with esmtpsa (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1VRvPW-0004z7-FS for emacs-devel@gnu.org; Thu, 03 Oct 2013 22:46:35 -0400 In-Reply-To: <877gdutp1l.fsf@flea.lifelogs.com> (Ted Zlatanov's message of "Thu, 03 Oct 2013 10:19:34 -0400") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.devel:163839 Archived-At: --=-=-= Content-Type: text/plain Ted Zlatanov 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 --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=package-allow-unsigned.patch === 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 --=-=-= Content-Type: text/plain Regards, -- Daiki Ueno --=-=-=--