Re: [PATCH 3/3] gnu: icedtea-6: Generate keystore.

[Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de>]

Ludovic Courtès <ludo@gnu.org> writes:

> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:
>
>> From: Ricardo Wurmus <rekado@elephly.net>
>>
>> * gnu/packages/java.scm (icedtea-6)[arguments]: Add phase
>> "install-keystore".
>> [native-inputs]: Add nss-certs and openssl.
>
> [...]
>
>> +         (add-after 'install 'install-keystore
>> +           (lambda* (#:key inputs outputs #:allow-other-keys)
>
> Could you add a comment to explain what’s going on here?
>
> Too bad IceTea’s build system doesn’t take care of that.
>
>> +             (let* ((keystore  "cacerts")
>> +                    (certs-dir (string-append (assoc-ref inputs "nss-certs")
>> +                                              "/etc/ssl/certs"))
>> +                    (keytool   (string-append (assoc-ref outputs "jdk")
>> +                                              "/bin/keytool"))
>> +                    (openssl   (which "openssl"))
>> +                    (recent    (date->time-utc (string->date "2016-1-1"
>> +                                                             "~Y-~m-~d"))))
>> +               (define (valid? cert)
>> +                 (let* ((port (open-pipe* OPEN_READ openssl
>> +                                          "x509" "-enddate" "-in" cert "-noout"))
>> +                        (str  (read-line port))
>> +                        (end  (begin (close-pipe port)
>> +                                     ;; TODO: use match?
>> +                                     (cadr (string-split str #\=)))))
>
> Why not use ‘match’, indeed.  :-)  No big deal though.
>
>> +                   (time>? (date->time-utc
>> +                            (string->date end "~b ~d ~H:~M:~S ~Y")) recent)))
>> +
>> +               (define (import-cert cert)
>> +                 (format #t "Importing certificate ~a\n" (basename cert))
>> +                 (let* ((port (open-pipe* OPEN_WRITE keytool
>> +                                          "-import"
>> +                                          "-alias" (basename cert)
>> +                                          "-keystore" keystore
>> +                                          "-storepass" "changeit"
>> +                                          "-file" cert)))
>> +                   (display "yes\n" port)
>> +                   (when (not (eqv? 0 (status:exit-val (close-pipe port))))
>
> Maybe (zero? (status:exit-val …)).
>
>> +                     (format (current-error-port)
>> +                             "Failed to import certificate.\n"))))
>
> Rather (error "failed to import" cert) so the process stops here.
>
>> +               ;; This is necessary because the certificate directory contains
>> +               ;; files with non-ASCII characters in their names.
>> +               (setlocale LC_ALL "en_US.utf8")
>> +               (setenv "LC_ALL" "en_US.utf8")
>> +
>> +               (for-each import-cert
>> +                         (filter valid? (find-files certs-dir "\\.pem$")))
>
> Why do we need to filter out invalid certificates?
>
> The problem I see is that the result of ‘valid?’, and thus the output of
> the build process, depends on the build time, which isn’t great.
>
> I would prefer to unconditionally install all the certificates if that
> doesn’t cause any problems.  WDYT?

I removed the validation (because I expect certs to be validated at
runtime).  I also added a comment explaining why this is needed and made
the suggested changes.

(I pushed from my workstation without signing key, because I forgot that
I normally push from my laptop.  Sorry, won’t happen again!  Key
replacement is on my list, and then I’ll get myself a subkey for the
office workstation.)

Thanks for the review!

~~ Ricardo