[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] package.el: check tarball signature
From: |
Daiki Ueno |
Subject: |
Re: [PATCH] package.el: check tarball signature |
Date: |
Fri, 04 Oct 2013 11:46:30 +0900 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3.50 (gnu/linux) |
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
- Re: [PATCH] package.el: check tarball signature, Daiki Ueno, 2013/10/02
- Re: [PATCH] package.el: check tarball signature, Thien-Thi Nguyen, 2013/10/02
- Re: [PATCH] package.el: check tarball signature, Stefan Monnier, 2013/10/02
- Re: [PATCH] package.el: check tarball signature, Daiki Ueno, 2013/10/03
- Re: [PATCH] package.el: check tarball signature, Stefan Monnier, 2013/10/04
- Re: [PATCH] package.el: check tarball signature, Eli Zaretskii, 2013/10/04
- Re: [PATCH] package.el: check tarball signature, Ted Zlatanov, 2013/10/04
- Re: [PATCH] package.el: check tarball signature, Daiki Ueno, 2013/10/04
- Re: [PATCH] package.el: check tarball signature, Stephen J. Turnbull, 2013/10/05
- Re: [PATCH] package.el: check tarball signature, Ted Zlatanov, 2013/10/05
- Re: [PATCH] package.el: check tarball signature, Stephen J. Turnbull, 2013/10/05
- Re: [PATCH] package.el: check tarball signature, Ted Zlatanov, 2013/10/05
- Re: [PATCH] package.el: check tarball signature, Ted Zlatanov, 2013/10/05