Re: Signed archives (preliminary patch)

[ludo@gnu.org (Ludovic Courtès)]

(Could you keep more context when replying, to make it easier to find
out what we’re referring to?)

Nikita Karetnikov <nikita@karetnikov.org> skribis:

>> For simplicity, change this pattern to ("1" id body).  That will allow
>> the inner ‘cond’ 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’m planning to send a proper patch as soon as I test (guix base64) 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 “canonical sexp” as a comment on the right.

> +  ;; The original contents of a narinfo file.  This field is needed because we
> +  ;; want to preserve the initial order of fields for verification purposes.
> +  ;; See <https://lists.gnu.org/archive/html/guix-devel/2014-02/msg00340.html>
> +  ;; 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’s ‘Signature’ field."

> +(define* (verify-signature sig #:optional (acl (current-acl)))

I really prefer something like ‘assert-valid-signature’ (possibly
copy/pasted from nar.scm) because:

  1. The name reflects that it’s a check whose failure leads to a
     non-local exit, and that the return value doesn’t matter.

  2. ‘assert-valid-signature’ in nar.scm does all the checks, including
     the hash comparison, which IMO makes it easier to see that we’re
     not forgetting anything.

WDYT?

(In the light of Apple’s “goto fail” story, it makes 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’d 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 catch
> +;; specific exceptions.

“allows us”

> +(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 ‘test-begin’ per file, and it should
be (test-begin "file-name-without-extension").  That’s 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 “(lambda () ...)” 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’m asking because I got lost as to why %SIGNATURE here
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-narinfo)
> +                                        "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’.