Re: Signed archives (preliminary patch)

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

Nikita Karetnikov <nikita@karetnikov.org> skribis:

> I think the current docstring of ‘assert-valid-signature’ is not correct
> since ‘signature’ must be a string (as produced by
> ‘canonical-sexp->string’), not an sexp.

In guix/nar.scm, the comment is:

  (define (assert-valid-signature signature hash file)
    ;; Bail out if SIGNATURE, an sexp, doesn't match HASH, a bytevector
    ;; containing the expected hash for FILE.

and indeed, SIGNATURE must be a string here.

> Similarly, the “signature is not a valid s-expression” and “corrupt
> signature data” messages are a bit confusing due to the way
> ‘string->canonical-sexp’ works (try ‘string->canonical-sexp "foo"’).
> But I may be wrong about the latter.

Ah right, you could get “corrupt signature data” when
(string->canonical-sexp signature) returns the null canonical sexp,
whereas you’d want “not a valid s-expression”.

Well, we can fix that in a separate patch if you want.

> +(define* (assert-valid-signature signature hash port
> +                                 #:optional (acl (current-acl)))
> +  ;; Bail out if SIGNATURE, a string, doesn't match HASH, a bytevector
> +  ;; containing the expected hash for PORT.

Make it a docstring.

Also, please make this change a separate patch.

> +  (let* ((file      (port-filename port))

I don’t think this will work, because most of the time PORT is a pipe
(an input port), whereas FILE is supposed to be the name of the file
being restored.

> +                (raise (condition (&message (message "invalid hash"))
> +                                  (&nar-invalid-hash-error
> +                                   (port port) (file file)
> +                                   (signature signature)
> +                                   (expected (hash-data->bytevector data))
> +                                   (actual hash)))))
> +            (raise (condition (&message (message "unauthorized public key"))
> +                              (&nar-signature-error
> +                               (signature signature) (file file) (port port)))))
> +        (raise (condition
> +                (&message (message "corrupt signature data"))
> +                (&nar-signature-error
> +                 (signature signature) (file file) (port port)))))))

Actually, the problem with making ‘assert-valid-signature’ public is
that it raises &nar error conditions.

It could be changed to raise a more generic &signature-error, but then
‘restore-file-set’ would have to guard against it to re-throw it along
with a &nar-error (making a compound condition.)  And then ui.scm would
figure it out.  Blech.

It’s worth factorizing, but I don’t see how to do it nicely.  Thoughts?

> +(define (parse-signature str)
> +  "Return the value of a narinfo's 'Signature' field as a canonical sexp."

I don’t remember if I said it before, but I’d prefer a name like
‘narinfo-signature->canonical-sexp’.

> +(define* (read-narinfo port #:optional url (acl (current-acl)))
> +  "Read a narinfo from PORT.  If URL is true, it must be a string used to
> +build full URIs from relative URIs found while reading PORT."
> +  (let* ((str       (begin (set-port-encoding! port "UTF-8")
> +                           (get-string-all port)))

Rather set the encoding when PORT is created, or use

  (utf8->string (get-bytevector-all port))

That’s it.

Did I miss something?

Thanks,
Ludo’.