From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: Signed archives (preliminary patch) Date: Thu, 27 Feb 2014 23:43:36 +0100 Message-ID: <874n3kp46f.fsf@gnu.org> References: <87txcqesqv.fsf@karetnikov.org> <87eh3ure1r.fsf@gnu.org> <87bnyyiv2u.fsf_-_@karetnikov.org> <87ha8qo7rl.fsf@gnu.org> <8761p5jv1g.fsf@karetnikov.org> <87r47tfmes.fsf@gnu.org> <8738k0pj8c.fsf@karetnikov.org> <874n4fnhs7.fsf@gnu.org> <87ppmigld8.fsf@karetnikov.org> <87y514dv2u.fsf@gnu.org> <87y50wffjy.fsf_-_@karetnikov.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:51389) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WJ9g7-0003c2-U1 for guix-devel@gnu.org; Thu, 27 Feb 2014 17:43:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WJ9g3-0005ys-84 for guix-devel@gnu.org; Thu, 27 Feb 2014 17:43:43 -0500 Received: from hera.aquilenet.fr ([2a01:474::1]:38135) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WJ9g2-0005xm-Qi for guix-devel@gnu.org; Thu, 27 Feb 2014 17:43:39 -0500 In-Reply-To: <87y50wffjy.fsf_-_@karetnikov.org> (Nikita Karetnikov's message of "Fri, 28 Feb 2014 00:48:01 +0400") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Nikita Karetnikov Cc: guix-devel@gnu.org Thanks for working on it. Some preliminary comments: Nikita Karetnikov skribis: > --- /dev/null > +++ b/guix/base64.scm > @@ -0,0 +1,212 @@ > +;; -*- mode: scheme; coding: utf-8 -*- > +;; > +;; This module was renamed from (weinholt text base64 (1 0 20100612)) to > +;; (guix base64) by Nikita Karetnikov on > +;; February 12, 2014. Cool. > +(define-record-type > + ;; See the last paragraph of this commit message: > + ;; , > + ;; but keep in mind that Guix uses Libgcrypt (which uses the canonical > + ;; S-expressions format) instead of OpenSSL. > + (%make-signature version key-id body) > + signature? > + (version signature-version) > + (key-id signature-key-id) > + ;; The base64-encoded signature of the SHA-256 hash of the contents of= the > + ;; NAR info file up to but not including the Signature line. > + (body signature-body)) Once the Signature field has been parsed, I think we can discard the =E2=80=98version=E2=80=99 and =E2=80=98key-id=E2=80=99 items: the original = version number doesn=E2=80=99t matter as long as it=E2=80=99s one we know how to parse, and the =E2=80=98key-id= =E2=80=99 is pretty much useless. The =E2=80=98key-id=E2=80=99 as envisioned in Hydra is useless notably beca= use with our SPKIsh approach, the ultimate identifier is the public key, which should be embedded in BODY (like =E2=80=98signature-sexp=E2=80=99 does.) So I think we can get rid of the record. > +;;; XXX: Is it reasonable to parse and verify at the same time? > +(define* (parse-signature str #:optional (acl (current-acl))) > + "Parse the Signature field of a NAR info file." Indeed I think it=E2=80=99d be better to separate parsing and verification. Also, that file generally reads =E2=80=9Cnarinfo=E2=80=9D, not =E2=80=9CNAR= info=E2=80=9D, so I think we should stick to that name. > + (let ((lst (string-split str #\;))) > + (match lst > + ((version id body) For simplicity, change this pattern to ("1" id body). That will allow the inner =E2=80=98cond=E2=80=99 to be simplified. > + (let* ((maybe-number (string->number version)) > + ;; XXX: Can we assume UTF-8 here? Probably not. Yes we can. Narinfos are ASCII in practice. > + (body* (string->canonical-sexp > + (utf8->string (base64-decode body)))) > + (key (signature-subject body*))) > + ;; XXX: All these checks are subject to TOCTOU, can we do anyth= ing > + ;; about it? Should we use file locking or 'catch'? I'm not s= ure. > + ;; We are already screwed if someone can alter files owned by r= oot, > + ;; aren't we? You mean if someone changes the ACL? Actually at this point the ACL file has already been loaded in memory, so if it changes that=E2=80=99s no problem. > + ((not (authorized-key? key acl)) > + (leave (_ "unauthorized public key: ~a~%") > + (canonical-sexp->string key))) > + ((not (valid-signature? body*)) > + (leave (_ "invalid signature: ~a~%") > + (canonical-sexp->string body*))) There=E2=80=99s an important check missing here: the code verifies that BOD= Y* is a valid signature, but it doesn=E2=80=99t check whether what it signs corresponds to this narinfo up to but excluding the =E2=80=98Signature=E2= =80=99 field. The check should look like =E2=80=98assert-valid-signature=E2=80=99 in nar.= scm: (if (authorized-key? subject) (if (equal? (hash-data->bytevector data) hash) (unless (valid-signature? signature) (raise (condition (&message (message "invalid signature")) (&nar-signature-error (file file) (signature signature) (port port))))) (Probably this should be factorized eventually.) The difficulty here will be to compute the hash up to the Signature field. To do that, =E2=80=98read-narinfo=E2=80=99 should probably: 1. read everything from PORT with =E2=80=98get-string-all=E2=80=99 in a s= tring (make sure PORT=E2=80=99s encoding is UTF-8); 2. isolate the lines before the ^[[:blank:]]*Signature[[:blank:]]: line; 3. compute the hash of those lines; 4. do (fields->alist (open-input-string the-whole-string)); 5. pass the hash to the signature verification procedure. Does that make sense? > (define (write-narinfo narinfo port) > "Write NARINFO to PORT." > @@ -254,7 +307,19 @@ reading PORT." > ("References" . ,(compose string-join narinfo-refere= nces)) > ("Deriver" . ,(compose empty-string-if-false > narinfo-deriver)) > - ("System" . ,narinfo-system)) > + ("System" . ,narinfo-system) > + ("Signature" . ,(lambda (narinfo) > + (let ((sig (narinfo-signature nari= nfo))) > + (string-append > + (number->string (signature-vers= ion sig)) > + ";" > + (signature-key-id sig) > + ";" > + (base64-encode > + ;; XXX: Can we assume UTF-8 he= re? > + (string->utf8 > + (canonical-sexp->string > + (signature-body sig))))))))) =E2=80=98write-narinfo=E2=80=99 is used in particular when writing narinfos= to the local cache. It=E2=80=99s important to keep the original signatures intact. Since the narinfo format is =E2=80=9Csloppy=E2=80=9D (included non-significant charac= ters, field ordering isn=E2=80=99t signficant, etc.), that means we must also keep the = exact narinfo as delivered by Hydra. We can no longer rebuild that string after the fact like =E2=80=98write-narinfo=E2=80=99 currently does because = we could build a string that slightly differs from the initial one and thus doesn=E2=80=99t pass the signature check. To fix this, the record must include an additional field to contain the original narinfo string. Then =E2=80=98write-narinfo=E2=80=99 = just needs to write it out as UTF-8. > +(define-module (test-substitute-binary) > + #:use-module (guix scripts substitute-binary) > + #:use-module (guix base64) > + #:use-module (guix hash) > + #:use-module (guix pk-crypto) > + #:use-module (guix pki) > + #:use-module (rnrs bytevectors) > + #:use-module ((srfi srfi-64) #:hide (test-error))) > + > +;; XXX: Replace with 'test-error' from SRFI-64 as soon as it allows to c= atch > +;; specific exceptions. > +(define (test-error name key thunk val) > + "Test whether THUNK throws a particular error KEY, e.g., 'misc-error, = by > +comparing the expected VAL and the one returned by the handler. This > +procedure assumes that THUNK itself will never return VAL, which is > +error-prone but better than catching everything with 'test-error' from > +SRFI-64." > + (test-equal name val > + (catch key > + thunk > + (const val)))) This should use =E2=80=98test-eq=E2=80=99, and VAL should be eq?-unique. > +(define %keypair > + (generate-key 1024-bit-rsa)) Don=E2=80=99t generate a key pair in the test: it=E2=80=99s slow and may fa= il due to insufficient entropy. Instead, keep the key pair inline (as in tests/pk-crypto.scm), or load it from signing-key.{pub,sec}. > +(define %signature-body > + ;; XXX: Can we assume UTF-8 here? Yes. > +(test-begin "parse-signature") > + > +(test-error* "not a number" > + (lambda () > + (parse-signature (signature "not-a-number") %acl))) > + > +(test-error* "wrong version number" > + (lambda () > + (parse-signature (signature "2") %acl))) > + > +(test-error* "unauthorized key" > + (lambda () > + (parse-signature (signature "1") (public-keys->acl '())))) > + > +(test-error* "invalid signature" > + (lambda () > + (parse-signature %wrong-signature > + (public-keys->acl (list %wrong-public-key))))) > + > +(test-assert "valid" > + (lambda () > + (parse-signature (signature "1") %acl))) > + > +(test-error* "invalid signature format" > + (lambda () > + (parse-signature "no signature here" %acl))) > + > +(test-end "parse-signature") OK. I think we need black-box tests in tests/store.scm (there are already a couple of substituter tests there.) WDYT? Looks like this is getting concrete, thanks! :-) Ludo=E2=80=99.