Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.
Roel Janssen writes: > Ricardo Wurmus writes: > >> Carlo Zancanaro 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
Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.
Ricardo Wurmus writes: > Carlo Zancanaro 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.
Leo Famulari 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.
Carlo Zancanaro 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.
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 From 8d499d361cb89c29902ef21c46b3899c2f6799f7 Mon Sep 17 00:00:00 2001 From: Carlo Zancanaro 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 ;;; Copyright © 2016 Leo Famulari -;;; Copyright © 2016 Roel Janssen +;;; Copyright © 2016, 2017 Roel Janssen +;;; Copyright © 2017 Carlo Zancanaro ;;; ;;; 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 signature.asc Description: PGP signature
Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.
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. signature.asc Description: PGP signature
Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.
On Mon, Feb 27 2017, Ricardo Wurmus wrote > Carlo Zancanaro 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 signature.asc Description: PGP signature
Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.
Carlo Zancanaro 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 > 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 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.
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 From 1fb1116475506495f8f026c9b53cf955dec29742 Mon Sep 17 00:00:00 2001 From: Carlo Zancanaro 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 ;;; Copyright © 2016 Leo Famulari -;;; Copyright © 2016 Roel Janssen +;;; Copyright © 2016, 2017 Roel Janssen +;;; Copyright © 2017 Carlo Zancanaro ;;; ;;; 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 signature.asc Description: PGP signature
Re: [PATCH] gnu: icedtea-8: Build keystore without id-ecPublicKey certificates.
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 > 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.
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). From b1ed0d53a72f95fdc42fa3741ae16726782ad414 Mon Sep 17 00:00:00 2001 From: Carlo Zancanaro 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 signature.asc Description: PGP signature
