[PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Roel Janssen]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-gnu-icedtea-8-Build-keystore-without-id-ecPublicKey-.patch --]
[-- Type: text/x-diff, Size: 9136 bytes --]

From 8383c24c8a3c723535fe59f700a5fd18c50b4780 Mon Sep 17 00:00:00 2001
From: Roel Janssen <roel@gnu.org>
Date: Fri, 10 Feb 2017 12:23:22 +0100
Subject: [PATCH] gnu: icedtea-8:  Build keystore without id-ecPublicKey
 certificates.

* gnu/packages/java.scm (icedtea-8): Add 'install-keystore phase.
---
 gnu/packages/java.scm | 125 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 124 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/java.scm b/gnu/packages/java.scm
index 92cbe2a02..2b204d860 100644
--- a/gnu/packages/java.scm
+++ b/gnu/packages/java.scm
@@ -1025,7 +1025,130 @@ build process and its dependencies, whereas Make uses Makefile format.")
                    #t)))
              ;; FIXME: This phase is needed but fails with this version of
              ;; IcedTea.
-             (delete 'install-keystore)
+             (replace 'install-keystore
+               (lambda* (#:key inputs outputs #:allow-other-keys)
+                 (let* ((keystore  "cacerts")
+                        (certs-dir (string-append (assoc-ref inputs "nss-certs")
+                                                  "/etc/ssl/certs"))
+                        (keytool   (string-append (assoc-ref outputs "jdk")
+                                                  "/bin/keytool")))
+                   (define (extract-cert file target)
+                     (call-with-input-file file
+                       (lambda (in)
+                         (call-with-output-file target
+                           (lambda (out)
+                             (let loop ((line (read-line in 'concat))
+                                        (copying? #f))
+                               (cond
+                                ((eof-object? line) #t)
+                                ((string-prefix? "-----BEGIN" line)
+                                 (display line out)
+                                 (loop (read-line in 'concat) #t))
+                                ((string-prefix? "-----END" line)
+                                 (display line out)
+                                 #t)
+                                (else
+                                 (when copying? (display line out))
+                                 (loop (read-line in 'concat) copying?)))))))))
+                   (define (import-cert cert)
+                     ;; These certificates use a different public key algorithm:
+                     ;; id-ecPublicKey.  The keytool does not seem to be able to
+                     ;; import these certificates.
+                     (let ((bad-certs
+                            (list
+                             (string-append "CA_WoSign_ECC_Root:2.16.104.74.88."
+                                            "112.128.107.240.143.2.250.246.222."
+                                            "232.176.144.144.pem")
+                             (string-append "AffirmTrust_Premium_ECC:2.8.116.151"
+                                            ".37.138.199.63.122.84.pem")
+                             (string-append "GeoTrust_Primary_Certification_Aut"
+                                            "hority_-_G2:2.16.60.178.244.72.10."
+                                            "0.226.254.235.36.59.94.96.62.195.1"
+                                            "07.pem")
+                             (string-append "DigiCert_Assured_ID_Root_G3:2.16.1"
+                                            "1.161.90.250.29.223.160.181.73.68."
+                                            "175.205.36.160.108.236.pem")
+                             (string-append "COMODO_ECC_Certification_Authority"
+                                            ":2.16.31.71.175.170.98.0.112.80.84"
+                                            ".76.1.158.155.99.153.42.pem")
+                             (string-append "OpenTrust_Root_CA_G3:2.18.17.32.23"
+                                            "0.248.76.252.36.176.190.5.64.172.2"
+                                            "18.131.27.52.96.63.pem")
+                             (string-append "DigiCert_Global_Root_G3:2.16.5.85."
+                                            "86.188.242.94.164.53.53.195.164.15"
+                                            ".213.171.69.114.pem")
+                             (string-append "GlobalSign_ECC_Root_CA_-_R5:2.17.9"
+                                            "6.89.73.224.38.46.187.85.249.10.11"
+                                            "9.138.113.249.74.216.108.pem")
+                             (string-append "VeriSign_Class_3_Public_Primary_Ce"
+                                            "rtification_Authority_-_G4:2.16.47"
+                                            ".128.254.35.140.14.34.15.72.103.18"
+                                            ".40.145.135.172.179.pem")
+                             (string-append "Entrust_Root_Certification_Authori"
+                                            "ty_-_EC1:2.13.0.166.139.121.41.0.0"
+                                            ".0.0.80.208.145.249.pem")
+                             (string-append "thawte_Primary_Root_CA_-_G2:2.16.5"
+                                            "3.252.38.92.217.132.79.201.61.38.6"
+                                            "1.87.155.174.215.86.pem")
+                             (string-append "Certplus_Root_CA_G2:2.18.17.32.217"
+                                            ".145.206.174.163.232.197.231.255.2"
+                                            "33.2.175.207.115.188.85.pem")
+                             (string-append "Hellenic_Academic_and_Research_Ins"
+                                            "titutions_ECC_RootCA_2015:2.1.0.pe"
+                                            "m")
+                             (string-append "USERTrust_ECC_Certification_Author"
+                                            "ity:2.16.92.139.153.197.90.148.197"
+                                            ".210.113.86.222.205.137.128.204.38"
+                                            ".pem")
+                             (string-append "GlobalSign_ECC_Root_CA_-_R4:2.17.4"
+                                            "2.56.164.28.150.10.4.222.66.178.40"
+                                            ".165.11.232.52.152.2.pem"))))
+                       (unless (member (basename cert) bad-certs)
+                         (format #t "Importing certificate ~a\n" (basename cert))
+                         (let ((temp "tmpcert"))
+                           (extract-cert cert temp)
+                           (let ((port (open-pipe* OPEN_WRITE keytool
+                                                   "-import"
+                                                   "-alias" (basename cert)
+                                                   "-keystore" keystore
+                                                   "-storepass" "changeit"
+                                                   "-file" temp)))
+                             (display "yes\n" port)
+                             (when (not (zero? (status:exit-val (close-pipe port))))
+                               (error "failed to import" cert)))
+                           (delete-file temp)))))
+                   ;; 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 (find-files certs-dir "\\.pem$"))
+               (mkdir-p (string-append (assoc-ref outputs "out")
+                                       "/lib/security"))
+               (mkdir-p (string-append (assoc-ref outputs "jdk")
+                                       "/jre/lib/security"))
+
+               ;; The cacerts files we are going to overwrite are chmod'ed
+               ;; as read-only (444).  We have to change this temporarily.
+               (chmod (string-append (assoc-ref outputs "out")
+                                     "/lib/security/" keystore) #o644)
+               (chmod (string-append (assoc-ref outputs "jdk")
+                                     "/jre/lib/security/" keystore) #o644)
+
+               (install-file keystore
+                             (string-append (assoc-ref outputs "out")
+                                            "/lib/security"))
+               (install-file keystore
+                             (string-append (assoc-ref outputs "jdk")
+                                            "/jre/lib/security"))
+
+               ;; Now make it read-only again.
+               (chmod (string-append (assoc-ref outputs "out")
+                                     "/lib/security/" keystore) #o444)
+               
+               (chmod (string-append (assoc-ref outputs "jdk")
+                                     "/jre/lib/security/" keystore) #o444)
+               #t)))
              (replace 'install
                (lambda* (#:key outputs #:allow-other-keys)
                  (let ((doc (string-append (assoc-ref outputs "doc")
-- 
2.11.1


[-- Attachment #2: Type: text/plain, Size: 618 bytes --]

Dear Guix,

Currently, for icedtea-8 we use an empty "keystore".  This results in
Java processes using our icedtea-8 package not being able to verify
the validity of a certificate from a CA, because there are none in its
store.

This patch imports most certificates from nss-certs.  Those using a
"id-ecPublicKey" public key algorithm are left out.

I realize this patch is big and inelegant, so I welcome anyone to come
up with suggestions.  For example, could I somehow gather the public key
algorithm from the certificate and then check that instead of creating
this blacklist?

Thanks!

Kind regards,
Roel Janssen

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Carlo Zancanaro]

[-- Attachment #1.1: Type: text/plain, Size: 656 bytes --]

On Fri, Feb 10 2017, Roel Janssen wrote
> [ ... ]

I was getting frustrated at not having certificates with java 8 (it's
surprisingly annoying to have to use one environment with java 7 to
download dependencies with maven, then a different environment with java
8 to actually run your program), so I downloaded and tried out your
patch. It seems to work!

But then I wondered, could we just change the generate-keystore phase of
the icedtea-6 package to log a failed certificate import without failing
the build? Then we could move the permissions change there, too, which
would give us a smaller patch that should accomplish a similar result
(attached).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-gnu-icedtea-6-Modify-certificate-import-to-not-fail-.patch --]
[-- Type: text/x-patch, Size: 2883 bytes --]

From b1ed0d53a72f95fdc42fa3741ae16726782ad414 Mon Sep 17 00:00:00 2001
From: Carlo Zancanaro <carlo@zancanaro.id.au>
Date: Sun, 26 Feb 2017 11:34:44 +1100
Subject: [PATCH] gnu: icedtea-6: Modify certificate import to not fail for
 icedtea-8.

* gnu/packages/java.scm (icedtea-6)[arguments]: Fix install-keystore phase to
  not fail the build when attempting to import unsupported certificate
  types (which occur with icedtea-8, which inherits from icedtea-6). Also
  ensure that the keystore is able to be written to before copying it.
---
 gnu/packages/java.scm | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/gnu/packages/java.scm b/gnu/packages/java.scm
index e7479e1b0..c7f9b9aad 100644
--- a/gnu/packages/java.scm
+++ b/gnu/packages/java.scm
@@ -706,7 +706,7 @@ build process and its dependencies, whereas Make uses Makefile format.")
                                            "-file" temp)))
                      (display "yes\n" port)
                      (when (not (zero? (status:exit-val (close-pipe port))))
-                       (error "failed to import" cert)))
+                       (format #t "failed to import ~a\n" cert)))
                    (delete-file temp)))
 
                ;; This is necessary because the certificate directory contains
@@ -719,6 +719,15 @@ build process and its dependencies, whereas Make uses Makefile format.")
                                        "/lib/security"))
                (mkdir-p (string-append (assoc-ref outputs "jdk")
                                        "/jre/lib/security"))
+
+               ;; The cacerts files we are going to overwrite are chmod'ed as
+               ;; read-only (444) in icedtea-8 (which derives from this
+               ;; package).  We have to change this so we can overwrite them.
+               (chmod (string-append (assoc-ref outputs "out")
+                                     "/lib/security/" keystore) #o644)
+               (chmod (string-append (assoc-ref outputs "jdk")
+                                     "/jre/lib/security/" keystore) #o644)
+
                (install-file keystore
                              (string-append (assoc-ref outputs "out")
                                             "/lib/security"))
@@ -1023,9 +1032,6 @@ build process and its dependencies, whereas Make uses Makefile format.")
                     (find-files "openjdk.src/jdk/src/solaris/native"
                                 "\\.c|\\.h"))
                    #t)))
-             ;; FIXME: This phase is needed but fails with this version of
-             ;; IcedTea.
-             (delete 'install-keystore)
              (replace 'install
                (lambda* (#:key outputs #:allow-other-keys)
                  (let ((doc (string-append (assoc-ref outputs "doc")
-- 
2.11.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Roel Janssen]

Carlo Zancanaro writes:

> On Fri, Feb 10 2017, Roel Janssen wrote
>> [ ... ]
>
> I was getting frustrated at not having certificates with java 8 (it's
> surprisingly annoying to have to use one environment with java 7 to
> download dependencies with maven, then a different environment with java
> 8 to actually run your program), so I downloaded and tried out your
> patch. It seems to work!

Thanks for picking up the patch!

> But then I wondered, could we just change the generate-keystore phase of
> the icedtea-6 package to log a failed certificate import without failing
> the build? Then we could move the permissions change there, too, which
> would give us a smaller patch that should accomplish a similar result
> (attached).

Great idea.  This is also a more durable solution for when certificates
change in nss-certs.

> From b1ed0d53a72f95fdc42fa3741ae16726782ad414 Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <carlo@zancanaro.id.au>
> Date: Sun, 26 Feb 2017 11:34:44 +1100
> Subject: [PATCH] gnu: icedtea-6: Modify certificate import to not fail for
>  icedtea-8.
>
> * gnu/packages/java.scm (icedtea-6)[arguments]: Fix install-keystore phase to
>   not fail the build when attempting to import unsupported certificate
>   types (which occur with icedtea-8, which inherits from icedtea-6). Also
>   ensure that the keystore is able to be written to before copying it.
> ---
>  gnu/packages/java.scm | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/gnu/packages/java.scm b/gnu/packages/java.scm
> index e7479e1b0..c7f9b9aad 100644
> --- a/gnu/packages/java.scm
> +++ b/gnu/packages/java.scm
> @@ -706,7 +706,7 @@ build process and its dependencies, whereas Make uses Makefile format.")
>                                             "-file" temp)))
>                       (display "yes\n" port)
>                       (when (not (zero? (status:exit-val (close-pipe port))))
> -                       (error "failed to import" cert)))
> +                       (format #t "failed to import ~a\n" cert)))
>                     (delete-file temp)))
>  
>                 ;; This is necessary because the certificate directory contains
> @@ -719,6 +719,15 @@ build process and its dependencies, whereas Make uses Makefile format.")
>                                         "/lib/security"))
>                 (mkdir-p (string-append (assoc-ref outputs "jdk")
>                                         "/jre/lib/security"))
> +
> +               ;; The cacerts files we are going to overwrite are chmod'ed as
> +               ;; read-only (444) in icedtea-8 (which derives from this
> +               ;; package).  We have to change this so we can overwrite them.
> +               (chmod (string-append (assoc-ref outputs "out")
> +                                     "/lib/security/" keystore) #o644)
> +               (chmod (string-append (assoc-ref outputs "jdk")
> +                                     "/jre/lib/security/" keystore) #o644)
> +
>                 (install-file keystore
>                               (string-append (assoc-ref outputs "out")
>                                              "/lib/security"))

I checked to see if the keystore is actually chmod'ed back to #o444, and
it is!  So this looks fine to me as well.

> @@ -1023,9 +1032,6 @@ build process and its dependencies, whereas Make uses Makefile format.")
>                      (find-files "openjdk.src/jdk/src/solaris/native"
>                                  "\\.c|\\.h"))
>                     #t)))
> -             ;; FIXME: This phase is needed but fails with this version of
> -             ;; IcedTea.
> -             (delete 'install-keystore)
>               (replace 'install
>                 (lambda* (#:key outputs #:allow-other-keys)
>                   (let ((doc (string-append (assoc-ref outputs "doc")

I tried this patch and it works fine.

I think we should add ourselves to the copyright notice.
Other than that, I think this patch is good to be pushed.

Kind regards,
Roel Janssen

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Carlo Zancanaro]

[-- Attachment #1.1: Type: text/plain, Size: 544 bytes --]

On Sun, Feb 26 2017, Roel Janssen wrote
> Great idea.  This is also a more durable solution for when certificates
> change in nss-certs.

Yeah, that was my thinking. I had tried to do it earlier, but hadn't
noticed the incorrect permissions later on in the build (which had
caused my attempts to fail).

> I think we should add ourselves to the copyright notice.
> Other than that, I think this patch is good to be pushed.

I've added both of us to the copyright notice (I hope that isn't too
presumptuous). Patch is attached.

Thanks!

Carlo


[-- Attachment #1.2: 0001-gnu-icedtea-6-Modify-certificate-import-to-not-fail-.patch --]
[-- Type: text/x-patch, Size: 3306 bytes --]

From 1fb1116475506495f8f026c9b53cf955dec29742 Mon Sep 17 00:00:00 2001
From: Carlo Zancanaro <carlo@zancanaro.id.au>
Date: Sun, 26 Feb 2017 11:34:44 +1100
Subject: [PATCH] gnu: icedtea-6: Modify certificate import to not fail for
 icedtea-8.

* gnu/packages/java.scm (icedtea-6)[arguments]: Fix install-keystore phase to
  not fail the build when attempting to import unsupported certificate
  types (which occur with icedtea-8, which inherits from icedtea-6). Also
  ensure that the keystore is able to be written to before copying it.
---
 gnu/packages/java.scm | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/gnu/packages/java.scm b/gnu/packages/java.scm
index e7479e1b0..1abdf607f 100644
--- a/gnu/packages/java.scm
+++ b/gnu/packages/java.scm
@@ -1,7 +1,8 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015, 2016 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2016 Leo Famulari <leo@famulari.name>
-;;; Copyright © 2016 Roel Janssen <roel@gnu.org>
+;;; Copyright © 2016, 2017 Roel Janssen <roel@gnu.org>
+;;; Copyright © 2017 Carlo Zancanaro <carlo@zancanaro.id.au>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -706,7 +707,7 @@ build process and its dependencies, whereas Make uses Makefile format.")
                                            "-file" temp)))
                      (display "yes\n" port)
                      (when (not (zero? (status:exit-val (close-pipe port))))
-                       (error "failed to import" cert)))
+                       (format #t "failed to import ~a\n" cert)))
                    (delete-file temp)))
 
                ;; This is necessary because the certificate directory contains
@@ -719,6 +720,15 @@ build process and its dependencies, whereas Make uses Makefile format.")
                                        "/lib/security"))
                (mkdir-p (string-append (assoc-ref outputs "jdk")
                                        "/jre/lib/security"))
+
+               ;; The cacerts files we are going to overwrite are chmod'ed as
+               ;; read-only (444) in icedtea-8 (which derives from this
+               ;; package).  We have to change this so we can overwrite them.
+               (chmod (string-append (assoc-ref outputs "out")
+                                     "/lib/security/" keystore) #o644)
+               (chmod (string-append (assoc-ref outputs "jdk")
+                                     "/jre/lib/security/" keystore) #o644)
+
                (install-file keystore
                              (string-append (assoc-ref outputs "out")
                                             "/lib/security"))
@@ -1023,9 +1033,6 @@ build process and its dependencies, whereas Make uses Makefile format.")
                     (find-files "openjdk.src/jdk/src/solaris/native"
                                 "\\.c|\\.h"))
                    #t)))
-             ;; FIXME: This phase is needed but fails with this version of
-             ;; IcedTea.
-             (delete 'install-keystore)
              (replace 'install
                (lambda* (#:key outputs #:allow-other-keys)
                  (let ((doc (string-append (assoc-ref outputs "doc")
-- 
2.11.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Roel Janssen]

Carlo Zancanaro <carlo@zancanaro.id.au> writes:

> On Sun, Feb 26 2017, Roel Janssen wrote
>> Great idea.  This is also a more durable solution for when certificates
>> change in nss-certs.
>
> Yeah, that was my thinking. I had tried to do it earlier, but hadn't
> noticed the incorrect permissions later on in the build (which had
> caused my attempts to fail).
>
>> I think we should add ourselves to the copyright notice.
>> Other than that, I think this patch is good to be pushed.
>
> I've added both of us to the copyright notice (I hope that isn't too
> presumptuous). Patch is attached.

Great!

Unfortunately, I don't seem to be able to apply your patch.  It errors
with the following:

error: patch failed: gnu/packages/java.scm:1
error: gnu/packages/java.scm: patch does not apply

I tried with two different machines, both resulting in the error
displayed above.

Sorry for the inconvenience, but could you redo the patch?

Kind regards,
Roel Janssen

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Ricardo Wurmus]

Carlo Zancanaro <carlo@zancanaro.id.au> writes:

> But then I wondered, could we just change the generate-keystore phase of
> the icedtea-6 package to log a failed certificate import without failing
> the build? Then we could move the permissions change there, too, which
> would give us a smaller patch that should accomplish a similar result
> (attached).

Hmm, I have a slight preference to have the build fail in those cases,
because that prompts us to fix the underlying problem.  Roel’s fix seems
more direct, even though it results in more lines of code.

> From b1ed0d53a72f95fdc42fa3741ae16726782ad414 Mon Sep 17 00:00:00 2001
> From: Carlo Zancanaro <carlo@zancanaro.id.au>
> Date: Sun, 26 Feb 2017 11:34:44 +1100
> Subject: [PATCH] gnu: icedtea-6: Modify certificate import to not fail for
>  icedtea-8.
>
> * gnu/packages/java.scm (icedtea-6)[arguments]: Fix install-keystore phase to
>   not fail the build when attempting to import unsupported certificate
>   types (which occur with icedtea-8, which inherits from icedtea-6). Also
>   ensure that the keystore is able to be written to before copying it.
> ---
>  gnu/packages/java.scm | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/gnu/packages/java.scm b/gnu/packages/java.scm
> index e7479e1b0..c7f9b9aad 100644
> --- a/gnu/packages/java.scm
> +++ b/gnu/packages/java.scm
> @@ -706,7 +706,7 @@ build process and its dependencies, whereas Make uses Makefile format.")
>                                             "-file" temp)))
>                       (display "yes\n" port)
>                       (when (not (zero? (status:exit-val (close-pipe port))))
> -                       (error "failed to import" cert)))
> +                       (format #t "failed to import ~a\n" cert)))
>                     (delete-file temp)))
>
>                 ;; This is necessary because the certificate directory contains
> @@ -719,6 +719,15 @@ build process and its dependencies, whereas Make uses Makefile format.")
>                                         "/lib/security"))
>                 (mkdir-p (string-append (assoc-ref outputs "jdk")
>                                         "/jre/lib/security"))
> +
> +               ;; The cacerts files we are going to overwrite are chmod'ed as
> +               ;; read-only (444) in icedtea-8 (which derives from this
> +               ;; package).  We have to change this so we can overwrite them.
> +               (chmod (string-append (assoc-ref outputs "out")
> +                                     "/lib/security/" keystore) #o644)
> +               (chmod (string-append (assoc-ref outputs "jdk")
> +                                     "/jre/lib/security/" keystore) #o644)
> +

I don’t understand this.  It also seems inelegant to make a change in
“icedtea-6” for the sake of “icedtea-8”.  Could this be done in
“icedtea-8” instead?

Also note that icedtea-6 will eventually be removed (as it will no
longer receive upstream updates) and the other icedtea* packages should
no longer use inheritance to make that possible.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Carlo Zancanaro]

[-- Attachment #1: Type: text/plain, Size: 3157 bytes --]


On Mon, Feb 27 2017, Ricardo Wurmus wrote
> Carlo Zancanaro <carlo@zancanaro.id.au> writes:
>> But then I wondered, could we just change the generate-keystore phase of
>> the icedtea-6 package to log a failed certificate import without failing
>> the build? Then we could move the permissions change there, too, which
>> would give us a smaller patch that should accomplish a similar result
>> (attached).
>
> Hmm, I have a slight preference to have the build fail in those cases,
> because that prompts us to fix the underlying problem.  Roel’s fix seems
> more direct, even though it results in more lines of code.

Sure, but then we get a situation like we currently have: the build
fails, so the keystore generation is disabled (rather than fixed) and we
have a Java package that is heavily crippled (especially for development
purposes).

If it failing actually lead to people fixing the issue then I would be
fine with that, but the issue I have is that it doesn't get fixed, it
gets disabled. I'd much rather have it work for most use cases, rather
than failing on the slightest hiccup.

My issue isn't that Roel's fix is more lines of code, it's that it is
more brittle, it hard codes package information, and it results in
unnecessary duplication of code.

>> @@ -719,6 +719,15 @@ build process and its dependencies, whereas Make uses Makefile format.")
>>                                         "/lib/security"))
>>                 (mkdir-p (string-append (assoc-ref outputs "jdk")
>>                                         "/jre/lib/security"))
>> +
>> +               ;; The cacerts files we are going to overwrite are chmod'ed as
>> +               ;; read-only (444) in icedtea-8 (which derives from this
>> +               ;; package).  We have to change this so we can overwrite them.
>> +               (chmod (string-append (assoc-ref outputs "out")
>> +                                     "/lib/security/" keystore) #o644)
>> +               (chmod (string-append (assoc-ref outputs "jdk")
>> +                                     "/jre/lib/security/" keystore) #o644)
>> +
>
> I don’t understand this.

What don't you understand about it? If the comment is unclear then I am
happy to clarify it further.

> It also seems inelegant to make a change in “icedtea-6” for the sake
> of “icedtea-8”. Could this be done in “icedtea-8” instead?

I agree that it's inelegant, but the alternative is to duplicate the
entire phase and make the changes in icedtea-8. Given we are using
inheritance for code re-use here, I don't think it's too bad to do
something that's always valid to fix a problem in a specific case.

> Also note that icedtea-6 will eventually be removed (as it will no
> longer receive upstream updates) and the other icedtea* packages should
> no longer use inheritance to make that possible.

The 'install-keystore phase will have to be moved to icedtea-7, then,
where the same code will be required. The phase will have to move at
some point anyway, so I don't think this is particularly significant
that it's currently in icedtea-6.

Carlo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

On Tue, Feb 28, 2017 at 08:16:34AM +1100, Carlo Zancanaro wrote:
> On Mon, Feb 27 2017, Ricardo Wurmus wrote
> > Also note that icedtea-6 will eventually be removed (as it will no
> > longer receive upstream updates) and the other icedtea* packages should
> > no longer use inheritance to make that possible.
> 
> The 'install-keystore phase will have to be moved to icedtea-7, then,
> where the same code will be required. The phase will have to move at
> some point anyway, so I don't think this is particularly significant
> that it's currently in icedtea-6.

Off-topic, but I'd like for this to happen ASAP. Icedtea-6 is currently
unsupported and exploitable bugs continue to be discovered.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Carlo Zancanaro]

[-- Attachment #1.1: Type: text/plain, Size: 473 bytes --]

On Mon, Feb 27 2017, Roel Janssen wrote
> Unfortunately, I don't seem to be able to apply your patch. [ ... ]

Hmm. That's strange. I generated a new patch which hopefully will work.
I tried applying it to master on my machine and it seemed to work fine.

I'm not sure what to do with this in light of Ricardo's comments, but
I'm hopeful that it can be pushed. (The advantage not having the ability
to push is that I don't have to make any real decisions. Hooray!)

Carlo


[-- Attachment #1.2: 0001-gnu-icedtea-6-Modify-certificate-import-to-not-fail-.patch --]
[-- Type: text/x-patch, Size: 3306 bytes --]

From 8d499d361cb89c29902ef21c46b3899c2f6799f7 Mon Sep 17 00:00:00 2001
From: Carlo Zancanaro <carlo@zancanaro.id.au>
Date: Sun, 26 Feb 2017 11:34:44 +1100
Subject: [PATCH] gnu: icedtea-6: Modify certificate import to not fail for
 icedtea-8.

* gnu/packages/java.scm (icedtea-6)[arguments]: Fix install-keystore phase to
  not fail the build when attempting to import unsupported certificate
  types (which occur with icedtea-8, which inherits from icedtea-6). Also
  ensure that the keystore is able to be written to before copying it.
---
 gnu/packages/java.scm | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/gnu/packages/java.scm b/gnu/packages/java.scm
index e7479e1b0..1abdf607f 100644
--- a/gnu/packages/java.scm
+++ b/gnu/packages/java.scm
@@ -1,7 +1,8 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015, 2016 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2016 Leo Famulari <leo@famulari.name>
-;;; Copyright © 2016 Roel Janssen <roel@gnu.org>
+;;; Copyright © 2016, 2017 Roel Janssen <roel@gnu.org>
+;;; Copyright © 2017 Carlo Zancanaro <carlo@zancanaro.id.au>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -706,7 +707,7 @@ build process and its dependencies, whereas Make uses Makefile format.")
                                            "-file" temp)))
                      (display "yes\n" port)
                      (when (not (zero? (status:exit-val (close-pipe port))))
-                       (error "failed to import" cert)))
+                       (format #t "failed to import ~a\n" cert)))
                    (delete-file temp)))
 
                ;; This is necessary because the certificate directory contains
@@ -719,6 +720,15 @@ build process and its dependencies, whereas Make uses Makefile format.")
                                        "/lib/security"))
                (mkdir-p (string-append (assoc-ref outputs "jdk")
                                        "/jre/lib/security"))
+
+               ;; The cacerts files we are going to overwrite are chmod'ed as
+               ;; read-only (444) in icedtea-8 (which derives from this
+               ;; package).  We have to change this so we can overwrite them.
+               (chmod (string-append (assoc-ref outputs "out")
+                                     "/lib/security/" keystore) #o644)
+               (chmod (string-append (assoc-ref outputs "jdk")
+                                     "/jre/lib/security/" keystore) #o644)
+
                (install-file keystore
                              (string-append (assoc-ref outputs "out")
                                             "/lib/security"))
@@ -1023,9 +1033,6 @@ build process and its dependencies, whereas Make uses Makefile format.")
                     (find-files "openjdk.src/jdk/src/solaris/native"
                                 "\\.c|\\.h"))
                    #t)))
-             ;; FIXME: This phase is needed but fails with this version of
-             ;; IcedTea.
-             (delete 'install-keystore)
              (replace 'install
                (lambda* (#:key outputs #:allow-other-keys)
                  (let ((doc (string-append (assoc-ref outputs "doc")
-- 
2.11.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Ricardo Wurmus]

Carlo Zancanaro <carlo@zancanaro.id.au> writes:

> On Mon, Feb 27 2017, Roel Janssen wrote
>> Unfortunately, I don't seem to be able to apply your patch. [ ... ]
>
> Hmm. That's strange. I generated a new patch which hopefully will work.
> I tried applying it to master on my machine and it seemed to work fine.
>
> I'm not sure what to do with this in light of Ricardo's comments, but
> I'm hopeful that it can be pushed. (The advantage not having the ability
> to push is that I don't have to make any real decisions. Hooray!)

Thanks for the new patch.  I applied it as
ea9e58ef66f0fc0235eb1b36690ad4e41bf8771d after making a few minor
changes to the commit message.

I also added a Co-authored-by line for Roel as you updated his copyright
line.

Thanks!

~~ Ricardo

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Ricardo Wurmus]

Leo Famulari <leo@famulari.name> writes:

> On Tue, Feb 28, 2017 at 08:16:34AM +1100, Carlo Zancanaro wrote:
>> On Mon, Feb 27 2017, Ricardo Wurmus wrote
>> > Also note that icedtea-6 will eventually be removed (as it will no
>> > longer receive upstream updates) and the other icedtea* packages should
>> > no longer use inheritance to make that possible.
>> 
>> The 'install-keystore phase will have to be moved to icedtea-7, then,
>> where the same code will be required. The phase will have to move at
>> some point anyway, so I don't think this is particularly significant
>> that it's currently in icedtea-6.
>
> Off-topic, but I'd like for this to happen ASAP. Icedtea-6 is currently
> unsupported and exploitable bugs continue to be discovered.

I’m currently working on some Java packages again and I’ll try to take
care of removing “icedtea-6” in the process.

~~ Ricardo

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Roel Janssen]

Ricardo Wurmus writes:

> Carlo Zancanaro <carlo@zancanaro.id.au> writes:
>
>> On Mon, Feb 27 2017, Roel Janssen wrote
>>> Unfortunately, I don't seem to be able to apply your patch. [ ... ]
>>
>> Hmm. That's strange. I generated a new patch which hopefully will work.
>> I tried applying it to master on my machine and it seemed to work fine.
>>
>> I'm not sure what to do with this in light of Ricardo's comments, but
>> I'm hopeful that it can be pushed. (The advantage not having the ability
>> to push is that I don't have to make any real decisions. Hooray!)
>
> Thanks for the new patch.  I applied it as
> ea9e58ef66f0fc0235eb1b36690ad4e41bf8771d after making a few minor
> changes to the commit message.
>
> I also added a Co-authored-by line for Roel as you updated his copyright
> line.
>
> Thanks!

Thanks!  What made you confident to apply it?  I think this is the right
decision, because it's a separate issue from whatever is going to happen
to icedtea-6.  Using the inheritance seems like the most effective way
of working here, and the fix does not lead to a potential security hole
because all that can happen is that certificates do not get imported
into the keystore.

We do have to pay attention to whether certificates fail to be added
though..

Thanks Carlo and Ricardo!

Kind regards,
Roel Janssen

Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.

[Ricardo Wurmus]

Roel Janssen <roel@gnu.org> writes:

> Ricardo Wurmus writes:
>
>> Carlo Zancanaro <carlo@zancanaro.id.au> writes:
>>
>>> On Mon, Feb 27 2017, Roel Janssen wrote
>>>> Unfortunately, I don't seem to be able to apply your patch. [ ... ]
>>>
>>> Hmm. That's strange. I generated a new patch which hopefully will work.
>>> I tried applying it to master on my machine and it seemed to work fine.
>>>
>>> I'm not sure what to do with this in light of Ricardo's comments, but
>>> I'm hopeful that it can be pushed. (The advantage not having the ability
>>> to push is that I don't have to make any real decisions. Hooray!)
>>
>> Thanks for the new patch.  I applied it as
>> ea9e58ef66f0fc0235eb1b36690ad4e41bf8771d after making a few minor
>> changes to the commit message.
>>
>> I also added a Co-authored-by line for Roel as you updated his copyright
>> line.
>>
>> Thanks!
>
> Thanks!  What made you confident to apply it?

I applied it for pretty much the same reasons you gave:

> I think this is the right
> decision, because it's a separate issue from whatever is going to happen
> to icedtea-6.  Using the inheritance seems like the most effective way
> of working here, and the fix does not lead to a potential security hole
> because all that can happen is that certificates do not get imported
> into the keystore.

+1

> We do have to pay attention to whether certificates fail to be added
> though..

Indeed.  This is something users will notice.

~~ Ricardo