From: Daiki Ueno <ueno@gnu.org>
To: emacs-devel@gnu.org
Subject: Re: [PATCH] package.el: check tarball signature
Date: Fri, 04 Oct 2013 11:46:30 +0900 [thread overview]
Message-ID: <m3siwhpxbt.fsf-ueno@gnu.org> (raw)
In-Reply-To: <877gdutp1l.fsf@flea.lifelogs.com> (Ted Zlatanov's message of "Thu, 03 Oct 2013 10:19:34 -0400")
[-- 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
next prev parent reply other threads:[~2013-10-04 2:46 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-30 19:48 [PATCH] package.el: check tarball signature Daiki Ueno
2013-09-30 19:58 ` Eli Zaretskii
2013-10-02 6:20 ` [PATCHv2] " Daiki Ueno
2013-10-02 10:43 ` Ted Zlatanov
2013-09-30 21:54 ` [PATCH] " Ted Zlatanov
2013-09-30 22:56 ` Stefan Monnier
2013-10-02 11:17 ` Ted Zlatanov
2013-10-02 7:16 ` Daiki Ueno
2013-10-02 10:41 ` Ted Zlatanov
2013-10-02 12:22 ` Daiki Ueno
2013-10-02 13:53 ` Ted Zlatanov
2013-10-03 3:51 ` Stefan Monnier
2013-10-02 13:15 ` Thien-Thi Nguyen
2013-10-03 3:45 ` Stefan Monnier
2013-10-03 3:52 ` Stefan Monnier
2013-10-03 7:18 ` Daiki Ueno
2013-10-03 14:19 ` Ted Zlatanov
2013-10-03 15:01 ` Stefan Monnier
2013-10-04 19:23 ` Eli Zaretskii
2013-10-04 21:14 ` Ted Zlatanov
2013-10-05 0:34 ` Daiki Ueno
2013-10-05 5:40 ` Stephen J. Turnbull
2013-10-05 10:03 ` Ted Zlatanov
2013-10-05 15:07 ` Stephen J. Turnbull
2013-10-05 21:51 ` Ted Zlatanov
2013-10-05 9:57 ` Ted Zlatanov
2013-10-05 7:09 ` Eli Zaretskii
2013-10-05 10:11 ` Ted Zlatanov
2013-10-05 12:37 ` Eli Zaretskii
2013-10-05 13:53 ` Stefan Monnier
2013-10-04 2:46 ` Daiki Ueno [this message]
2013-10-04 16:19 ` Ted Zlatanov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=m3siwhpxbt.fsf-ueno@gnu.org \
--to=ueno@gnu.org \
--cc=emacs-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.