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: Tue, 04 Mar 2014 22:59:31 +0100 Message-ID: <87bnxl62ws.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> <874n3kp46f.fsf@gnu.org> <87lhwqsxjr.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]:46898) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKxNE-0006L6-MJ for guix-devel@gnu.org; Tue, 04 Mar 2014 16:59:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKxN9-00072C-4H for guix-devel@gnu.org; Tue, 04 Mar 2014 16:59:40 -0500 Received: from hera.aquilenet.fr ([2a01:474::1]:43833) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKxN8-00071G-N5 for guix-devel@gnu.org; Tue, 04 Mar 2014 16:59:35 -0500 In-Reply-To: <87lhwqsxjr.fsf@karetnikov.org> (Nikita Karetnikov's message of "Tue, 04 Mar 2014 02:54:32 +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 (Could you keep more context when replying, to make it easier to find out what we=E2=80=99re referring to?) Nikita Karetnikov skribis: >> For simplicity, change this pattern to ("1" id body). That will allow >> the inner =E2=80=98cond=E2=80=99 to be simplified. > > I like informative error messages, may I keep it please? The attached > diff should address all the things you mentioned except this one. > Please review. OK. > I=E2=80=99m planning to send a proper patch as soon as I test (guix base6= 4) and > change a couple of things in (test-substitute-binary). Cool, thanks! > -(define (narinfo-maker cache-url) > - "Return a narinfo constructor for narinfos originating from CACHE-URL." > + (system narinfo-system) > + (signature narinfo-signature) Add =E2=80=9Ccanonical sexp=E2=80=9D as a comment on the right. > + ;; The original contents of a narinfo file. This field is needed beca= use we > + ;; want to preserve the initial order of fields for verification purpo= ses. > + ;; See > + ;; for more information. > + (contents narinfo-contents)) s/initial order of fields/exact textual representation/ > +(define (parse-signature str) > + "Parse the Signature field of a narinfo file." Rather something like: "Return the as a canonical sexp the signature read from STR, the value of a narinfo=E2=80=99s =E2=80=98Signature=E2=80=99 field." > +(define* (verify-signature sig #:optional (acl (current-acl))) I really prefer something like =E2=80=98assert-valid-signature=E2=80=99 (po= ssibly copy/pasted from nar.scm) because: 1. The name reflects that it=E2=80=99s a check whose failure leads to a non-local exit, and that the return value doesn=E2=80=99t matter. 2. =E2=80=98assert-valid-signature=E2=80=99 in nar.scm does all the check= s, including the hash comparison, which IMO makes it easier to see that we=E2=80=99= re not forgetting anything. WDYT? (In the light of Apple=E2=80=99s =E2=80=9Cgoto fail=E2=80=9D story, it make= s sense to pay extra attention to the way we write these things.) > (define (write-narinfo narinfo port) > "Write NARINFO to PORT." [...] > + (set-port-encoding! port "UTF-8") > + (put-string port (narinfo-contents narinfo))) I=E2=80=99d prefer: (put-bytevector port (string->utf8 (narinfo-contents narinfo))) > +(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. =E2=80=9Callows us=E2=80=9D > +(define %wrong-public-key > + ;; (display > + ;; (canonical-sexp->string > + ;; (find-sexp-token (generate-key "(genkey (rsa (nbits 4:1024)))") > + ;; 'public-key))) You can remove the comment here. > +(test-begin "parse-signature") Actually there should be only one =E2=80=98test-begin=E2=80=99 per file, an= d it should be (test-begin "file-name-without-extension"). That=E2=80=99s because that= is then used as the base of the .log file name. > +(test-assert "valid" > + (lambda () > + (parse-signature %signature))) This test will always pass because (lambda () ...) is true. Instead it should read: (test-assert "valid" (canonical-sexp? (parse-signature %signature))) For consistency, I would write test-error* like: (define-syntax-rule (test-error* name exp) (test-assert name (catch 'quit (lambda () exp #f) (lambda args #t)))) because then =E2=80=9C(lambda () ...)=E2=80=9D can be omitted. > +(test-error* "invalid hash" > + (lambda () > + (read-narinfo (open-input-string (narinfo %signature)) > + "https://example.com" %acl))) For these tests, could you add one-line comments specifying why they should fail? I=E2=80=99m asking because I got lost as to why %SIGNATURE he= re should have a hash mismatch. > +(test-assert "valid" > + (lambda () > + (read-narinfo (open-input-string %signed-narinfo) > + "https://example.com" %acl))) Same as above: remove (lambda () ...). > +(let ((port (open-output-string))) > + (test-equal "valid" > + %signed-narinfo > + (begin (write-narinfo (read-narinfo (open-input-string %signed-narin= fo) > + "https://example.com" %acl) > + port) > + (get-output-string port)))) Rather: (test-equal "valid" %signed-narinfo (call-with-output-string (lambda (port) ...))) Thank you for the great work! Ludo=E2=80=99.