Re: Signed archives (preliminary patch)

[Ludovic Courtès]

Thanks for working on it.  Some preliminary comments:

Nikita Karetnikov <address@hidden> 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 <address@hidden> on
> +;; February 12, 2014.

Cool.

> +(define-record-type <signature>
> +  ;; See the last paragraph of this commit message:
> +  ;; 
> <https://github.com/NixOS/nix/commit/0fdf4da0e979f992db75cc17376e455ddc5a96d8>,
> +  ;; 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
‘version’ and ‘key-id’ items: the original version number doesn’t matter
as long as it’s one we know how to parse, and the ‘key-id’ is pretty
much useless.

The ‘key-id’ as envisioned in Hydra is useless notably because with our
SPKIsh approach, the ultimate identifier is the public key, which should
be embedded in BODY (like ‘signature-sexp’ does.)

So I think we can get rid of the <signature> 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’d be better to separate parsing and verification.

Also, that file generally reads “narinfo”, not “NAR info”, 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 ‘cond’ 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 anything
> +         ;; about it?  Should we use file locking or 'catch'?  I'm not sure.
> +         ;; We are already screwed if someone can alter files owned by root,
> +         ;; 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’s 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’s an important check missing here: the code verifies that BODY* is
a valid signature, but it doesn’t check whether what it signs
corresponds to this narinfo up to but excluding the ‘Signature’ field.

The check should look like ‘assert-valid-signature’ 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, ‘read-narinfo’ should probably:

  1. read everything from PORT with ‘get-string-all’ in a string (make
     sure PORT’s 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-references))
>                      ("Deriver" . ,(compose empty-string-if-false
>                                             narinfo-deriver))
> -                    ("System" . ,narinfo-system))
> +                    ("System" . ,narinfo-system)
> +                    ("Signature" . ,(lambda (narinfo)
> +                                      (let ((sig (narinfo-signature 
> narinfo)))
> +                                        (string-append
> +                                         (number->string (signature-version 
> sig))
> +                                         ";"
> +                                         (signature-key-id sig)
> +                                         ";"
> +                                         (base64-encode
> +                                          ;; XXX: Can we assume UTF-8 here?
> +                                          (string->utf8
> +                                           (canonical-sexp->string
> +                                            (signature-body sig)))))))))

‘write-narinfo’ is used in particular when writing narinfos to the local
cache.

It’s important to keep the original signatures intact.  Since the
narinfo format is “sloppy” (included non-significant characters, field
ordering isn’t 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 ‘write-narinfo’ currently does because we could
build a string that slightly differs from the initial one and thus
doesn’t pass the signature check.

To fix this, the <narinfo> record must include an additional field to
contain the original narinfo string.  Then ‘write-narinfo’ 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 catch
> +;; 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 ‘test-eq’, and VAL should be eq?-unique.

> +(define %keypair
> +  (generate-key 1024-bit-rsa))

Don’t generate a key pair in the test: it’s slow and may fail 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’.