RE: [11u] RFR: 8226374: Restrict TLS signature schemes and named groups

[Langer, Christoph]

Hi Paul,

thanks for the review. The CSR that Martin mentions is the one that Oracle has 
filed for 11.0.12-oracle. so we can simply reuse it.

As for 13, there exists a CSR as well: JDK-8256335

Best regards
Christoph

> -Original Message-
> From: Hohensee, Paul 
> Sent: Mittwoch, 7. April 2021 23:42
> To: Doerr, Martin ; jdk-updates-dev  d...@openjdk.java.net>; security-dev 
> Cc: Lindenmaier, Goetz ; Langer, Christoph
> 
> Subject: Re: [11u] RFR: 8226374: Restrict TLS signature schemes and named
> groups
> 
> The backport looks fine, except there's a missing blank line after FFDHE_2048
> in NamedGroup.java. :) Thanks for filing a CSR (there doesn't seem to be one
> for the 13u backport: perhaps Yan will add one after the fact). I'm not a
> security person, so it would be great if someone who is reviews the CSR to
> see if there are any 11u-specific issues with it.
> 
> Thanks,
> Paul
> 
> -Original Message-
> From: jdk-updates-dev  on
> behalf of "Doerr, Martin" 
> Date: Wednesday, April 7, 2021 at 9:10 AM
> To: jdk-updates-dev , security-dev
> 
> Cc: "Lindenmaier, Goetz" , "Langer,
> Christoph" 
> Subject: [11u] RFR: 8226374: Restrict TLS signature schemes and named
> groups
> 
> Hi,
> 
> JDK-8226374 is backported to 11.0.12-oracle. I'd like to backport it for 
> parity.
> It doesn't apply cleanly. I've taken the 13u backport as source because it
> resolves the wrong backport order with JDK-8242141.
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8226374
> 
> 11u CSR:
> https://bugs.openjdk.java.net/browse/JDK-8264555
> 
> Original change (JDK14):
> https://hg.openjdk.java.net/jdk/jdk/rev/a93b7b28f644
> 
> 13u backport:
> https://github.com/openjdk/jdk13u-dev/commit/384445d2
> 
> 11u rejected hunks (integrated manually):
> http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/8226374_TLS_rej.txt
> 
> my new 11u backport:
> http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/webrev.00/
> 
> Please review.
> 
> Best regards,
> Martin
> 

RE: [11u] RFR: 8254631: Better support ALPN byte wire values in SunJSSE

[Langer, Christoph]

Hi Martin,

looks good to me.

Best regards
Christoph

From: Doerr, Martin 
Sent: Dienstag, 30. März 2021 14:01
To: jdk-updates-dev ; security-dev 

Cc: Langer, Christoph 
Subject: [11u] RFR: 8254631: Better support ALPN byte wire values in SunJSSE

Hi,

JDK-8254631 is backported to 11.0.12-oracle. I'd like to backport it for parity.
It applies cleanly, but the javadoc parts don't compile with 11u. They are not 
compatible with 11u and are documented to be dropped in the CSR (linked below).
As also documented in the CSR, the old behavior can get restored by specifying 
the property: jdk.tls.alpnCharset=UTF-8

Bug:
https://bugs.openjdk.java.net/browse/JDK-8254631

Original change:
https://git.openjdk.java.net/jdk/commit/fe51

11u CSR:
https://bugs.openjdk.java.net/browse/JDK-8264177

11u backport:
http://cr.openjdk.java.net/~mdoerr/8254631_ALPN_11u/webrev.00/

Please review.

Best regards,
Martin

RE: [11u] RFR: 8206925: Support the certificate_authorities extension

[Langer, Christoph]

Hi Martin,

your backport looks good. I see the new tests pass and our testing does not 
unveil other regressions. Reviewed.

Oracle has already included this item in 11.0.10 but it fell through the cracks 
for OpenJDK 11u due to an issue with the updates filter. However, it seems like 
an important item for TLS 1.3 usability. We have just received a customer 
request why this wasn’t included in 11u yet, they would need it for their 
product to move on to TLS 1.3. So I think we should strive for 11.0.11 with 
this backport. Please label accordingly. Adding @Andrew 
Haley<mailto:a...@redhat.com> and @Severin Gehwolf<mailto:sgehw...@redhat.com> 
for their opinion on this decision 😊

The CSR https://bugs.openjdk.java.net/browse/JDK-8248709 should apply to this 
backport, please link it to the JBS issue.

Thanks & Best regards
Christoph

From: Doerr, Martin 
Sent: Dienstag, 23. März 2021 16:25
To: jdk-updates-...@openjdk.java.net; security-dev 

Cc: Lindenmaier, Goetz ; Langer, Christoph 

Subject: [11u] RFR: 8206925: Support the certificate_authorities extension

Hi,

JDK-8206925 was backported to 11.0.10-oracle, but it’s still missing in the 
Open Source version.
I'd like to backport it for parity.
It does apply cleanly, but I had to modify it, because the following change is 
not in 11u:
https://bugs.openjdk.java.net/browse/JDK-8215712

Bug:
https://bugs.openjdk.java.net/browse/JDK-8206925

Original change:
https://hg.openjdk.java.net/jdk/jdk/rev/827bac238aa0

11u backport:
http://cr.openjdk.java.net/~mdoerr/8206925_ca_ext_11u/webrev.00/

Manual change to make it work without JDK-8215712 (SSLStringizer and derived 
classes don’t take a HandshakeContext in 11u):
http://cr.openjdk.java.net/~mdoerr/8206925_ca_ext_11u/8206925_ca_ext_diff.txt

Please review.

Best regards,
Martin

RE: [11u] RFR: 8256421: Add 2 HARICA roots to cacerts truststore

[Langer, Christoph]

Hi Martin,

this backport looks good to me.

Best regards
Christoph

From: Doerr, Martin 
Sent: Donnerstag, 18. Februar 2021 12:11
To: security-dev ; 
jdk-updates-...@openjdk.java.net
Cc: Langer, Christoph ; Lindenmaier, Goetz 

Subject: [11u] RFR: 8256421: Add 2 HARICA roots to cacerts truststore

Hi,

JDK-8256421 is backported to 11.0.11-oracle. I'd like to backport it for parity.
It doesn't apply cleanly.

I'm using the jdk16u backport. See "Fix Request (jdk16u)" comment.

VerifyCACerts.java:
I had to change the COUNT manually:
-private static final int COUNT = 95;
+private static final int COUNT = 97;
And I've computed the new CHECKSUM which gets verified by the test:
shasum -a 256 jdk/lib/security/cacerts | sed -e 's/../&:/g' | tr '[:lower:]' 
'[:upper:]' | cut -c1-95
9F:6B:41:1D:05:AF:E3:C5:4F:E8:39:89:50:79:60:B1:F6:A4:02:40:0C:28:8D:73:78:08:E5:61:7C:17:EA:59

Bug:
https://bugs.openjdk.java.net/browse/JDK-8256421

Original change (from 16u):
https://github.com/openjdk/jdk16u/commit/4ccaf6b8

11u backport:
http://cr.openjdk.java.net/~mdoerr/8256421_HARICA_cacerts_11u/webrev.00/

Please review.

Best regards,
Martin

RE: RFR [XS]: 8239457: call ReleaseStringUTFChars before early returns in Java_sun_security_pkcs11_wrapper_PKCS11_connect

[Langer, Christoph]

Hi Matthias,

this looks correct. Thanks for taking care of it.

Best regards
Christoph

From: Baesken, Matthias 
Sent: Freitag, 21. Februar 2020 13:18
To: security-dev@openjdk.java.net
Cc: Langer, Christoph 
Subject: RE: RFR [XS]: 8239457: call ReleaseStringUTFChars before early returns 
in Java_sun_security_pkcs11_wrapper_PKCS11_connect

Ping - any reviews please ?

Best regards, Matthias


From: Baesken, Matthias
Sent: Mittwoch, 19. Februar 2020 13:55
To: security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>
Subject: RFR [XS]: 8239457: call ReleaseStringUTFChars before early returns in 
Java_sun_security_pkcs11_wrapper_PKCS11_connect

Hello, please review this small  change to  unix  
Java_sun_security_pkcs11_wrapper_PKCS11_connect .
Here we miss a call to ReleaseStringUTFChars  before early returns , e.g.  
these early returns .


127 // with the old JAR file jGetFunctionList is null, temporarily check 
for that
128 if (jGetFunctionList != NULL) {
129 getFunctionListStr = (*env)->GetStringUTFChars(env, 
jGetFunctionList, 0);
130 if (getFunctionListStr == NULL) {
131 return;
132 }
 ...
 135 }
136 if (C_GetFunctionList == NULL) {
137 throwIOException(env, "ERROR: C_GetFunctionList == NULL");
138 return;
139 } else if ( (systemErrorMessage = dlerror()) != NULL ){
140 throwIOException(env, systemErrorMessage);
141 return;
142 }

Fix is to move the last call to (*env)->ReleaseStringUTFChars(env, 
jPkcs11ModulePath, libraryNameStr);
Some lines  forward in the function  
Java_sun_security_pkcs11_wrapper_PKCS11_connect .


Bug/webrev :

https://bugs.openjdk.java.net/browse/JDK-8239457

http://cr.openjdk.java.net/~mbaesken/webrevs/8239457.0/


Thanks, Matthias

RE: RFR [XS]: 8239333: test/jdk/security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java fails intermittent

[Langer, Christoph]

Hi Matthias,

this makes sense. Change looks fine. Thanks for doing it.

Cheers
Christoph

From: security-dev  On Behalf Of 
Baesken, Matthias
Sent: Dienstag, 18. Februar 2020 10:56
To: security-dev@openjdk.java.net
Subject: [CAUTION] RFR [XS]: 8239333: 
test/jdk/security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java
 fails intermittent

Hello, please review this small test  change .
AmazonCA.java fails intermittent
In the discussion of  https://bugs.openjdk.java.net/browse/JDK-8238157it 
has been found  that   test   AmazonCA.java fails intermittent  .

So to make this transparent we should add the  intermittent   key to the test 
(until this issue is resolved).



Bug/webrev :

https://bugs.openjdk.java.net/browse/JDK-8239333

http://cr.openjdk.java.net/~mbaesken/webrevs/8239333.0/


Thanks, Matthias

RE: RFR: 8225128: Add exception for expiring DocuSign root to VerifyCACerts test

[Langer, Christoph]

Hi Rajan,

looks good. Thanks for doing this.

Best regards
Christoph

From: security-dev  On Behalf Of Rajan 
Halade
Sent: Freitag, 14. Februar 2020 22:15
To: security-dev@openjdk.java.net
Subject: RFR: 8225128: Add exception for expiring DocuSign root to 
VerifyCACerts test

May I request you to review this small update to add keynectisrootca to expiry 
check exempt list. JDK-8225068 will remove this certificate after expiry.

diff -r af8e77a59bd8 test/jdk/sun/security/lib/cacerts/VerifyCACerts.java
--- a/test/jdk/sun/security/lib/cacerts/VerifyCACerts.javaFri Feb 
14 12:47:18 2020 -0800
+++ b/test/jdk/sun/security/lib/cacerts/VerifyCACerts.java Fri Feb 14 
13:13:14 2020 -0800
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2020, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -259,6 +259,8 @@
 {
 // Valid until: Tue Jul 09 14:40:36 EDT 2019
 add("utnuserfirstobjectca [jdk]");
+// Valid until: Tue May 26 00:00:00 GMT 2020
+add("keynectisrootca [jdk]");
 }
 };


Thanks,
Rajan

RE: RFR: 8237962: give better error output for invalid OCSP response intervals in CertPathValidator checks

[Langer, Christoph]

Hi Matthias,

I'm wondering whether we should add cpve as cause of the RuntimeException in 
test/jdk/security/infra/java/security/cert/CertPathValidator/certification/ValidatePathWithParams.java,
 line 177, instead of printing it out explicitly?

Like:
throw new RuntimeException(
"TEST FAILED: couldn't determine EE certificate 
status", cvpe);

Best regards
Christoph

> -Original Message-
> From: Baesken, Matthias 
> Sent: Donnerstag, 30. Januar 2020 09:25
> To: Sean Mullan ; security-
> d...@openjdk.java.net
> Cc: Langer, Christoph 
> Subject: RE: RFR: 8237962: give better error output for invalid OCSP response
> intervals in CertPathValidator checks
> 
> Thanks, may I have a second review ?
> 
> Best regards, Matthias
> 
> 
> 
> > Looks good.
> >
> > Thanks,
> > Sean
> >
> > On 1/29/20 4:20 AM, Baesken, Matthias wrote:
> > >
> > > Hi Sean, new webrev :
> > >
> > >
> > > http://cr.openjdk.java.net/~mbaesken/webrevs/8237962.1/
> > >
> > >
> > > Best Regards, Matthias
> > >

RE: [CAUTION] RFR [XS]: 8237869: exclude jtreg test security/infra/java/security/cert/CertPathValidator/certification/LuxTrustCA.java because of instabilities - was : RE: jtreg test security/infra/jav

[Langer, Christoph]

+1

I converted JDK-8237869 to be a subtask of JDK-8237888.

@Sean: I couldn't assign it to you but I assume you'll pick it yourself and add 
do the necessary updates. Thanks.

Best regards
Christoph

> -Original Message-
> From: Sean Mullan 
> Sent: Montag, 27. Januar 2020 17:13
> To: Baesken, Matthias ; Langer, Christoph
> ; security-dev@openjdk.java.net
> Subject: Re: [CAUTION] RFR [XS]: 8237869: exclude jtreg test
> security/infra/java/security/cert/CertPathValidator/certification/LuxTrustCA.
> java because of instabilities - was : RE: jtreg test
> security/infra/java/security/cert/CertPathValidator/certification/LuxTrus
> 
> Looks fine to me.
> 
> --Sean
> 
> On 1/27/20 11:10 AM, Baesken, Matthias wrote:
> > Hi,  new webrev , now with the other bugid :
> >
> > http://cr.openjdk.java.net/~mbaesken/webrevs/8237869.1/
> >
> >
> >
> >   ... and  new  issue to track the LuxTrust instabilities :
> >
> > https://bugs.openjdk.java.net/browse/JDK-8237888
> >
> security/infra/java/security/cert/CertPathValidator/certification/LuxTrustCA.
> java fails when checking validity interval
> >
> >
> > Best regards, Matthias
> >
> >
> >> On 1/27/20 8:41 AM, Langer, Christoph wrote:
> >>> Hi Matthias,
> >>>
> >>> the entry in ProblemList.txt can't refer to 8237869, which is the bug that
> >> you're using to submit the exclusion. It must refer to the item that shall
> >> resolve the underlying issue which probably is Oracle's private bug that
> Sean
> >> referred to.
> >> Right.
> >>
> >>> @Sean: In the interest of backportability, I'd ask you to either open up
> the
> >> internal bug and supply its id. If that isn't possible, could you please 
> >> create
> a
> >> new public item and have your internal bug refer to it?
> >>
> >> I can't open it up because it contains internal network addresses. Even
> >> if I scrub them out, I believe you could still see them in the History.
> >>
> >> I would suggest that Matthias open a new public bug to track this issue
> >> using the information in the Description of JDK-8237869, and I will then
> >> mark 8237869 as a duplicate. You can assign that bug to me for now. Most
> >> likely it is an issue that LuxTrust has to fix on their end, and once
> >> that is fixed we can simply remove it from the ProblemList.
> >>
> >> Thanks,

RE: [CAUTION] RFR [XS]: 8237869: exclude jtreg test security/infra/java/security/cert/CertPathValidator/certification/LuxTrustCA.java because of instabilities - was : RE: jtreg test security/infra/jav

[Langer, Christoph]

Hi Matthias,

the entry in ProblemList.txt can't refer to 8237869, which is the bug that 
you're using to submit the exclusion. It must refer to the item that shall 
resolve the underlying issue which probably is Oracle's private bug that Sean 
referred to.

@Sean: In the interest of backportability, I'd ask you to either open up the 
internal bug and supply its id. If that isn't possible, could you please create 
a new public item and have your internal bug refer to it?

Thanks
Christoph


> -Original Message-
> From: security-dev  On Behalf Of
> Baesken, Matthias
> Sent: Montag, 27. Januar 2020 10:25
> To: Sean Mullan ; security-
> d...@openjdk.java.net
> Subject: [CAUTION] RFR [XS]: 8237869: exclude jtreg test
> security/infra/java/security/cert/CertPathValidator/certification/LuxTrustCA.
> java because of instabilities - was : RE: jtreg test
> security/infra/java/security/cert/CertPathValidator/certification/LuxTrust...
> 
> Hello, please review the exclusion of  jtreg test
> security/infra/java/security/cert/CertPathValidator/certification/LuxTrustCA.
> java  .
> 
> Bug/webrev :
> 
> https://bugs.openjdk.java.net/browse/JDK-8237869
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8237869.0/
> 
> Thanks, Matthias
> 
> 
> > -Original Message-
> > From: Sean Mullan 
> > Sent: Freitag, 24. Januar 2020 21:11
> > To: Baesken, Matthias ; security-
> > d...@openjdk.java.net
> > Subject: Re: jtreg test
> >
> security/infra/java/security/cert/CertPathValidator/certification/LuxTrustCA.
> > java instabilities
> >
> > On 1/24/20 3:40 AM, Baesken, Matthias wrote:
> > > Hi Sean, thanks for looking into it .
> > >
> > >>However, there is no nextUpdate field set, which means there should
> be
> > always newer information available. So while the 5 minute delay may not
> be
> > a huge issue, the fact that they are returning cached responses,
> > >
> > >>looks like a problem to me.This could be the underlying problem, in that
> > they are not generating fresh OCSPResponses. I will contact LuxTrust and
> see
> > if we can get some information from them.
> > >
> > > Can we exclude  the test  until  the issue is resolved ?
> >
> > Ok, that is fine with me.
> >

RE: [11u] RFR: 8213010: Supporting keys created with certmgr.exe

[Langer, Christoph]

Hi Goetz,

this looks good to me. I compared security.cpp after I've applied your changes 
to the version in jdk/jdk and there are only the changes missing that come with 
newer changes JDK-8223003 and JDK-8223063.

Thanks for backporting this.

Cheers
Christoph

> -Original Message-
> From: jdk-updates-dev  On
> Behalf Of Lindenmaier, Goetz
> Sent: Samstag, 21. Dezember 2019 13:21
> To: jdk-updates-...@openjdk.java.net; OpenJDK Dev list  d...@openjdk.java.net>
> Subject: [CAUTION] [11u] RFR: 8213010: Supporting keys created with
> certmgr.exe
> 
> Hi,
> 
> I would like to downport this for parity with 11.0.7-Oracle.
> It does not apply clean because in 11 changes were applied in
> a different order than in 14.
> 
> http://cr.openjdk.java.net/~goetz/wr19/8213010-certmgr.exe_keys-jdk11/
> 
> In ECUtil.java I had to resolve the Copyright.
> 
> patching file
> src/jdk.crypto.mscapi/windows/native/libsunmscapi/security.cpp
> This fails because "8221407: Windows 32bit build error in
> libsunmscapi/security.cpp" and
> "8210870: Libsunmscapi improved interactions" are already in 11, but in 14
> they come
> on top of this change.
> I resolved this file so that it looks the same as in 14 after 8221407.
> 
> After pushing 8213009 and this, jdk11u-dev and jdk/jdk will have the same
> changes
> for security.cpp:
> 
> jdk11 Filelog for security.cpp:
> 
> 8213010: Supporting keys created with certmgr.exe
> 8213009: Refactoring existing SunMSCAPI classes
> 8210476: sun/security/mscapi/PrngSlow.java fails with Still too slow
> 8201355: Avoid native memory allocation in
> sun.security.mscapi.PRNG.generateSeed
> 8221407: Windows 32bit build error in libsunmscapi/security.cpp
> 8210870: Libsunmscapi improved interactions
> 8196897: Improve PRNG supportjdk-11.0.1+0
> 8205445: Add RSASSA-PSS Signature support to SunMSCAPI
> 8204572: SetupJdkLibrary should setup SRC and -I flags automatically
> 8193262: JNI array not released in libsunmscapi convertToLittleEndian
> 8198898: Compilation errors in jdk.crypto.mscapi with VS 2017
> 8199198: Remove unused functions in jdk.crypto.mscapi native code
> 8187443: Forest Consolidation: Move files to unified layout
> 
> Jdk/jdk filelog for security.cpp:
> 
> 8221407: Windows 32bit build error in libsunmscapi/security.cpp
> 8210870: Libsunmscapi improved interactions
> 8213010: Supporting keys created with certmgr.exe
> 8213009: Refactoring existing SunMSCAPI classes
> 8210476: sun/security/mscapi/PrngSlow.java fails with Still too slow
> 8201355: Avoid native memory allocation in
> sun.security.mscapi.PRNG.generateSeed
> 8196897: Improve PRNG support
> 8205445: Add RSASSA-PSS Signature support to SunMSCAPI
> 8204572: SetupJdkLibrary should setup SRC and -I flags automatically
> 8193262: JNI array not released in libsunmscapi convertToLittleEndian
> 8198898: Compilation errors in jdk.crypto.mscapi with VS 2017
> 8199198: Remove unused functions in jdk.crypto.mscapi native code
> 8187443: Forest Consolidation: Move files to unified layout
> 
> Jdk/jdk has two changes on top of this.
> But if updating to 8221407 in jdk/jdk, security.cpp are
> Similar in both repos.
> 
> Best regards,
>   Goetz.

RE: [8u] RFR: 8232019: Add LuxTrust certificate updates to the existing root program

[Langer, Christoph]

Hi,

Just FWIW: JDK-8234245 fixed an issue where a wrong checksum was used in the 
test update that came with JDK-8232019. So, both can probably marked fixed with 
one change (e.g. adding both bugs to the submit message...)

Cheers
Christoph

> -Original Message-
> From: jdk8u-dev  On Behalf Of
> Severin Gehwolf
> Sent: Donnerstag, 19. Dezember 2019 21:14
> To: Andrew John Hughes ; jdk8u-dev  d...@openjdk.java.net>
> Cc: security-dev 
> Subject: Re: [8u] RFR: 8232019: Add LuxTrust certificate updates to the
> existing root program
> 
> Hi Andrew,
> 
> On Thu, 2019-12-19 at 19:29 +, Andrew John Hughes wrote:
> >
> > On 17/12/2019 19:30, Severin Gehwolf wrote:
> > > Hi,
> > >
> > > Could I please get a review of this OpenJDK 8u backport of 8232019. The
> > > JDK 11 patch did not apply cleanly for a couple of reasons:
> > >
> > >1. 8u still has the binary blob for cacerts (JDK-8193255 not
> > >   backported, yet). Instead, I've updated to the revision in jdk11u,
> > >   performed a build and copied the cacerts binary to 8u.
> > >2. JDK-8225392 not present in 8u, which added the checksum to
> > >   VerifyCACerts.java. Thus, the 8u backport does not include this
> > >   hunk. @bug annotation modified manually for the same reason.
> > >
> > > Everything else is the same.
> > >
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8232019
> > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> 8232019/jdk8/01/webrev/
> > >
> > > Testing: sun/security/lib/cacerts/VerifyCACerts.java and
> > >  security/infra/java/security/cert/CertPathValidator/certification
> > >  Pass, except for ActalisCA.java which is problem-listed and still
> > >  broken in HEAD (JDK-8224768)
> > >
> > > Thoughts?
> > >
> > > If reviewed, I'll try to get this in 8u242 via the critical fix request
> > > label workflow.
> > >
> > > Thanks,
> > > Severin
> > >
> >
> > Going on this & the similar Amazon fix, I'd say we should backport
> > JDK-8193255 & JDK-8225392 first. The previous updates which alter a
> > binary file have been pretty much unreviewable and, if there's a better
> > solution to that, I'd rather we had it sooner rather than later.
> 
> I agree with you that we should backport JDK-8193255. Question is when
> would be a good time to do this. Given that there would be some benefit
> for these to go into 8u242 if possible I'm not sure we should do JDK-
> 8193255 right now. After all, we've accepted this situation of having
> the binary blob for all of 8u222 and 8u232. Note that JDK-8189131 -
> which brought this in - was integrated in 8u222.
> 
> The risk of a backport of JDK-8193255 seems higher (the build system in
> 8u is different, there is a bug tail) than accepting these backports
> with the binary blob updates. Note that the backports as-is have been
> reviewed by Christoph Langer, Volker Simonis and internally by Martin
> Balao.
> 
> So, it looks like there are the following options:
>1. Accept backports of JDK-8232019 and JDK-8233223 into 8u242 as is.
>2. Backport JDK-8193255, JDK-8225392, JDK-8234245, JDK-8232019 and JDK-
>   8233223 to 8u242.
>3. Defer backports of 2) to 8u252 with the caveat that we won't have
>   certain certificates as compared to Oracle JDK.
> 
> I'm leaning towards option 1) or 3). Slight preference for 1)
> 
> > Likewise, judging by the comment on JDK-8234245, I'd say that needs to
> > be applied between the LuxTrust & Amazon ones:
> >
> > "This fixes an issue after JDK-8232019, so it needs to be included.
> > Patch applies cleanly."
> 
> Not sure what caused JDK-8234245. It might be that it was caused by the
> list of certs in the keystore not being ordered at first (fixed by JDK-
> 8225392?).
> 
> Thanks,
> Severin

RE: [8u] RFR: 8233223: Add Amazon Root CA certificates

[Langer, Christoph]

Hi Severin,

same here, looks good - when VerifyCACerts passes, everything is correct.

Cheers
Christoph

> -Original Message-
> From: security-dev  On Behalf Of
> Severin Gehwolf
> Sent: Donnerstag, 19. Dezember 2019 11:10
> To: Volker Simonis 
> Cc: jdk8u-dev ; security-dev  d...@openjdk.java.net>
> Subject: Re: [8u] RFR: 8233223: Add Amazon Root CA certificates
> 
> Hi Volker,
> 
> On Wed, 2019-12-18 at 22:27 +0100, Volker Simonis wrote:
> > Hi Severin,
> >
> > not strictly a 8u "Reviewer" yet, but I've looked at your changes
> > (this one and 8232019) nevertheless :)
> 
> Thanks for the review!
> 
> > They both look good, except that I can not verify the new "cacert"
> > file because it is not in the patch (because it is binary). Not sure
> > if it is necessary to upload the whole file to cr.openjdk.java.net as
> > well? If you say that sun/security/lib/cacerts/VerifyCACerts.java and
> > security/infra/java/security/cert/CertPathValidator/certification both
> > pass, then everything seems to be fine.
> 
> FWIW the raw download of the webrev's cacerts file should have the
> binary blob:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> 8233223/jdk8/01/webrev/raw_files/new/src/share/lib/security/cacerts
> 
> And for 8232019:
> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> 8232019/jdk8/01/webrev/raw_files/new/src/share/lib/security/cacerts
> 
> Thanks,
> Severin
> 
> > So thumbs up from me (for both, this one and 8232019).
> >
> > Best regards,
> > Volker
> >
> > On Tue, Dec 17, 2019 at 8:39 PM Severin Gehwolf 
> wrote:
> > > Hi,
> > >
> > > Could I please get a review of this OpenJDK 8u backport of 8233223
> > > which depends on 8u backport of 8232019[1]. The JDK 11u patch did not
> > > apply cleanly for a couple of reasons:
> > >
> > >1. 8u still has the binary blob for cacerts (JDK-8193255
> > >   not backported, yet). Instead, I've updated to the revision in
> > >   jdk11u, performed a build and copied the cacerts binary to 8u.
> > >2. JDK-8225392 not present in 8u, which added the checksum to
> > >   VerifyCACerts.java. Thus, the 8u backport does not include
> > >   this hunk.
> > >3. JDK-8234245 not present in 8u.
> > >4. Due to 2) and 3) above @bug annotation modified manually for these
> > >   reasons.
> > >
> > > Everything else is the same.
> > >
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8233223
> > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> 8233223/jdk8/01/webrev/
> > >
> > > Testing: sun/security/lib/cacerts/VerifyCACerts.java and
> > >  security/infra/java/security/cert/CertPathValidator/certification
> > >  Pass, except for ActalisCA.java which is problem-listed and still
> > >  broken in HEAD (JDK-8224768)
> > >
> > > Thoughts?
> > >
> > > Once reviewed, I'll try to get this into 8u242 via the critical fix
> > > request label workflow.
> > >
> > > Thanks,
> > > Severin
> > >
> > > [1] http://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-
> December/010813.html
> > >

RE: [8u] RFR: 8232019: Add LuxTrust certificate updates to the existing root program

[Langer, Christoph]

Hi Severin,

this looks good - when VerifyCACerts passes, everything is correct.

We shall definitely try to backport "JDK-8193255: Root Certificates should be 
stored in text format and assembled at build time" somehow, to have easier 
certificate backports.

Cheers
Christoph

> -Original Message-
> From: jdk8u-dev  On Behalf Of
> Severin Gehwolf
> Sent: Dienstag, 17. Dezember 2019 20:30
> To: jdk8u-dev 
> Cc: security-dev 
> Subject: [8u] RFR: 8232019: Add LuxTrust certificate updates to the existing
> root program
> 
> Hi,
> 
> Could I please get a review of this OpenJDK 8u backport of 8232019. The
> JDK 11 patch did not apply cleanly for a couple of reasons:
> 
>1. 8u still has the binary blob for cacerts (JDK-8193255 not
>   backported, yet). Instead, I've updated to the revision in jdk11u,
>   performed a build and copied the cacerts binary to 8u.
>2. JDK-8225392 not present in 8u, which added the checksum to
>   VerifyCACerts.java. Thus, the 8u backport does not include this
>   hunk. @bug annotation modified manually for the same reason.
> 
> Everything else is the same.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8232019
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> 8232019/jdk8/01/webrev/
> 
> Testing: sun/security/lib/cacerts/VerifyCACerts.java and
>  security/infra/java/security/cert/CertPathValidator/certification
>  Pass, except for ActalisCA.java which is problem-listed and still
>  broken in HEAD (JDK-8224768)
> 
> Thoughts?
> 
> If reviewed, I'll try to get this in 8u242 via the critical fix request
> label workflow.
> 
> Thanks,
> Severin

RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

[Langer, Christoph]

Hi Martin,

ok, you are the author – so I won’t insist. 😊

Best regards
Christoph

From: Doerr, Martin 
Sent: Freitag, 6. Dezember 2019 12:22
To: Langer, Christoph 
Cc: core-libs-...@openjdk.java.net; security-dev 
; Lindenmaier, Goetz 
; Thomas Stüfe 
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Christoph,

that would work, but I don’t want to pollute this file with compiler specific 
defines. In addition, I don’t like introducing a macro which works on some 
platforms and does nothing on other ones (which is the case for hotspot’s 
ATTRIBUTE_ALIGNED).

Because Windows 32 bit is the only affected platform, I prefer not to touch 
other ones. Are you ok with webrev.00 as it is?

Best regards,
Martin



From: Langer, Christoph 
mailto:christoph.lan...@sap.com>>
Sent: Donnerstag, 5. Dezember 2019 12:16
To: Doerr, Martin mailto:martin.do...@sap.com>>
Cc: core-libs-...@openjdk.java.net<mailto:core-libs-...@openjdk.java.net>; 
security-dev 
mailto:security-dev@openjdk.java.net>>; 
Lindenmaier, Goetz 
mailto:goetz.lindenma...@sap.com>>; Thomas Stüfe 
mailto:thomas.stu...@gmail.com>>
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Martin,

I can see that both places already include headers from 
src/java.base/share/native/libjava/. I suggest adding the define in jni_util.h. 
WindowsPreferences.c already includes it.

Best regards
Christoph

From: Doerr, Martin mailto:martin.do...@sap.com>>
Sent: Donnerstag, 5. Dezember 2019 12:00
To: Thomas Stüfe mailto:thomas.stu...@gmail.com>>; 
Langer, Christoph mailto:christoph.lan...@sap.com>>
Cc: core-libs-...@openjdk.java.net<mailto:core-libs-...@openjdk.java.net>; 
security-dev 
mailto:security-dev@openjdk.java.net>>; 
Lindenmaier, Goetz mailto:goetz.lindenma...@sap.com>>
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Thomas and Christoph,

thanks for the reviews.

Other code in java.security.jgss also uses these #defined checks:
src/java.security.jgss/share/native/libj2gss/gssapi.h:#if defined (_WIN32) && 
defined (_MSC_VER)

I’d like to have it consistent with that.

@Christoph: I think having ATTRIBUTE_ALIGNED(x) would be nice. It could get 
defined easily for Visual Studio and GCC, but some other compilers may be more 
difficult. Note that this macro is only defined for a selected set of compilers 
in hotspot. If we wanted to add it, where should we define it?

Windows 32 bit seems to be the only platform which is affected by the problem 
that jlongs on stack are not 8 byte aligned.
(AFAIK, GCC uses -malign-double by default on 32 bit which should do the job 
for jlongs, too:
https://gcc.gnu.org/onlinedocs/gcc-4.5.3/gcc/i386-and-x86_002d64-Options.html)

Best regards,
Martin


From: Thomas Stüfe mailto:thomas.stu...@gmail.com>>
Sent: Mittwoch, 4. Dezember 2019 17:56
To: Doerr, Martin mailto:martin.do...@sap.com>>
Cc: core-libs-...@openjdk.java.net<mailto:core-libs-...@openjdk.java.net>; 
security-dev 
mailto:security-dev@openjdk.java.net>>; 
Lindenmaier, Goetz mailto:goetz.lindenma...@sap.com>>
Subject: Re: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Martin,

this makes sense. This is the right way to force alignment. I do not like the 
platform code in the shared file but do not think this is a big deal.

+#if defined (_WIN32) && defined (_MSC_VER)

Why do you think we need _MSC_VER too? Is OpenJDK on Windows even buildable 
with anything other than VC++?

Cheers, Thomas

On Mon, Dec 2, 2019 at 4:14 PM Doerr, Martin 
mailto:martin.do...@sap.com>> wrote:
Hi,

I'd like to propose a fix for an old issue on 32 bit Windows (also for an 11u 
backport):
https://bugs.openjdk.java.net/browse/JDK-8220348

Some jdk native methods use jni_SetLongArrayRegion with a stack allocated 
buffer.
jni_SetLongArrayRegion uses Copy::conjoint_jlongs_atomic which requires jlongs 
to be 8 byte aligned (asserted).
However, Windows 32 bit only uses 4 byte alignment for jlong arrays by default.
I found such issues in the following files:
src/java.prefs/windows/native/libprefs/WindowsPreferences.c
src/java.security.jgss/share/native/libj2gss/GSSLibStub.c
I suggest to use __declspec(align(8)) there.

Webrev:
http://cr.openjdk.java.net/~mdoerr/8220348_ntintel_stack_align/webrev.00/
Please review.

I think using 8 byte alignment is not a disadvantage for 64 bit.

I guess there are still people interested in this platform with jdk14. 
Otherwise I could contribute it as 11u only fix.

Is there a better way to force 8 byte alignment for jlongs or jlong arrays on 
stack?
Best regards,
Martin

RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

[Langer, Christoph]

Hi Martin,

I can see that both places already include headers from 
src/java.base/share/native/libjava/. I suggest adding the define in jni_util.h. 
WindowsPreferences.c already includes it.

Best regards
Christoph

From: Doerr, Martin 
Sent: Donnerstag, 5. Dezember 2019 12:00
To: Thomas Stüfe ; Langer, Christoph 

Cc: core-libs-...@openjdk.java.net; security-dev 
; Lindenmaier, Goetz 
Subject: RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Thomas and Christoph,

thanks for the reviews.

Other code in java.security.jgss also uses these #defined checks:
src/java.security.jgss/share/native/libj2gss/gssapi.h:#if defined (_WIN32) && 
defined (_MSC_VER)

I’d like to have it consistent with that.

@Christoph: I think having ATTRIBUTE_ALIGNED(x) would be nice. It could get 
defined easily for Visual Studio and GCC, but some other compilers may be more 
difficult. Note that this macro is only defined for a selected set of compilers 
in hotspot. If we wanted to add it, where should we define it?

Windows 32 bit seems to be the only platform which is affected by the problem 
that jlongs on stack are not 8 byte aligned.
(AFAIK, GCC uses -malign-double by default on 32 bit which should do the job 
for jlongs, too:
https://gcc.gnu.org/onlinedocs/gcc-4.5.3/gcc/i386-and-x86_002d64-Options.html)

Best regards,
Martin


From: Thomas Stüfe mailto:thomas.stu...@gmail.com>>
Sent: Mittwoch, 4. Dezember 2019 17:56
To: Doerr, Martin mailto:martin.do...@sap.com>>
Cc: core-libs-...@openjdk.java.net<mailto:core-libs-...@openjdk.java.net>; 
security-dev 
mailto:security-dev@openjdk.java.net>>; 
Lindenmaier, Goetz mailto:goetz.lindenma...@sap.com>>
Subject: Re: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

Hi Martin,

this makes sense. This is the right way to force alignment. I do not like the 
platform code in the shared file but do not think this is a big deal.

+#if defined (_WIN32) && defined (_MSC_VER)

Why do you think we need _MSC_VER too? Is OpenJDK on Windows even buildable 
with anything other than VC++?

Cheers, Thomas

On Mon, Dec 2, 2019 at 4:14 PM Doerr, Martin 
mailto:martin.do...@sap.com>> wrote:
Hi,

I'd like to propose a fix for an old issue on 32 bit Windows (also for an 11u 
backport):
https://bugs.openjdk.java.net/browse/JDK-8220348

Some jdk native methods use jni_SetLongArrayRegion with a stack allocated 
buffer.
jni_SetLongArrayRegion uses Copy::conjoint_jlongs_atomic which requires jlongs 
to be 8 byte aligned (asserted).
However, Windows 32 bit only uses 4 byte alignment for jlong arrays by default.
I found such issues in the following files:
src/java.prefs/windows/native/libprefs/WindowsPreferences.c
src/java.security.jgss/share/native/libj2gss/GSSLibStub.c
I suggest to use __declspec(align(8)) there.

Webrev:
http://cr.openjdk.java.net/~mdoerr/8220348_ntintel_stack_align/webrev.00/
Please review.

I think using 8 byte alignment is not a disadvantage for 64 bit.

I guess there are still people interested in this platform with jdk14. 
Otherwise I could contribute it as 11u only fix.

Is there a better way to force 8 byte alignment for jlongs or jlong arrays on 
stack?
Best regards,
Martin

RE: RFR(S): 8220348: [ntintel] asserts about copying unalinged array

[Langer, Christoph]

Hi Martin,

thanks for looking into this and coming up with this patch. The test failures 
were quite annoying 😊

In hotspot, there is coding to define a macro "ATTRIBUTE_ALIGNED(x)". I'd 
rather like to see that we define such a macro in the JDK code as well and use 
it. I think it would make the code more readable. Other than that, +1

Cheers
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Doerr, Martin
> Sent: Montag, 2. Dezember 2019 16:13
> To: core-libs-...@openjdk.java.net; security-dev  d...@openjdk.java.net>
> Cc: Lindenmaier, Goetz 
> Subject: [CAUTION] RFR(S): 8220348: [ntintel] asserts about copying
> unalinged array
> 
> Hi,
> 
> I'd like to propose a fix for an old issue on 32 bit Windows (also for an 11u
> backport):
> https://bugs.openjdk.java.net/browse/JDK-8220348
> 
> Some jdk native methods use jni_SetLongArrayRegion with a stack allocated
> buffer.
> jni_SetLongArrayRegion uses Copy::conjoint_jlongs_atomic which requires
> jlongs to be 8 byte aligned (asserted).
> However, Windows 32 bit only uses 4 byte alignment for jlong arrays by
> default.
> I found such issues in the following files:
> src/java.prefs/windows/native/libprefs/WindowsPreferences.c
> src/java.security.jgss/share/native/libj2gss/GSSLibStub.c
> I suggest to use __declspec(align(8)) there.
> 
> Webrev:
> http://cr.openjdk.java.net/~mdoerr/8220348_ntintel_stack_align/webrev.0
> 0/
> Please review.
> 
> I think using 8 byte alignment is not a disadvantage for 64 bit.
> 
> I guess there are still people interested in this platform with jdk14. 
> Otherwise
> I could contribute it as 11u only fix.
> 
> Is there a better way to force 8 byte alignment for jlongs or jlong arrays on
> stack?
> Best regards,
> Martin

RE: RFR 8233946: Add @since 13 annotation to KerberosPrincipal.KRB_NT_ENTERPRISE field

[Langer, Christoph]

Hi Martin,

looks good.

Best regards
Christoph

> -Original Message-
> From: security-dev  On Behalf Of
> Martin Balao
> Sent: Montag, 11. November 2019 23:35
> To: security-dev ; Sean Mullan
> 
> Subject: RFR 8233946: Add @since 13 annotation to
> KerberosPrincipal.KRB_NT_ENTERPRISE field
> 
> Hi,
> 
> I'd like to request a review for 8233946 [1].
> 
> Webrev.00:
> 
>  *
> http://cr.openjdk.java.net/~mbalao/webrevs/8233946/8233946.webrev.00/
> 
> Thanks,
> Martin.-
> 
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8233946

RE: CSR Review request (11-pool): JDK-8233825: Update SunPKCS11 provider with PKCS11 v2.40 support

[Langer, Christoph]

That's great, thanks Sean.

I'll take care of backporting the other items as well.

/Christoph

From: Seán Coffey 
Sent: Freitag, 8. November 2019 17:03
To: Langer, Christoph ; Sean Mullan 
; Valerie Peng ; OpenJDK Dev 
list 
Cc: Joe Darcy ; jdk-updates-...@openjdk.java.net
Subject: Re: CSR Review request (11-pool): JDK-8233825: Update SunPKCS11 
provider with PKCS11 v2.40 support


Hey Christoph,

I've added myself as reviewer for this CSR. Hope that's ok.

There was a bug tail with this PKCS11 upgrade (interoperability issues due to 
ambiguity in the spec)
See JDK-8229243 also (and the JDK-8225695 regression)

Regards,

Sean.
On 07/11/19 22:19, Langer, Christoph wrote:
Hi Valerie, Sean,

may I please ask you to add yourself as reviewer to the backport CSR 
JDK-8233825: "Update SunPKCS11 provider with PKCS11 v2.40 support" [0]. It is a 
CSR for backporting JDK-8080462 to OpenJKD 11u. Oracle did that already for 
11.0.6 but with the internal ccc process. Joe indicated, though, that it should 
be clearer to do an 11u backport CSR still for OpenJDK. I've linked the 
backport CSR to the Oracle 11.0.6 backport as well.

Thanks
Christoph

[0] https://bugs.openjdk.java.net/browse/JDK-8233825
[1] https://bugs.openjdk.java.net/browse/JDK-8221442

CSR Review request (11-pool): JDK-8233825: Update SunPKCS11 provider with PKCS11 v2.40 support

[Langer, Christoph]

Hi Valerie, Sean,

may I please ask you to add yourself as reviewer to the backport CSR 
JDK-8233825: "Update SunPKCS11 provider with PKCS11 v2.40 support" [0]. It is a 
CSR for backporting JDK-8080462 to OpenJKD 11u. Oracle did that already for 
11.0.6 but with the internal ccc process. Joe indicated, though, that it should 
be clearer to do an 11u backport CSR still for OpenJDK. I've linked the 
backport CSR to the Oracle 11.0.6 backport as well.

Thanks
Christoph

[0] https://bugs.openjdk.java.net/browse/JDK-8233825
[1] https://bugs.openjdk.java.net/browse/JDK-8221442

RE: [11u] RFR 8215032: Support Kerberos cross-realm referrals (RFC 6806)

[Langer, Christoph]

Hi Martin,

Sorry for the late reply... been rather busy the last days.

You're asking me to review the CSR for backporting JDK8215032. I, however, am 
not an expert in that area and could probably only check whether you copied the 
content correctly from the original CSR. So, I'd like to refer this to the 
original CSR reviewer.

@Weijun Wang, can you please have a look at CSR 
https://bugs.openjdk.java.net/browse/JDK-8230496 and add yourself as reviewer 
once you find everything is ok and it's a thing feasible to backport to JDK11?

Thanks
Christoph

> -Original Message-
> From: Martin Balao 
> Sent: Montag, 21. Oktober 2019 22:56
> To: Langer, Christoph ; jdk-updates-
> d...@openjdk.java.net
> Subject: Re: [11u] RFR 8215032: Support Kerberos cross-realm referrals (RFC
> 6806)
> 
> Hi Christoph,
> 
> Can you please have a look at the CSR [1] so we move it from Provisional
> to Finalized?
> 
> Now that 8224589 is in 11u, patch applies cleanly net copyright.
> 
> Thanks,
> Martin.-
> 
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8230496

RE: RFR: 8231887: ComodoCA.java fails because certificate was revoked

[Langer, Christoph]

+1

Thanks,
Christoph

> -Original Message-
> From: Sean Mullan 
> Sent: Mittwoch, 9. Oktober 2019 20:58
> To: Rajan Halade ; security-
> d...@openjdk.java.net
> Cc: Langer, Christoph 
> Subject: Re: RFR: 8231887: ComodoCA.java fails because certificate was
> revoked
> 
> You should add the bugid to @bug and it probably should have a
> noreg-self label. Looks good otherwise.
> 
> --Sean
> 
> On 10/9/19 1:56 PM, Rajan Halade wrote:
> > Please review this fix in ComodoCA.java test to update test certificates
> > used. CA revoked earlier test artifacts so we need to update those.
> >
> > Webrev: http://cr.openjdk.java.net/~rhalade/8231887/webrev.00/
> >
> > Thanks,
> > Rajan
> >

RE: Re: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java fails on SLES11 using mozilla-nss-3.14

[Langer, Christoph]

Hi Matthias,

looks good.

One minor thing I just recognized: java.security.Provider::getVersion() is 
deprecated. Can you please use p.getVersionStr in line 323? But no need to see 
another webrev 😊

Best regards
Christoph

From: Baesken, Matthias 
Sent: Mittwoch, 25. September 2019 16:34
To: Valerie Peng ; security-dev@openjdk.java.net
Cc: Langer, Christoph 
Subject: RE: Re: RFR [XS] 8231357: 
sun/security/pkcs11/Cipher/TestKATForGCM.java fails on SLES11 using 
mozilla-nss-3.14

Hi Valerie /  Christoph  ,

New webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8231357.2/


  *   Followed  the idea of Christoph : “One minor style nit: You could close 
the else case in line 335 with “}” and then have just one throw e;”
  *   Moved the isNSS(Provider)   check in front of the version  checking
and potential output

Best regards, Matthias



Hi Matthias,

The output on line 324 may report inconsistent info - the provider in use may 
not be NSS but yet you put NSS version with provider name?

Since you are updating the code here, how about re-org the code a bit and first 
check for whether NSS provider is used then do the various version-specific 
handling.
Regards,

Valerie


On 9/24/2019 5:39 AM, Langer, Christoph wrote:
Hi Matthias,

looks good to me.

One minor style nit: You could close the else case in line 335 with “}” and 
then have just one throw e; after the whole if else block.

Best regards
Christoph

From: Baesken, Matthias 
<mailto:matthias.baes...@sap.com>
Sent: Dienstag, 24. September 2019 10:30
To: Langer, Christoph 
<mailto:christoph.lan...@sap.com>; 
security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>
Cc: Zeller, Arno <mailto:arno.zel...@sap.com>
Subject: RE: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java 
fails on SLES11 using mozilla-nss-3.14

New webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8231357.1/

I adjusted the imports and  now output a warning .

Best regards, Matthias

From: Baesken, Matthias
Sent: Montag, 23. September 2019 16:07
To: Langer, Christoph 
mailto:christoph.lan...@sap.com>>; 
security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>
Cc: Zeller, Arno mailto:arno.zel...@sap.com>>
Subject: RE: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java 
fails on SLES11 using mozilla-nss-3.14

Hi Christoph,   I am okay  with your suggestion to   just  print  a  message  
in  case that  an  older nss lib  < 3.15 is found ;  however   let’s  see what 
others think .
(but for Solaris  we let the test pass when   noticing  old nss versions ,  so 
I thought  it might make some sense to behave on Linux in the same way )


Best regards, Matthias



From: Langer, Christoph 
mailto:christoph.lan...@sap.com>>
Sent: Montag, 23. September 2019 15:53
To: Baesken, Matthias 
mailto:matthias.baes...@sap.com>>; 
security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>
Cc: Zeller, Arno mailto:arno.zel...@sap.com>>
Subject: RE: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java 
fails on SLES11 using mozilla-nss-3.14

Hi Matthias,

generally I agree that it’s a good idea to have more diagnostic output in case 
of failures in this test.

But, given that there’s an upgrade path even on our old Linux SLES 11.3 system 
to a newer libnss that has a fix for the bug that we observe, I suggest that 
the test should still fail with libnss 3.14. So I suggest you only add the line
System.out.println("Exception occured using " + p.getName() + " version " + 
ver);
and maybe a comment stating that libnss 3.14 on Linux isn’t good for this test.

BTW, if you want to clean up the testcase a bit, you might remove line 36, 
import java.math.*; because it’s not needed. Or replace all the imports with:

import java.security.GeneralSecurityException;
import java.security.Provider;
import java.util.Arrays;

import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.SecretKeySpec;

Thanks
Christoph


From: Baesken, Matthias 
mailto:matthias.baes...@sap.com>>
Sent: Montag, 23. September 2019 15:16
To: security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>
Cc: Langer, Christoph 
mailto:christoph.lan...@sap.com>>; Zeller, Arno 
mailto:arno.zel...@sap.com>>
Subject: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java fails 
on SLES11 using mozilla-nss-3.14

Hello,  please review  this small test related change .

We  noticed that  on our SLES (SuSE Linux) 11   test  machines, the test

sun/security/pkcs11/Cipher/TestKATForGCM.java

fails when older nss versions  are used on the system , especially  nss 3.14 .

The used package is named   mozilla-nss-3.14 .
Upgrading to newer versions (e.g. 3.20)   makes the test succeed , so it might 
be helpful
to add a check in the test like it is done already for old nss versions on 
Sola

RE: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java fails on SLES11 using mozilla-nss-3.14

[Langer, Christoph]

Hi Matthias,

looks good to me.

One minor style nit: You could close the else case in line 335 with "}" and 
then have just one throw e; after the whole if else block.

Best regards
Christoph

From: Baesken, Matthias 
Sent: Dienstag, 24. September 2019 10:30
To: Langer, Christoph ; security-dev@openjdk.java.net
Cc: Zeller, Arno 
Subject: RE: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java 
fails on SLES11 using mozilla-nss-3.14

New webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8231357.1/

I adjusted the imports and  now output a warning .

Best regards, Matthias

From: Baesken, Matthias
Sent: Montag, 23. September 2019 16:07
To: Langer, Christoph 
mailto:christoph.lan...@sap.com>>; 
security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>
Cc: Zeller, Arno mailto:arno.zel...@sap.com>>
Subject: RE: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java 
fails on SLES11 using mozilla-nss-3.14

Hi Christoph,   I am okay  with your suggestion to   just  print  a  message  
in  case that  an  older nss lib  < 3.15 is found ;  however   let's  see what 
others think .
(but for Solaris  we let the test pass when   noticing  old nss versions ,  so 
I thought  it might make some sense to behave on Linux in the same way )


Best regards, Matthias



From: Langer, Christoph 
mailto:christoph.lan...@sap.com>>
Sent: Montag, 23. September 2019 15:53
To: Baesken, Matthias 
mailto:matthias.baes...@sap.com>>; 
security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>
Cc: Zeller, Arno mailto:arno.zel...@sap.com>>
Subject: RE: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java 
fails on SLES11 using mozilla-nss-3.14

Hi Matthias,

generally I agree that it's a good idea to have more diagnostic output in case 
of failures in this test.

But, given that there's an upgrade path even on our old Linux SLES 11.3 system 
to a newer libnss that has a fix for the bug that we observe, I suggest that 
the test should still fail with libnss 3.14. So I suggest you only add the line
System.out.println("Exception occured using " + p.getName() + " version " + 
ver);
and maybe a comment stating that libnss 3.14 on Linux isn't good for this test.

BTW, if you want to clean up the testcase a bit, you might remove line 36, 
import java.math.*; because it's not needed. Or replace all the imports with:

import java.security.GeneralSecurityException;
import java.security.Provider;
import java.util.Arrays;

import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.SecretKeySpec;

Thanks
Christoph


From: Baesken, Matthias 
mailto:matthias.baes...@sap.com>>
Sent: Montag, 23. September 2019 15:16
To: security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>
Cc: Langer, Christoph 
mailto:christoph.lan...@sap.com>>; Zeller, Arno 
mailto:arno.zel...@sap.com>>
Subject: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java fails 
on SLES11 using mozilla-nss-3.14

Hello,  please review  this small test related change .

We  noticed that  on our SLES (SuSE Linux) 11   test  machines, the test

sun/security/pkcs11/Cipher/TestKATForGCM.java

fails when older nss versions  are used on the system , especially  nss 3.14 .

The used package is named   mozilla-nss-3.14 .
Upgrading to newer versions (e.g. 3.20)   makes the test succeed , so it might 
be helpful
to add a check in the test like it is done already for old nss versions on 
Solaris.

(nss  3.15  contains a couple  of  AES cipher with GCMrelated fixes, those 
might be the ones needed to run the test successfully ).



Bug/webrev :

https://bugs.openjdk.java.net/browse/JDK-8231357

http://cr.openjdk.java.net/~mbaesken/webrevs/8231357.0/


Thanks, Matthias

RE: RFR: 8231222: fix pkcs11 P11_DEBUG guarded native traces

[Langer, Christoph]

Hi Matthias,



your change looks good to me. Don't forget to update the copyright years before 
pushing.



Best regards

Christoph



From: security-dev  On Behalf Of 
Baesken, Matthias
Sent: Donnerstag, 19. September 2019 16:28
To: security-dev@openjdk.java.net
Subject: [CAUTION] RFR: 8231222: fix pkcs11 P11_DEBUG guarded native traces



Hello, please reviews this fix of the  pkcs11  native  tracing( P11_DEBUG 
guarded native traces ).

I had enabled it  to look into   a issue  with pkcs11 not working on  a 
recently patched Linux system .

But noticed  that the traces did not work any more (probably mostly because  of 
  changed compilers  and related warning levels ).



With  this change I could  successfully   again  for run on Linux  with traces 
enabled .



(see pkcs11wrapper.h   P11_DEBUG )





Maybe it would make sense to always  compile  with tracing enabled , and make 
the trace  depend  on a system property, what do you think ?   (but this would 
be a separate change) .





Bug/webrev :



https://bugs.openjdk.java.net/browse/JDK-8231222



http://cr.openjdk.java.net/~mbaesken/webrevs/8231222.0/



Best regards, Matthias

RE: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java fails on SLES11 using mozilla-nss-3.14

[Langer, Christoph]

Hi Matthias,



generally I agree that it's a good idea to have more diagnostic output in case 
of failures in this test.



But, given that there's an upgrade path even on our old Linux SLES 11.3 system 
to a newer libnss that has a fix for the bug that we observe, I suggest that 
the test should still fail with libnss 3.14. So I suggest you only add the line

System.out.println("Exception occured using " + p.getName() + " version " + 
ver);

and maybe a comment stating that libnss 3.14 on Linux isn't good for this test.



BTW, if you want to clean up the testcase a bit, you might remove line 36, 
import java.math.*; because it's not needed. Or replace all the imports with:



import java.security.GeneralSecurityException;

import java.security.Provider;

import java.util.Arrays;



import javax.crypto.Cipher;

import javax.crypto.SecretKey;

import javax.crypto.spec.GCMParameterSpec;

import javax.crypto.spec.SecretKeySpec;



Thanks

Christoph



From: Baesken, Matthias 
Sent: Montag, 23. September 2019 15:16
To: security-dev@openjdk.java.net
Cc: Langer, Christoph ; Zeller, Arno 

Subject: RFR [XS] 8231357: sun/security/pkcs11/Cipher/TestKATForGCM.java fails 
on SLES11 using mozilla-nss-3.14



Hello,  please review  this small test related change .



We  noticed that  on our SLES (SuSE Linux) 11   test  machines, the test

sun/security/pkcs11/Cipher/TestKATForGCM.java

fails when older nss versions  are used on the system , especially  nss 3.14 .


The used package is named   mozilla-nss-3.14 .
Upgrading to newer versions (e.g. 3.20)   makes the test succeed , so it might 
be helpful
to add a check in the test like it is done already for old nss versions on 
Solaris.



(nss  3.15  contains a couple  of  AES cipher with GCMrelated fixes, those 
might be the ones needed to run the test successfully ).







Bug/webrev :



https://bugs.openjdk.java.net/browse/JDK-8231357



http://cr.openjdk.java.net/~mbaesken/webrevs/8231357.0/





Thanks, Matthias

RE: [11u] RFR: 8204203: Many pkcs11 tests failed in Provider initialization, after compiler on Windows changed

[Langer, Christoph]

Hi,

since there’s apparently no veto on this (nor encouraging statements), I’ll go 
ahead and push this testfix soon. After all, it’ll just re-enable a few tests 
where I can’t see problems with. Should somebody run into issues with the tests 
because of this update, we can always disable them again.

Thanks
Christoph

From: Langer, Christoph
Sent: Mittwoch, 4. September 2019 17:00
To: Baesken, Matthias ; 
security-dev@openjdk.java.net; jdk-updates-...@openjdk.java.net; Aleksey 
Shipilev 
Subject: RE: [11u] RFR: 8204203: Many pkcs11 tests failed in Provider 
initialization, after compiler on Windows changed

Hi Matthias,

good point. What do e.g. Redhat folks say about VS2017 requirement? (@Aleksey)

Or will the test also work with these artifacts if using a VS < 2017 to build 
the JDK? (Maybe somebody on security-dev with some experience with these tests 
can shed some light on this…)

Thanks
Christoph

From: Baesken, Matthias 
mailto:matthias.baes...@sap.com>>
Sent: Mittwoch, 4. September 2019 16:54
To: Langer, Christoph 
mailto:christoph.lan...@sap.com>>; 
security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>; 
jdk-updates-...@openjdk.java.net<mailto:jdk-updates-...@openjdk.java.net>
Subject: RE: [11u] RFR: 8204203: Many pkcs11 tests failed in Provider 
initialization, after compiler on Windows changed


Hello Christoph,  the change looks good to me .

However I suggest to wait a day or two  for more feedback ,  because

http://cr.openjdk.java.net/~clanger/webrevs/8204203.11u.0/test/jdk/sun/security/pkcs11/PKCS11Test.java.frames.html


889 @Artifact(
890 organization = "jpg.tests.jdk.nsslib",
891 name = "nsslib-windows_x64",
892 revision = "3.41-VS2017",
893 extension = "zip")
894 private static class WINDOWS_X64 { }
895
 896 @Artifact(
897 organization = "jpg.tests.jdk.nsslib",
898 name = "nsslib-windows_x86",
899 revision = "3.41-VS2017",
900 extension = "zip")


… looks likeeveryone executing these tests  on Windows  needs  the VS2017  
runtimes , isn’t it ?
Is this really true for  at least the main parties working on   jdk11 ?

In  current  11u-dev  I find :

toolchain_windows.m4:28:VALID_VS_VERSIONS="2017 2013 2015 2012 2010"

Best regards, Matthias



From: Langer, Christoph
Sent: Mittwoch, 4. September 2019 08:24
To: jdk-updates-...@openjdk.java.net<mailto:jdk-updates-...@openjdk.java.net>
Cc: security-dev 
mailto:security-dev@openjdk.java.net>>
Subject: [11u] RFR: 8204203: Many pkcs11 tests failed in Provider 
initialization, after compiler on Windows changed

Hi,

please review the backport of this testfix to JDK11 updates.

Bug: https://bugs.openjdk.java.net/browse/JDK-8204203
Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/251090f84412
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8204203.11u.0/

The backport differs in that sun/security/tools/keytool/NssTest.java doesn’t 
exist in 11u and hence the changes for it were omitted.

Jtreg testing over all platforms, especially Windows, shows no regressions.

Thanks
Christoph

RE: [11u] RFR: 8204203: Many pkcs11 tests failed in Provider initialization, after compiler on Windows changed

[Langer, Christoph]

Hi Matthias,

good point. What do e.g. Redhat folks say about VS2017 requirement? (@Aleksey)

Or will the test also work with these artifacts if using a VS < 2017 to build 
the JDK? (Maybe somebody on security-dev with some experience with these tests 
can shed some light on this…)

Thanks
Christoph

From: Baesken, Matthias 
Sent: Mittwoch, 4. September 2019 16:54
To: Langer, Christoph ; 
security-dev@openjdk.java.net; jdk-updates-...@openjdk.java.net
Subject: RE: [11u] RFR: 8204203: Many pkcs11 tests failed in Provider 
initialization, after compiler on Windows changed


Hello Christoph,  the change looks good to me .

However I suggest to wait a day or two  for more feedback ,  because

http://cr.openjdk.java.net/~clanger/webrevs/8204203.11u.0/test/jdk/sun/security/pkcs11/PKCS11Test.java.frames.html


889 @Artifact(
890 organization = "jpg.tests.jdk.nsslib",
891 name = "nsslib-windows_x64",
892 revision = "3.41-VS2017",
893 extension = "zip")
894 private static class WINDOWS_X64 { }
895
 896 @Artifact(
897 organization = "jpg.tests.jdk.nsslib",
898 name = "nsslib-windows_x86",
899 revision = "3.41-VS2017",
900 extension = "zip")


… looks likeeveryone executing these tests  on Windows  needs  the VS2017  
runtimes , isn’t it ?
Is this really true for  at least the main parties working on   jdk11 ?

In  current  11u-dev  I find :

toolchain_windows.m4:28:VALID_VS_VERSIONS="2017 2013 2015 2012 2010"

Best regards, Matthias



From: Langer, Christoph
Sent: Mittwoch, 4. September 2019 08:24
To: jdk-updates-...@openjdk.java.net<mailto:jdk-updates-...@openjdk.java.net>
Cc: security-dev 
mailto:security-dev@openjdk.java.net>>
Subject: [11u] RFR: 8204203: Many pkcs11 tests failed in Provider 
initialization, after compiler on Windows changed

Hi,

please review the backport of this testfix to JDK11 updates.

Bug: https://bugs.openjdk.java.net/browse/JDK-8204203
Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/251090f84412
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8204203.11u.0/

The backport differs in that sun/security/tools/keytool/NssTest.java doesn’t 
exist in 11u and hence the changes for it were omitted.

Jtreg testing over all platforms, especially Windows, shows no regressions.

Thanks
Christoph

[11u] RFR: 8204203: Many pkcs11 tests failed in Provider initialization, after compiler on Windows changed

[Langer, Christoph]

Hi,

please review the backport of this testfix to JDK11 updates.

Bug: https://bugs.openjdk.java.net/browse/JDK-8204203
Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/251090f84412
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8204203.11u.0/

The backport differs in that sun/security/tools/keytool/NssTest.java doesn't 
exist in 11u and hence the changes for it were omitted.

Jtreg testing over all platforms, especially Windows, shows no regressions.

Thanks
Christoph

[11u] RFR(XS): 8224991: Problemlist javax/net/ssl/ServerName/SSLEngineExplorerMatchedSNI.java

[Langer, Christoph]

Hi,

please review the problemlisting of 
javax/net/ssl/ServerName/SSLEngineExplorerMatchedSNI.java in jdk11u. There's an 
issue with the test, tracked by JDK-8212096. We see this issue in 11u testing, 
too. In jdk/jdk resp. jdk/jdk13, the test is excluded, so I think we should 
exclude it in 11u as well. Since the exclusion lists diverged, I had to resolve 
the patch manually.

Bug: https://bugs.openjdk.java.net/browse/JDK-8224991
Original change: http://hg.openjdk.java.net/jdk/jdk/rev/3e5dba06a663
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8224991.11u-dev.0/

Thanks
Christoph

RE: [11u] RFR: 8216039: TLS with BC and RSASSA-PSS breaks ECDHServerKeyExchange

[Langer, Christoph]

Ping...

Can somebody please have a look at this backport? Regression testing shows no 
problems...

Thanks
Christoph

From: Langer, Christoph
Sent: Donnerstag, 4. Juli 2019 15:11
To: jdk-updates-...@openjdk.java.net
Cc: security-dev 
Subject: [11u] RFR: 8216039: TLS with BC and RSASSA-PSS breaks 
ECDHServerKeyExchange

Hi,

please help reviewing the backport of JDK-8216039 to jdk11u-dev.

Since predecessor patch JDK-8211122 could not be applied to JDK 11 updates, 
some manual work is necessary.

In src/java.base/share/classes/java/security/Signature.java and 
src/java.base/share/classes/sun/security/util/SignatureUtil.java the imports of 
jdk.internal.access have to be changed into jdk.internal.misc. The update that 
originally went to 
src/java.base/share/classes/jdk/internal/access/SharedSecrets.java obviously 
needs to be applied to 
src/java.base/share/classes/jdk/internal/misc/SharedSecrets.java. The new file 
src/java.base/share/classes/jdk/internal/access/JavaSecuritySignatureAccess.java
 needs to be 
src/java.base/share/classes/jdk/internal/misc/JavaSecuritySignatureAccess.java 
in 11u.

See the full webrev here: 
http://cr.openjdk.java.net/~clanger/webrevs/8216039.11u.full.0/
The webrev for manual changes only: 
http://cr.openjdk.java.net/~clanger/webrevs/8216039.11u.manual.0/
Original Bug: https://bugs.openjdk.java.net/browse/JDK-8216039

Thanks
Christoph

RE: [11u] RFR: 8216039: TLS with BC and RSASSA-PSS breaks ECDHServerKeyExchange

[Langer, Christoph]

Ping...

would somebody please eyeball this backport? No regressions seen in testing...

Thanks
Christoph

From: Langer, Christoph
Sent: Donnerstag, 4. Juli 2019 15:11
To: jdk-updates-...@openjdk.java.net
Cc: security-dev 
Subject: [11u] RFR: 8216039: TLS with BC and RSASSA-PSS breaks 
ECDHServerKeyExchange

Hi,

please help reviewing the backport of JDK-8216039 to jdk11u-dev.

Since predecessor patch JDK-8211122 could not be applied to JDK 11 updates, 
some manual work is necessary.

In src/java.base/share/classes/java/security/Signature.java and 
src/java.base/share/classes/sun/security/util/SignatureUtil.java the imports of 
jdk.internal.access have to be changed into jdk.internal.misc. The update that 
originally went to 
src/java.base/share/classes/jdk/internal/access/SharedSecrets.java obviously 
needs to be applied to 
src/java.base/share/classes/jdk/internal/misc/SharedSecrets.java. The new file 
src/java.base/share/classes/jdk/internal/access/JavaSecuritySignatureAccess.java
 needs to be 
src/java.base/share/classes/jdk/internal/misc/JavaSecuritySignatureAccess.java 
in 11u.

See the full webrev here: 
http://cr.openjdk.java.net/~clanger/webrevs/8216039.11u.full.0/
The webrev for manual changes only: 
http://cr.openjdk.java.net/~clanger/webrevs/8216039.11u.manual.0/
Original Bug: https://bugs.openjdk.java.net/browse/JDK-8216039

Thanks
Christoph

[11u] RFR: 8216039: TLS with BC and RSASSA-PSS breaks ECDHServerKeyExchange

[Langer, Christoph]

Hi,

please help reviewing the backport of JDK-8216039 to jdk11u-dev.

Since predecessor patch JDK-8211122 could not be applied to JDK 11 updates, 
some manual work is necessary.

In src/java.base/share/classes/java/security/Signature.java and 
src/java.base/share/classes/sun/security/util/SignatureUtil.java the imports of 
jdk.internal.access have to be changed into jdk.internal.misc. The update that 
originally went to 
src/java.base/share/classes/jdk/internal/access/SharedSecrets.java obviously 
needs to be applied to 
src/java.base/share/classes/jdk/internal/misc/SharedSecrets.java. The new file 
src/java.base/share/classes/jdk/internal/access/JavaSecuritySignatureAccess.java
 needs to be 
src/java.base/share/classes/jdk/internal/misc/JavaSecuritySignatureAccess.java 
in 11u.

See the full webrev here: 
http://cr.openjdk.java.net/~clanger/webrevs/8216039.11u.full.0/
The webrev for manual changes only: 
http://cr.openjdk.java.net/~clanger/webrevs/8216039.11u.manual.0/
Original Bug: https://bugs.openjdk.java.net/browse/JDK-8216039

Thanks
Christoph

RE: [11u]: RFR: Backport of 8215694: keytool cannot generate RSASSA-PSS certificates

[Langer, Christoph]

Hi Paul,

thanks for your review.

> In CertAndKeyGen.java, does generate() need a throws declaration?

It doesn't. IllegalArgumentException is a RuntimeException and as such doesn't 
need a throws declaration. And InvalidKeyException is obviously not needed and 
was removed in the original changeset as well.

> Otherwise looks good.

Thanks. I also asked Max Wang to have a look off list and he seems to agree as 
well.

> We've been talking about backporting patches with CSRs and have done at
> least one. Imo, 8076190 and 8213400 are good backport candidates since the
> spec changes are minor.

Yes, a CSR is not necessarily a showstopper for a backport. But as it is not my 
area of expertise and there's no other reason that makes backports of these 
bugs important to me, I don't want to take the additional work and 
responsibility for these backports. But feel free to do this 😊

So, I guess I'm good to push this, once approved.

Cheers,
Christoph

RE: [11u]: RFR: Backport of 8215694: keytool cannot generate RSASSA-PSS certificates

[Langer, Christoph]

Hi again,

I had to make some additions to get the test 
sun/security/tools/keytool/PSS.java to work.

Firstly, I had to include the testlibrary utility class 
'test/lib/jdk/test/lib/security/DerUtils.java' from the change for JDK-8076190. 
Then I had to add some code to 
src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java from 
JDK-8213400 to tolerate a keyBits value of -1. This is exercised in the PSS 
test when keytool is called with "-genkeypair -keyalg RSASSA-PSS -sigalg 
RSASSA-PSS" without specifying the -keysize parameter.

Backporting JDK-8076190 or JDK-8213400 over to JDK11 is not possible due to 
their nature (CSR attached, behavioral change).

The webrevs were updated in-place:

http://cr.openjdk.java.net/~clanger/webrevs/8215694.11u.full.0/
http://cr.openjdk.java.net/~clanger/webrevs/8215694.11u.manual.0/


/Christoph

> -Original Message-
> From: jdk-updates-dev  On
> Behalf Of Langer, Christoph
> Sent: Mittwoch, 26. Juni 2019 17:30
> To: jdk-updates-...@openjdk.java.net
> Cc: security-dev 
> Subject: [CAUTION] [11u]: RFR: Backport of 8215694: keytool cannot
> generate RSASSA-PSS certificates
> 
> Hi,
> 
> please help reviewing the backport of JDK- 8215694: keytool cannot generate
> RSASSA-PSS certificates. The patch doesn't apply cleanly but the rejects are
> only minor. The Item is needed as prerequisite to apply JDK-8216039.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8215694
> Original Change: http://hg.openjdk.java.net/jdk/jdk12/rev/bdb29aa5fd31
> Rejects when applying original change:
> http://cr.openjdk.java.net/~clanger/webrevs/8215694.rejects.patch
> Full Webrev:
> http://cr.openjdk.java.net/~clanger/webrevs/8215694.11u.full.0/
> Incremental Webrev of added modifications:
> http://cr.openjdk.java.net/~clanger/webrevs/8215694.11u.manual.0/
> 
> Thanks
> Christoph

[11u] RFR (S): 8226880: Backport of JDK-8208698 (Improved ECC Implementation) should not bring parts of JDK-8205476 (KeyAgreement#generateSecret is not reset for ECDH based algorithm)

[Langer, Christoph]

Hi,

I made a mistake when bringing JDK-8226880 to 11u. The patch introduced coding 
of JDK-8205476 that should not be there. Here is a patch to fix this.

Bug: https://bugs.openjdk.java.net/browse/JDK-8226880
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8226880.11u.0/

As the backport is in 11.0.4, this fix needs to be applied there during closed 
rampdown.

Thanks
Christoph

RE: [11u] RFR: 8208698: Improved ECC Implementation

[Langer, Christoph]

Dang, you're right!

I'll open an issue and put it out for review. I guess you'll want to push it in 
your closed repository?

BTW: I just reported  another regression which affects 11.0.4 as well: 
https://bugs.openjdk.java.net/browse/JDK-8226876 It's "just" a Java level 
assert but maybe we'll need to take some (backout?) action here for 11.0.4, 
too...

Thanks
Christoph



> -Original Message-
> From: Andrew John Hughes 
> Sent: Donnerstag, 27. Juni 2019 06:09
> To: Langer, Christoph ; jdk-updates-
> d...@openjdk.java.net
> Cc: security-dev 
> Subject: Re: [11u] RFR: 8208698: Improved ECC Implementation
> 
> On 28/05/2019 08:21, Langer, Christoph wrote:
> > Hi,
> >
> > please review this backport of JDK-8208698: Improved ECC
> Implementation.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8208698
> > Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/752e57845ad2
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8208698.11u/
> >
> > The patch did not apply cleanly because there were conflicts in
> src/jdk.crypto.ec/share/classes/sun/security/ec/ECDHKeyAgreement.java
> due to JDK-8205476 which is part of jdk/jdk but not of jdk11u-dev.
> Unfortunately, JDK-8205476 can not be downported as prerequisite because
> it brings a behavioral change and is associated with a CSR. So I resolved the
> rejects manually. I add the rejects below.
> >
> > Thanks
> > Christoph
> >
> >
> 
> I'm just looking over this in backporting to 8u. You mention not being
> able to include the JDK-8205476 change, but then it is included in the
> patch.
> 
> JDK-8205476 is essentially:
> 
> @@ -127,7 +127,9 @@
> 
>  try {
> 
> -return deriveKey(s, publicValue, encodedParams);
> +byte[] result = deriveKey(s, publicValue, encodedParams);
> +publicValue = null;
> +return result;
> 
>  } catch (GeneralSecurityException e) {
>  throw new ProviderException("Could not derive key", e);
> 
> The same change is made in 11u by adopting the line in the 8208698 patch
> verbatim:
> 
> -try {
> -
> -return deriveKey(s, publicValue, encodedParams);
> -
> -} catch (GeneralSecurityException e) {
> -throw new ProviderException("Could not derive key", e);
> -}
> -
> +Optional resultOpt = deriveKeyImpl(privateKey, publicKey);
> +byte[] result = resultOpt.orElseGet(
> +() -> deriveKeyNative(privateKey, publicKey)
> +);
> +publicKey = null;
> +return result;
> 
> I think this should actually be:
> 
> return resultOpt.orElseGet(() -> deriveKeyNative(privateKey, publicKey));
> 
> if we want to avoid the JDK-8205476 change of nulling publicKey.
> 
> Thanks,
> --
> Andrew :)
> 
> Senior Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
> 
> PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
> Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
> https://keybase.io/gnu_andrew

[11u]: RFR: Backport of 8215694: keytool cannot generate RSASSA-PSS certificates

[Langer, Christoph]

Hi,

please help reviewing the backport of JDK- 8215694: keytool cannot generate 
RSASSA-PSS certificates. The patch doesn't apply cleanly but the rejects are 
only minor. The Item is needed as prerequisite to apply JDK-8216039.

Bug: https://bugs.openjdk.java.net/browse/JDK-8215694
Original Change: http://hg.openjdk.java.net/jdk/jdk12/rev/bdb29aa5fd31
Rejects when applying original change: 
http://cr.openjdk.java.net/~clanger/webrevs/8215694.rejects.patch
Full Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8215694.11u.full.0/
Incremental Webrev of added modifications: 
http://cr.openjdk.java.net/~clanger/webrevs/8215694.11u.manual.0/

Thanks
Christoph

RE: OpenJDK 11u backport of JDK-8148188: Enhance the security libraries to record events of interest

[Langer, Christoph]

Thanks, David.

Then I probably did the right thing by manually creating the backport bug.

cc-ing ops@o.j.n: Maybe you want to have a look why hgupdater failed in this 
case?

Cheers
Christoph

> -Original Message-
> From: David Holmes 
> Sent: Sonntag, 23. Juni 2019 00:15
> To: Langer, Christoph ; jdk-updates-
> d...@openjdk.java.net; security-dev 
> Subject: Re: OpenJDK 11u backport of JDK-8148188: Enhance the security
> libraries to record events of interest
> 
> Hi Christoph,
> 
> On 22/06/2019 2:25 pm, Langer, Christoph wrote:
> > Hi,
> >
> > I pushed the backport of JDK-8148188 [0] to jdk11u-dev [1] on Friday. I was
> wondering why no 11.0.5 backport JBS item was created. I then manually
> created JDK-8226636 [2], but the minute I had created it I got an idea what
> the reason for that strange JBS/hgupdater behavior was (all other pushes
> around this commit seem to have correct backport bugs). I now suspect that
> there existed a confidential 11-pool backport item already which was
> resolved by my push.
> >
> > Can anybody within Oracle please check this and if I’m correct, please also
> check whether the backport bug can be made public? I’d then close JDK-
> 8226636 as duplicate. But maybe I’m wrong with that guess… please
> enlighten me 😊
> 
> No there was no confidential issue resolved by your push. I would have
> to suspect hgupdater may be down, or else some other hgupdater
> configuration problem.
> 
> David
> --
> 
> > Thanks
> > Christoph
> >
> > [0] https://bugs.openjdk.java.net/browse/JDK-8148188
> > [1] http://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/5f98a6a92842
> > [2] https://bugs.openjdk.java.net/browse/JDK-8226636
> >

OpenJDK 11u backport of JDK-8148188: Enhance the security libraries to record events of interest

[Langer, Christoph]

Hi,

I pushed the backport of JDK-8148188 [0] to jdk11u-dev [1] on Friday. I was 
wondering why no 11.0.5 backport JBS item was created. I then manually created 
JDK-8226636 [2], but the minute I had created it I got an idea what the reason 
for that strange JBS/hgupdater behavior was (all other pushes around this 
commit seem to have correct backport bugs). I now suspect that there existed a 
confidential 11-pool backport item already which was resolved by my push.

Can anybody within Oracle please check this and if I’m correct, please also 
check whether the backport bug can be made public? I’d then close JDK-8226636 
as duplicate. But maybe I’m wrong with that guess… please enlighten me 😊

Thanks
Christoph

[0] https://bugs.openjdk.java.net/browse/JDK-8148188
[1] http://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/5f98a6a92842
[2] https://bugs.openjdk.java.net/browse/JDK-8226636

RE: [11u] RFR: 8208698: Improved ECC Implementation

[Langer, Christoph]

Hi Martin,

thanks for the review. Makes sense to take the fix regarding the overflow check 
along. I requested approval for JDK-8217344, too, and will push both patches 
together.

Best regards
Christoph

> -Original Message-
> From: Doerr, Martin
> Sent: Freitag, 31. Mai 2019 14:40
> To: Langer, Christoph ; jdk-updates-
> d...@openjdk.java.net
> Cc: security-dev 
> Subject: RE: [11u] RFR: 8208698: Improved ECC Implementation
> 
> Hi Christoph,
> 
> looks like quite some manual resolution just because of a small conflicting
> change in one file.
> Backport looks good, but please backport it together with JDK-8217344.
> After that, ECDHKeyAgreement.java should be identical to the jdk13 version.
> 
> Best regards,
> Martin
> 
> 
> > -Original Message-
> > From: jdk-updates-dev  On
> > Behalf Of Langer, Christoph
> > Sent: Dienstag, 28. Mai 2019 09:21
> > To: jdk-updates-...@openjdk.java.net
> > Cc: security-dev 
> > Subject: [11u] RFR: 8208698: Improved ECC Implementation
> >
> > Hi,
> >
> > please review this backport of JDK-8208698: Improved ECC
> Implementation.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8208698
> > Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/752e57845ad2
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8208698.11u/
> >
> > The patch did not apply cleanly because there were conflicts in
> > src/jdk.crypto.ec/share/classes/sun/security/ec/ECDHKeyAgreement.java
> > due to JDK-8205476 which is part of jdk/jdk but not of jdk11u-dev.
> > Unfortunately, JDK-8205476 can not be downported as prerequisite
> because
> > it brings a behavioral change and is associated with a CSR. So I resolved 
> > the
> > rejects manually. I add the rejects below.
> >
> > Thanks
> > Christoph
> >
> >
> > --- ECDHKeyAgreement.java
> > +++ ECDHKeyAgreement.java
> > @@ -99,42 +104,74 @@
> >  ("Key must be a PublicKey with algorithm EC");
> >  }
> > -ECPublicKey ecKey = (ECPublicKey)key;
> > -ECParameterSpec params = ecKey.getParams();
> > +this.publicKey = (ECPublicKey) key;
> > -if (ecKey instanceof ECPublicKeyImpl) {
> > -publicValue = ((ECPublicKeyImpl)ecKey).getEncodedPublicValue();
> > -} else { // instanceof ECPublicKey
> > -publicValue =
> > -ECUtil.encodePoint(ecKey.getW(), params.getCurve());
> > -}
> > +ECParameterSpec params = publicKey.getParams();
> >  int keyLenBits = params.getCurve().getField().getFieldSize();
> >  secretLen = (keyLenBits + 7) >> 3;
> >  return null;
> >  }
> > +private static void validateCoordinate(BigInteger c, BigInteger mod) {
> > +if (c.compareTo(BigInteger.ZERO) < 0) {
> > +throw new ProviderException("invalid coordinate");
> > +}
> > +
> > +if (c.compareTo(mod) >= 0) {
> > +throw new ProviderException("invalid coordinate");
> > +}
> > +}
> > +
> > +/*
> > + * Check whether a public key is valid. Throw ProviderException
> > + * if it is not valid or could not be validated.
> > + */
> > +private static void validate(ECOperations ops, ECPublicKey key) {
> > +
> > +// ensure that integers are in proper range
> > +BigInteger x = key.getW().getAffineX();
> > +BigInteger y = key.getW().getAffineY();
> > +
> > +BigInteger p = ops.getField().getSize();
> > +validateCoordinate(x, p);
> > +validateCoordinate(y, p);
> > +
> > +// ensure the point is on the curve
> > +EllipticCurve curve = key.getParams().getCurve();
> > +BigInteger rhs = x.modPow(BigInteger.valueOf(3),
> p).add(curve.getA()
> > +.multiply(x)).add(curve.getB()).mod(p);
> > +BigInteger lhs = y.modPow(BigInteger.valueOf(2), p).mod(p);
> > +if (!rhs.equals(lhs)) {
> > +throw new ProviderException("point is not on curve");
> > +}
> > +
> > +// check the order of the point
> > +ImmutableIntegerModuloP xElem = ops.getField().getElement(x);
> > +ImmutableIntegerModuloP yElem = ops.getField().getElement(y);
> > +AffinePoint affP = new AffinePoint(xElem, yElem);
> > +byte[] order = key.getParams().getOrder().toByteArray();
> > +ArrayUtil.reverse(order);
> > +  

RE: RFR CSR for 8162628: Migrating cacerts keystore to password-less PKCS12 format

[Langer, Christoph]

Hi Max,

> But you changed
> 
>everyone (yes, everyone) has been loading cacerts with a null password
> and they are able to read all certificate inside.
> 
> to
> 
>everyone (yes, everyone) who loads cacerts with a null password is able to
> read all certificates inside.
> 
> I *did* mean that *everyone* is using the null password. Did you not?
> 

Well, I wanted to phrase that everyone who's currently using a null password is 
still able to read all the certificates. If that's not what you're meaning, 
please change it back.

Best,
Christoph

RE: RFR CSR for 8162628: Migrating cacerts keystore to password-less PKCS12 format

[Langer, Christoph]

Hi Max,

I've already made some updates to the wording in the CSR.

In the specification section, it should probably also not mention the source 
location src/java.base/share/lib/security/cacerts as it is about to be 
eliminated by JDK-8193255. It should rather refer to 
/lib/security/cacerts, I think.

Best regards
Christoph

> -Original Message-
> From: security-dev  On Behalf Of
> Weijun Wang
> Sent: Freitag, 31. Mai 2019 05:33
> To: security-dev@openjdk.java.net
> Subject: RFR CSR for 8162628: Migrating cacerts keystore to password-less
> PKCS12 format
> 
> Please review the CSR at
> 
>https://bugs.openjdk.java.net/browse/JDK-8224891
> 
> (Oh, I hate the CSR having a different bug id.)
> 
> Basically, with this change, the cacerts file can be loaded with
> 
>KeyStore.getInstance("JKS" or "PKCS12").load(stream, null or anything) or
>KeyStore.getInstance(new File("cacerts"), null or anything)
> 
> so hopefully all your old code should still work.
> 
> I've also opened another RFE [1] that intends to find a different way to tag
> jdkCA entries in cacerts other than appending "[jdk]" to the alias.
> 
> Thanks,
> Max
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8225099

RE: RFR 8193255: Root Certificates should be stored in text format and assembled at build time

[Langer, Christoph]

Hi Max,

this looks all good to me now :)

Best regards
Christoph

> -Original Message-
> From: build-dev  On Behalf Of
> Weijun Wang
> Sent: Freitag, 31. Mai 2019 05:01
> To: Erik Joelsson 
> Cc: security-dev@openjdk.java.net; build-dev  d...@openjdk.java.net>
> Subject: Re: RFR 8193255: Root Certificates should be stored in text format
> and assembled at build time
> 
> New webrev at
> 
>https://cr.openjdk.java.net/~weijun/8193255/webrev.01
> 
> Changes:
> 
> 1. Textual info at the beginning of each cert
> 
> 2. Makefile: indentation, BUILD_TOOLS_JDK, MakeTargetDir, files instead of
> dir
> 
> Thanks,
> Max
> 
> > On May 31, 2019, at 1:34 AM, Erik Joelsson 
> wrote:
> >
> > On 2019-05-30 08:32, Weijun Wang wrote:
> >>
> >>> On May 30, 2019, at 10:01 PM, Erik Joelsson 
> wrote:
> >>>
> >>> In my experience, using directories for dependencies in make does not
> work well. Since all the files in make/data/cacerts are in a flat structure, I
> would recommend expressing the prerequisites as:
> >>>
> >>> $(wildcard $(GENDATA_CACERTS_SRC)/*)
> >>>
> >>> This will not cover the case where a file is removed, but that case is
> rarely handled well in make based build systems.
> >> But in my experiment, using the directory name does detect the file
> removal.
> >
> > It believe that worked well on your machine, but directory timestamp
> updates are file system dependent. I'm not sure we can count on all
> filesystems to accurately reflect time stamps based on file modification. It's
> also possible that an OS would touch directory timestamps for other reasons,
> which should not affect the build. I haven't tried having source directories 
> as
> prerequisites before, so I simply don't know how reliable it is. My experience
> is rather with directories as targets, which certainly doesn't work. If you
> verified that it worked as expected on all supported OSes, I would be less
> worried.
> >
> >> Or, can I list *both* the files and the directory to get maximum
> awareness?
> >
> > The directory modification time will usually not change when a file in it is
> modified, only when adding or removing files (and possibly some other
> operations), so adding the files is certainly a must. If you go with both, 
> then I
> would just be worried about potential false positives.
> >
> > /Erik
> >

RE: RFR 8193255: Root Certificates should be stored in text format and assembled at build time

[Langer, Christoph]

Hi Max,

first of all, thanks for doing this enhancement. That'll really help in the 
future when downstream vendors merge in additional certificates (or remove 
some) like we do with SapMachine. Currently we have to resolve manually 
everytime cacerts is modified.

As for the dependencies: if you want to handle removals of certificates you 
might want to consider having a target that counts files in make/data/cacerts 
and stores the count in an intermediate file. You can do the counting every 
time (e.g. phony target) but only update the count file if the number has 
changed. And the actual cacerts target would need to depend on the count file 
as well.

However, I guess it would also be ok to just depend on all the files in 
make/data/cacerts.

Looking forward to see your update with Erik's suggestions worked in. Some 
copyright years need to be updated as well.

Thanks & Best regards
Christoph

> -Original Message-
> From: build-dev  On Behalf Of
> Weijun Wang
> Sent: Donnerstag, 30. Mai 2019 17:32
> To: Erik Joelsson 
> Cc: security-dev@openjdk.java.net; build-dev  d...@openjdk.java.net>
> Subject: Re: RFR 8193255: Root Certificates should be stored in text format
> and assembled at build time
> 
> 
> 
> > On May 30, 2019, at 10:01 PM, Erik Joelsson 
> wrote:
> >
> > In my experience, using directories for dependencies in make does not
> work well. Since all the files in make/data/cacerts are in a flat structure, I
> would recommend expressing the prerequisites as:
> >
> > $(wildcard $(GENDATA_CACERTS_SRC)/*)
> >
> > This will not cover the case where a file is removed, but that case is 
> > rarely
> handled well in make based build systems.
> 
> But in my experiment, using the directory name does detect the file removal.
> 
> Or, can I list *both* the files and the directory to get maximum awareness?
> 
> --Max

RE: RFR(S): Cleanups in sun/security/provider/certpath/ldap/LDAPCertStoreImpl.java

[Langer, Christoph]

Hi Sean,

thanks for the review. I've addressed your points: 
http://cr.openjdk.java.net/~clanger/webrevs/8224729.2/

Thanks
Christoph

> -Original Message-
> From: Sean Mullan 
> Sent: Dienstag, 28. Mai 2019 15:18
> To: Langer, Christoph ; security-dev  d...@openjdk.java.net>
> Subject: Re: RFR(S): Cleanups in
> sun/security/provider/certpath/ldap/LDAPCertStoreImpl.java
>
> Hi Christoph,
>
> This is not really a bug, so I would change it to an Enhancement.
>
> 73 // private static final String DELTA_CRL =
> "deltaRevocationList;binary";
>
> I would just remove this line.
>
> Looks good otherwise.
>
> --Sean
>
> On 5/25/19 5:51 AM, Langer, Christoph wrote:
> > Hi,
> >
> > please review this small cleanup which started off under a different
> > subject [0] but turned out to be the wrong thing to do. Still the
> > cleanup parts seem to be valuable, so requesting a review for that here.
> >
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8224729.1/
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8224729
> >
> > Thanks
> >
> > Christoph
> >
> > [0]
> > https://mail.openjdk.java.net/pipermail/security-dev/2019-
> May/019967.html
> >

[11u] RFR: 8208698: Improved ECC Implementation

[Langer, Christoph]

Hi,

please review this backport of JDK-8208698: Improved ECC Implementation.

Bug: https://bugs.openjdk.java.net/browse/JDK-8208698
Original Change: http://hg.openjdk.java.net/jdk/jdk/rev/752e57845ad2
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8208698.11u/

The patch did not apply cleanly because there were conflicts in 
src/jdk.crypto.ec/share/classes/sun/security/ec/ECDHKeyAgreement.java due to 
JDK-8205476 which is part of jdk/jdk but not of jdk11u-dev. Unfortunately, 
JDK-8205476 can not be downported as prerequisite because it brings a 
behavioral change and is associated with a CSR. So I resolved the rejects 
manually. I add the rejects below.

Thanks
Christoph


--- ECDHKeyAgreement.java
+++ ECDHKeyAgreement.java
@@ -99,42 +104,74 @@
 ("Key must be a PublicKey with algorithm EC");
 }
-ECPublicKey ecKey = (ECPublicKey)key;
-ECParameterSpec params = ecKey.getParams();
+this.publicKey = (ECPublicKey) key;
-if (ecKey instanceof ECPublicKeyImpl) {
-publicValue = ((ECPublicKeyImpl)ecKey).getEncodedPublicValue();
-} else { // instanceof ECPublicKey
-publicValue =
-ECUtil.encodePoint(ecKey.getW(), params.getCurve());
-}
+ECParameterSpec params = publicKey.getParams();
 int keyLenBits = params.getCurve().getField().getFieldSize();
 secretLen = (keyLenBits + 7) >> 3;
 return null;
 }
+private static void validateCoordinate(BigInteger c, BigInteger mod) {
+if (c.compareTo(BigInteger.ZERO) < 0) {
+throw new ProviderException("invalid coordinate");
+}
+
+if (c.compareTo(mod) >= 0) {
+throw new ProviderException("invalid coordinate");
+}
+}
+
+/*
+ * Check whether a public key is valid. Throw ProviderException
+ * if it is not valid or could not be validated.
+ */
+private static void validate(ECOperations ops, ECPublicKey key) {
+
+// ensure that integers are in proper range
+BigInteger x = key.getW().getAffineX();
+BigInteger y = key.getW().getAffineY();
+
+BigInteger p = ops.getField().getSize();
+validateCoordinate(x, p);
+validateCoordinate(y, p);
+
+// ensure the point is on the curve
+EllipticCurve curve = key.getParams().getCurve();
+BigInteger rhs = x.modPow(BigInteger.valueOf(3), p).add(curve.getA()
+.multiply(x)).add(curve.getB()).mod(p);
+BigInteger lhs = y.modPow(BigInteger.valueOf(2), p).mod(p);
+if (!rhs.equals(lhs)) {
+throw new ProviderException("point is not on curve");
+}
+
+// check the order of the point
+ImmutableIntegerModuloP xElem = ops.getField().getElement(x);
+ImmutableIntegerModuloP yElem = ops.getField().getElement(y);
+AffinePoint affP = new AffinePoint(xElem, yElem);
+byte[] order = key.getParams().getOrder().toByteArray();
+ArrayUtil.reverse(order);
+Point product = ops.multiply(affP, order);
+if (!ops.isNeutral(product)) {
+throw new ProviderException("point has incorrect order");
+}
+
+}
+
 // see JCE spec
 @Override
 protected byte[] engineGenerateSecret() throws IllegalStateException {
-if ((privateKey == null) || (publicValue == null)) {
+if ((privateKey == null) || (publicKey == null)) {
 throw new IllegalStateException("Not initialized correctly");
 }
-byte[] s = privateKey.getS().toByteArray();
-byte[] encodedParams =   // DER OID
-ECUtil.encodeECParameterSpec(null, privateKey.getParams());
-
-try {
-
-byte[] result = deriveKey(s, publicValue, encodedParams);
-publicValue = null;
-return result;
-
-} catch (GeneralSecurityException e) {
-throw new ProviderException("Could not derive key", e);
-}
-
+Optional resultOpt = deriveKeyImpl(privateKey, publicKey);
+byte[] result = resultOpt.orElseGet(
+() -> deriveKeyNative(privateKey, publicKey)
+);
+publicKey = null;
+return result;
 }
 // see JCE spec

RFR(S): Cleanups in sun/security/provider/certpath/ldap/LDAPCertStoreImpl.java

[Langer, Christoph]

Hi,

please review this small cleanup which started off under a different subject 
[0] but turned out to be the wrong thing to do. Still the cleanup parts seem to 
be valuable, so requesting a review for that here.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8224729.1/
Bug: https://bugs.openjdk.java.net/browse/JDK-8224729

Thanks
Christoph

[0] https://mail.openjdk.java.net/pipermail/security-dev/2019-May/019967.html

RE: RFR(S): 8224729: sun/security/provider/certpath/ldap/LDAPCertStoreImpl.java can't handle forward slash characters in Certificate Issuer Names

[Langer, Christoph]

> > As for Bug JDK-8224729: Shall I just close the ticket, dropping a comment
> about the ignorance of the author or shall I repurpose it to do the other
> cleanups in LDAPCertStoreImpl.java that I suggested? Don't know if these
> are appreciated... what do you think?
> 
> Either way, the debugging does seem useful so either open a new bug or
> change the description of the existing bug. Please post an updated
> webrev w/o the ldap escaping change too.

Ok, will do and start a new review thread.

Thanks
Christoph

RE: RFR(S): 8224727: Problem list 2 tests in security/infra/java/security/cert/CertPathValidator/certification

[Langer, Christoph]

Hi Rajan,

thanks for this. Problem list update would be then:
http://cr.openjdk.java.net/~clanger/webrevs/8224727.1/

Please review.

Thanks
Christoph


> -Original Message-
> From: Rajan Halade 
> Sent: Freitag, 24. Mai 2019 18:52
> To: Langer, Christoph 
> Cc: Sean Mullan ; security-dev  d...@openjdk.java.net>
> Subject: Re: RFR(S): 8224727: Problem list 2 tests in
> security/infra/java/security/cert/CertPathValidator/certification
> 
> I have pushed fix for ComodoCA with JDK-8202651 and have filed
> JDK-8224768 to address ActalisCA issue. You can problem list ActalisCA
> for now with JDK-8224727.
> 
> Thanks,
> Rajan
> 
> On 5/24/19 6:43 AM, Sean Mullan wrote:
> > On 5/24/19 4:00 AM, Langer, Christoph wrote:
> >> Hi,
> >>
> >> please review the problem listing of
> >>
> >>
> security/infra/java/security/cert/CertPathValidator/certification/ActalisCA.ja
> va
> >> and
> >>
> >>
> security/infra/java/security/cert/CertPathValidator/certification/ComodoCA.
> java
> >>
> >>
> >> until JDK-8202651 and JDK-8215546 are resolved.
> >>
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8224727
> >>
> >> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8224727.0/
> >
> > I would prefer to just problem list ActalisCA.java. Rajan has a fix
> > for ComodoCA which he posted the other day as part of
> > https://bugs.openjdk.java.net/browse/JDK-8202651, so if he just splits
> > that off into a separate bug then he can push that fix. Can you
> > coordinate with Rajan the best way to handle this?
> >
> > There seems to be overlap in these various bugs for the failures in
> > Actalis, Buypass and Comodo - I think we should clean this up so that
> > there is one unique bug per failing test.
> >
> > --Sean

RE: RFR(S): 8224729: sun/security/provider/certpath/ldap/LDAPCertStoreImpl.java can't handle forward slash characters in Certificate Issuer Names

[Langer, Christoph]

Hi Sean,

ok, I see, you're fully correct. So I hereby withdraw my fix proposal.

As for the exclusion of the test: I have requested it this morning anyway: 
https://mail.openjdk.java.net/pipermail/security-dev/2019-May/019966.html. So I 
would assume that you ask Actalis to reissue the certificates anyway and then 
eventually resolve 8202651 with a good certificate?

As for Bug JDK-8224729: Shall I just close the ticket, dropping a comment about 
the ignorance of the author or shall I repurpose it to do the other cleanups in 
LDAPCertStoreImpl.java that I suggested? Don't know if these are appreciated... 
what do you think?

Best regards
Christoph


> -Original Message-
> From: Sean Mullan 
> Sent: Freitag, 24. Mai 2019 14:02
> To: Langer, Christoph ; security-dev  d...@openjdk.java.net>
> Subject: Re: RFR(S): 8224729:
> sun/security/provider/certpath/ldap/LDAPCertStoreImpl.java can't handle
> forward slash characters in Certificate Issuer Names
> 
> Hi Christoph,
> 
> I don't think this is the right fix. The LDAP URL in the Certificate is
> incorrect and the forward slash should be escaped. If we start to make
> workarounds in the code to accept certificates that are not properly
> encoded, it becomes a slipperly slope. I base my rationale on the
> following RFCs:
> 
> 1. https://tools.ietf.org/html/rfc5280#section-4.2.1.13
> 
> When the LDAP URI scheme [RFC4516] is
> used, the URI MUST include a  field containing the distinguished
> name of the entry holding the CRL, MUST include a single 
> that contains an appropriate attribute description for the attribute
> that holds the CRL [RFC4523], and SHOULD include a 
> (e.g., <ldap://ldap.example.com/cn=example%20CA,dc=example,dc=com?
> certificateRevocationList;binary>).
> 
> 2. https://tools.ietf.org/html/rfc4516#section-2
> 
> The  is an LDAP Distinguished Name using the string format
> described in [RFC4514].  It identifies the base object of the LDAP
> search or the target of a non-search operation.
> 
> 3. https://tools.ietf.org/html/rfc4514#section-2.4
> 
> If that UTF-8-encoded Unicode
> string does not have any of the following characters that need
> escaping, then that string can be used as the string representation
> of the value.
> 
> ...
> 
>- one of the characters '"', '+', ',', ';', '<', '>',  or '\'
>  (U+0022, U+002B, U+002C, U+003B, U+003C, U+003E, or U+005C,
>  respectively);
> 
> So, I think the proper way to handle this is to contact the CA and
> inform that the certificate does not comply with RFC 5280 and should be
> re-issued. Rajan or I can take care of that and get back to you. For
> now, if this is blocking your testing, I suggest putting the test on the
> ProblemList.
> 
> Thanks,
> Sean
> 
> On 5/24/19 5:11 AM, Langer, Christoph wrote:
> > Hi,
> >
> > please review this fix for an issue that I've discovered when working
> > with test
> >
> security/infra/java/security/cert/CertPathValidator/certification/ActalisCA.ja
> va.
> >
> > It fails when the test tries to do the CRL verification of the
> > certificate. It has issues in the LDAP implementation because of the
> > certificate's name "cn=Actalis Authentication Root CA,o=Actalis
> > S.p.A./03358520967,c=IT". The name contains a forward slash which is at
> > the same time a compound separator in javax.naming/LDAP. So it needs
> > some escaping.
> >
> > I also cleaned up some debugging code and removed/commented out
> unused
> > fields and methods.
> >
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8224729.0/
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8224729
> >
> > Thanks
> >
> > Christoph
> >

RFR(S): 8224729: sun/security/provider/certpath/ldap/LDAPCertStoreImpl.java can't handle forward slash characters in Certificate Issuer Names

[Langer, Christoph]

Hi,

please review this fix for an issue that I've discovered when working with test 
security/infra/java/security/cert/CertPathValidator/certification/ActalisCA.java.

It fails when the test tries to do the CRL verification of the certificate. It 
has issues in the LDAP implementation because of the certificate's name 
"cn=Actalis Authentication Root CA,o=Actalis S.p.A./03358520967,c=IT". The name 
contains a forward slash which is at the same time a compound separator in 
javax.naming/LDAP. So it needs some escaping.

I also cleaned up some debugging code and removed/commented out unused fields 
and methods.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8224729.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8224729

Thanks
Christoph

RFR(S): 8224727: Problem list 2 tests in security/infra/java/security/cert/CertPathValidator/certification

[Langer, Christoph]

Hi,

please review the problem listing of

security/infra/java/security/cert/CertPathValidator/certification/ActalisCA.java
 and
security/infra/java/security/cert/CertPathValidator/certification/ComodoCA.java

until JDK-8202651 and JDK-8215546 are resolved.

Bug: https://bugs.openjdk.java.net/browse/JDK-8224727
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8224727.0/

Thanks
Christoph

RE: RFR: 8202651: Test ActalisCA.java and ComodoCA fails

[Langer, Christoph]

Hi Rajan, Sean,

first of all, thanks for the update on the tests.

As I see, there are still open questions for the ActalisCA test. So I'm 
wondering whether the fix for ComodoCA can be pushed as fix for 
https://bugs.openjdk.java.net/browse/JDK-8215546 ?

Furthermore, can we also exclude the ActalisCA test via the ProblemList for the 
time being? I believe this has not been done because these tests aren't part of 
any regular test tiers. However, we at SAP run all possible tests in the test 
folder in some suite of the test environment and it would be good if the test 
could be excluded then. I would file a bug and do the change if you agree.

Other than that, I'm observing another issue in the CRL test for ActalisCA. It 
fails with this message: "Received exception: 
java.security.cert.CertPathValidatorException: 
sun.security.provider.certpath.PKIX$CertStoreTypeException: Invalid name: 
cn=Actalis Authentication Root CA,o=Actalis S.p.A./03358520967,c=IT". The 
problem is that the certificate's issue name is literally taken as 
javax.naming. CompositeName in LDAPCertStoreImpl. The forward slash symbol '/' 
is, however, used as component separator in LDAP composite names. So it needs 
escaping. I'll file a bug and propose a fix for this in a separate thread. I'm 
actually wondering why this hasn't been reported yet but I don't see similar 
bugs...

Thank you
Christoph

> -Original Message-
> From: security-dev  On Behalf Of
> Rajan Halade
> Sent: Mittwoch, 22. Mai 2019 19:35
> To: Sean Mullan 
> Cc: security-dev 
> Subject: Re: RFR: 8202651: Test ActalisCA.java and ComodoCA fails
> 
> On 5/22/19 9:34 AM, Sean Mullan wrote:
> > On 5/22/19 12:04 PM, Rajan Halade wrote:
> >> On 5/22/19 8:39 AM, Sean Mullan wrote:
> >>> On 5/21/19 5:31 PM, Rajan Halade wrote:
>  Please review this fix to update test certificates used in Actalis
>  and Comodo CA interop tests. The bug also mentioned QuoVadisCA
> test
>  but I am not able to reproduce the failure. For Actalis CA, I
>  couldn't get revoked test certificate but the test is updated for
>  valid certificate and will pass now by skipping expired revoked chain.
> >>>
> >>> It looks like the test is still expecting a revoked status - is that
> >>> still working because the IntCA is revoked?:
> >> It is working because revoked certificate is expired, test is skipped
> >> then.
> >
> > Have you asked Actalis for a new revoked test certificate? If you
> > can't get one, I would remove the revoked certificates and the test
> > for it then, since you are not testing that behavior anymore and that
> > is not apparent from the test right now.
> I will follow up with CA then and leave this bug open for now.
> >
> > Also do you know why the revocation check for the intermediate CA is
> > not working?
> Revocation check on intermediate CA is working fine. INT_REVOKED is a
> good certificate, may name is misleading. INT_REVOKED here means that
> this is a intermediate CA for revoked EE certificate.
> 
> Thanks,
> Rajan
> >
> >>>
> >>>  232 // Validate Revoked
> >>>  233 pathValidator.validate(new String[]{REVOKED, INT_REVOKED},
> >>>  234 ValidatePathWithParams.Status.REVOKED,
> >>>  235 "Fri Jan 29 01:06:42 PST 2016", System.out);
> >>>  236
> >>>
> >>> It should be ok if the revoked certificate is expired though as you
> >>> can set the validation date to the past (within the interval where
> >>> the certificate is still valid).
> >>> Or is it because the Actalis OCSP responder is no longer reporting
> >>> that the certificate is revoked?
> >> Earlier test had past validation with OCSP but for some time now OCSP
> >> is returning UNKNOWN status instead of REVOKED. This could be an
> >> issue depending on how implementation treats UNKNOWN status. We
> will
> >> have to follow up with CA to check on policy - Is this only happening
> >> because we are using test certificate or is it a policy?
> >
> > It depends, if it is a TLS certificate then it is usually acceptable
> > to report the revoked certificate as UNKNOWN after it expires since
> > you should not be trusting expired TLS certificates. For a code
> > signing certificate, it is better to retain the REVOKED status for a
> > longer time period after it expires since it may still be in use (for
> > example, in a timestamped application).
> >
> > --Sean
> >
> >>
> >> Thanks,
> >> Rajan
> >>>
> >>> --Sean
> >>>
> 
>  Webrev: http://cr.openjdk.java.net/~rhalade/8202651/webrev.00/
>  Bug: https://bugs.openjdk.java.net/browse/JDK-8202651
> 
>  Thanks,
>  Rajan
> >>

RE: [8u] RFR: 8189131: Open-source the Oracle JDK Root Certificates (Integration for JEP 319: Root Certificates)

[Langer, Christoph]

Hi Martin,

thanks for your review.

Regarding the failing tests: They are not executed in any tier. You'd either 
have to execute all tests below jdk/test or the test group 
"jdk_security_infra". But anyway, I'll follow up on these tests as we see the 
issues in our test system and have to exclude them locally.

/Christoph

> -Original Message-
> From: Doerr, Martin
> Sent: Dienstag, 14. Mai 2019 10:22
> To: Langer, Christoph ; 'jdk8u-
> d...@openjdk.java.net' ; security-dev
> 
> Subject: RE: [8u] RFR: 8189131: Open-source the Oracle JDK Root Certificates
> (Integration for JEP 319: Root Certificates)
> 
> Hi Christoph,
> 
> this looks good to me.
> I don't know if anybody has issues with the failing tests. Should they get
> added to a problem list?
> 
> Best regards,
> Martin
> 
> 
> -Original Message-
> From: jdk8u-dev  On Behalf Of
> Langer, Christoph
> Sent: Dienstag, 7. Mai 2019 16:15
> To: 'jdk8u-...@openjdk.java.net' ; security-
> dev 
> Subject: [CAUTION] RE: [8u] RFR: 8189131: Open-source the Oracle JDK Root
> Certificates (Integration for JEP 319: Root Certificates)
> 
> Ping: can I please have a review for this?
> 
> From: Langer, Christoph
> Sent: Donnerstag, 2. Mai 2019 14:55
> To: 'jdk8u-...@openjdk.java.net' ; security-
> dev 
> Subject: [8u] RFR: 8189131: Open-source the Oracle JDK Root Certificates
> (Integration for JEP 319: Root Certificates)
> 
> Hi,
> 
> as was already discussed and requested on the mailing lists ([0], [1]), I 
> hereby
> propose a change to add the root certificates of upstream OpenJDK to
> OpenJDK 8 updates.
> 
> The main bug that (initially) brought the Oracle certificates to OpenJDK is
> 8189131: Open-source the Oracle JDK Root Certificates [2]. My proposed
> change will also backport all updates to the contents of cacerts since then:
> 
> 8191844: Remove SECOM root (secomevrootca1)
> 8189949: Remove Baltimore Cybertrust Code Signing CA
> 8191031: Remove several Symantec Root CAs
> 8196141: Add GoDaddy root certificates
> 8204923: Restore Symantec root verisignclass2g2ca
> 8195774: Add Entrust root certificates
> 8199779: Add T-Systems, GlobalSign and Starfield services root certificates
> 8209506: Add Google Trust Services GlobalSign root certificates
> 8210432: Add additional TeliaSonera root certificate
> 8195793: Remove GTE CyberTrust Global Root
> 8216577: Add GlobalSign's R6 Root certificate
> 8222137: Remove T-Systems root CA certificate
> 
> Please find the webrev here:
> http://cr.openjdk.java.net/~clanger/webrevs/8189131.8u/
> 
> I took the current state of cacerts from the jdk/jdk repo along with the
> provided testcases and brought them down to the jdk8 repository layout.
> 
> To make the test run in JDK8, I had to
> a) modify test/sun/security/lib/cacerts/VerifyCACerts.java:
>   240 private static final HashSet EXPIRY_EXC_ENTRIES = new
> HashSet() {
> I needed to add the String type to the constructor of the HashSet, since the
> JDK8 java compiler will not accept <> in that place.
> 
> b) modify
> test/security/infra/java/security/cert/CertPathValidator/certification/Validat
> ePathWithParams.java
> 
>   60 private static final String CACERTS_STORE =
> System.getProperty("test.jdk")
> 
>   61 + FS + "jre" + FS + "lib" + FS + "security" + FS + "cacerts";
> I needed to adapt the path to cacerts in a JDK8 JDK/JRE as it is located in
> subdirectory jre there.
> 
> Out of the tests in
> test/security/infra/java/security/cert/CertPathValidator/certification, there
> are 2 failing:
> FAILED:
> security/infra/java/security/cert/CertPathValidator/certification/ActalisCA.ja
> va
> FAILED:
> security/infra/java/security/cert/CertPathValidator/certification/ComodoCA.
> java
> However, for jdk/jdk it is the same. JBS Issues for these tests exist and are
> yet unresolved: JDK-8202651 and JDK-8215546.
> 
> Since the tests don't seem to be part of any tier, I propose to include them 
> in
> this backport and later on also backport possible fixes to them.
> 
> Thanks
> Christoph
> 
> [0] https://mail.openjdk.java.net/pipermail/security-dev/2019-
> March/019557.html
> [1] https://mail.openjdk.java.net/pipermail/security-dev/2019-
> April/019733.html
> [2] https://bugs.openjdk.java.net/browse/JDK-8189131

RE: RFR(XS): 8223555: Cleanups in cacerts tests

[Langer, Christoph]

Hi Xuelei,

thanks for looking.

The warnings would appear in my Eclipse IDE. During the tests (build) nothing 
had been observed and this will hopefully still be the same. 😊

So you are good with the changes?

Best regards
Christoph

> -Original Message-
> From: Xuelei Fan 
> Sent: Mittwoch, 8. Mai 2019 15:58
> To: Langer, Christoph ; security-dev  d...@openjdk.java.net>
> Subject: Re: RFR(XS): 8223555: Cleanups in cacerts tests
> 
> Hi Christoph,
> 
> Did you see compiler warning for the files?  I tried with the JDK
> repository build, no compiler warning message.  The update is safe.  I
> may miss something about why we want to make the update.
> 
> Thanks,
> Xuelei
> 
> On 5/8/2019 1:21 AM, Langer, Christoph wrote:
> > Hi,
> >
> > please review this small change which contains a few minor cleanups to
> > the tests for the CA certificates. I had it in my mercurial queue for
> > quite some time and always wanted to contribute it 😊
> >
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8223555.0/
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8223555
> >
> > Thanks
> >
> > Christoph
> >

RFR(XS): 8223555: Cleanups in cacerts tests

[Langer, Christoph]

Hi,

please review this small change which contains a few minor cleanups to the 
tests for the CA certificates. I had it in my mercurial queue for quite some 
time and always wanted to contribute it 😊

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8223555.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8223555

Thanks
Christoph

RE: [8u] RFR: 8189131: Open-source the Oracle JDK Root Certificates (Integration for JEP 319: Root Certificates)

[Langer, Christoph]

Ping: can I please have a review for this?

From: Langer, Christoph
Sent: Donnerstag, 2. Mai 2019 14:55
To: 'jdk8u-...@openjdk.java.net' ; security-dev 

Subject: [8u] RFR: 8189131: Open-source the Oracle JDK Root Certificates 
(Integration for JEP 319: Root Certificates)

Hi,

as was already discussed and requested on the mailing lists ([0], [1]), I 
hereby propose a change to add the root certificates of upstream OpenJDK to 
OpenJDK 8 updates.

The main bug that (initially) brought the Oracle certificates to OpenJDK is 
8189131: Open-source the Oracle JDK Root Certificates [2]. My proposed change 
will also backport all updates to the contents of cacerts since then:

8191844: Remove SECOM root (secomevrootca1)
8189949: Remove Baltimore Cybertrust Code Signing CA
8191031: Remove several Symantec Root CAs
8196141: Add GoDaddy root certificates
8204923: Restore Symantec root verisignclass2g2ca
8195774: Add Entrust root certificates
8199779: Add T-Systems, GlobalSign and Starfield services root certificates
8209506: Add Google Trust Services GlobalSign root certificates
8210432: Add additional TeliaSonera root certificate
8195793: Remove GTE CyberTrust Global Root
8216577: Add GlobalSign's R6 Root certificate
8222137: Remove T-Systems root CA certificate

Please find the webrev here: 
http://cr.openjdk.java.net/~clanger/webrevs/8189131.8u/

I took the current state of cacerts from the jdk/jdk repo along with the 
provided testcases and brought them down to the jdk8 repository layout.

To make the test run in JDK8, I had to
a) modify test/sun/security/lib/cacerts/VerifyCACerts.java:
  240 private static final HashSet EXPIRY_EXC_ENTRIES = new 
HashSet() {
I needed to add the String type to the constructor of the HashSet, since the 
JDK8 java compiler will not accept <> in that place.

b) modify 
test/security/infra/java/security/cert/CertPathValidator/certification/ValidatePathWithParams.java

  60 private static final String CACERTS_STORE = 
System.getProperty("test.jdk")

  61 + FS + "jre" + FS + "lib" + FS + "security" + FS + "cacerts";
I needed to adapt the path to cacerts in a JDK8 JDK/JRE as it is located in 
subdirectory jre there.

Out of the tests in 
test/security/infra/java/security/cert/CertPathValidator/certification, there 
are 2 failing:
FAILED: 
security/infra/java/security/cert/CertPathValidator/certification/ActalisCA.java
FAILED: 
security/infra/java/security/cert/CertPathValidator/certification/ComodoCA.java
However, for jdk/jdk it is the same. JBS Issues for these tests exist and are 
yet unresolved: JDK-8202651 and JDK-8215546.

Since the tests don't seem to be part of any tier, I propose to include them in 
this backport and later on also backport possible fixes to them.

Thanks
Christoph

[0] https://mail.openjdk.java.net/pipermail/security-dev/2019-March/019557.html
[1] https://mail.openjdk.java.net/pipermail/security-dev/2019-April/019733.html
[2] https://bugs.openjdk.java.net/browse/JDK-8189131

[8u] RFR: 8189131: Open-source the Oracle JDK Root Certificates (Integration for JEP 319: Root Certificates)

[Langer, Christoph]

Hi,

as was already discussed and requested on the mailing lists ([0], [1]), I 
hereby propose a change to add the root certificates of upstream OpenJDK to 
OpenJDK 8 updates.

The main bug that (initially) brought the Oracle certificates to OpenJDK is 
8189131: Open-source the Oracle JDK Root Certificates [2]. My proposed change 
will also backport all updates to the contents of cacerts since then:

8191844: Remove SECOM root (secomevrootca1)
8189949: Remove Baltimore Cybertrust Code Signing CA
8191031: Remove several Symantec Root CAs
8196141: Add GoDaddy root certificates
8204923: Restore Symantec root verisignclass2g2ca
8195774: Add Entrust root certificates
8199779: Add T-Systems, GlobalSign and Starfield services root certificates
8209506: Add Google Trust Services GlobalSign root certificates
8210432: Add additional TeliaSonera root certificate
8195793: Remove GTE CyberTrust Global Root
8216577: Add GlobalSign's R6 Root certificate
8222137: Remove T-Systems root CA certificate

Please find the webrev here: 
http://cr.openjdk.java.net/~clanger/webrevs/8189131.8u/

I took the current state of cacerts from the jdk/jdk repo along with the 
provided testcases and brought them down to the jdk8 repository layout.

To make the test run in JDK8, I had to
a) modify test/sun/security/lib/cacerts/VerifyCACerts.java:
  240 private static final HashSet EXPIRY_EXC_ENTRIES = new 
HashSet() {
I needed to add the String type to the constructor of the HashSet, since the 
JDK8 java compiler will not accept <> in that place.

b) modify 
test/security/infra/java/security/cert/CertPathValidator/certification/ValidatePathWithParams.java

  60 private static final String CACERTS_STORE = 
System.getProperty("test.jdk")

  61 + FS + "jre" + FS + "lib" + FS + "security" + FS + "cacerts";
I needed to adapt the path to cacerts in a JDK8 JDK/JRE as it is located in 
subdirectory jre there.

Out of the tests in 
test/security/infra/java/security/cert/CertPathValidator/certification, there 
are 2 failing:
FAILED: 
security/infra/java/security/cert/CertPathValidator/certification/ActalisCA.java
FAILED: 
security/infra/java/security/cert/CertPathValidator/certification/ComodoCA.java
However, for jdk/jdk it is the same. JBS Issues for these tests exist and are 
yet unresolved: JDK-8202651 and JDK-8215546.

Since the tests don't seem to be part of any tier, I propose to include them in 
this backport and later on also backport possible fixes to them.

Thanks
Christoph

[0] https://mail.openjdk.java.net/pipermail/security-dev/2019-March/019557.html
[1] https://mail.openjdk.java.net/pipermail/security-dev/2019-April/019733.html
[2] https://bugs.openjdk.java.net/browse/JDK-8189131

RE: RFR: 8222137: Remove T-Systems root CA certificate

[Langer, Christoph]

Hi Sean,

sounds good. I’ll create an 11.0.4 bug before pushing to jkd11u-dev.

I assume you’ll take care of jdk12u?

Thanks
Christoph

From: Seán Coffey 
Sent: Donnerstag, 2. Mai 2019 10:25
To: Langer, Christoph 
Cc: Rajan Halade ; Sean Mullan 
; security-dev 
Subject: Re: RFR: 8222137: Remove T-Systems root CA certificate


Christoph,

if possible, can you create an "11.0.4" fixVersion backport before pushing your 
changeset? That will help with JBS records. Alternatively, I'll just ensure 
there's an 11-pool or 11.0.5 record before I push.

regards,
Sean.
On 02/05/2019 08:48, Langer, Christoph wrote:
Thanks, Raj.

Hi Seán,

since there is an 11-pool request, I guess you should push the change to some 
11.o.-oracle repository first such that you’ll get this item resolved by 
hgupdater. Please let me know how to proceed.

Thanks
Christoph

From: Rajan Halade <mailto:rajan.hal...@oracle.com>
Sent: Mittwoch, 1. Mai 2019 21:08
To: Langer, Christoph 
<mailto:christoph.lan...@sap.com>
Cc: Sean Mullan <mailto:sean.mul...@oracle.com>; 
security-dev 
<mailto:security-dev@openjdk.java.net>; Seán 
Coffey <mailto:sean.cof...@oracle.com>
Subject: Re: RFR: 8222137: Remove T-Systems root CA certificate

I have created needed backports including 12-pool, 11-pool, 8-pool, and 7-pool. 
These are assigned to Sean C. for now. Please co-ordinate with him.

Thanks,
Rajan
On 5/1/19 6:19 AM, Langer, Christoph wrote:
Hi Rajan,

I’ll take this to jdk11 updates. What about jdk12 updates? I can process both 
releases, if you want.

Best regards
Christoph

From: security-dev 
<mailto:security-dev-boun...@openjdk.java.net>
 On Behalf Of Rajan Halade
Sent: Dienstag, 30. April 2019 20:39
To: Sean Mullan <mailto:sean.mul...@oracle.com>; 
security-dev 
<mailto:security-dev@openjdk.java.net>
Subject: RFR: 8222137: Remove T-Systems root CA certificate

Please review this fix for removal of T-Systems Deutsche Telekom Root CA 2 
certificate.

Webrev: http://cr.openjdk.java.net/~rhalade/8222137/webrev.00/

Release note is at - https://bugs.openjdk.java.net/browse/JDK-8223161

Thanks,
Rajan

RE: RFR: 8222137: Remove T-Systems root CA certificate

[Langer, Christoph]

Thanks, Raj.

Hi Seán,

since there is an 11-pool request, I guess you should push the change to some 
11.o.-oracle repository first such that you’ll get this item resolved by 
hgupdater. Please let me know how to proceed.

Thanks
Christoph

From: Rajan Halade 
Sent: Mittwoch, 1. Mai 2019 21:08
To: Langer, Christoph 
Cc: Sean Mullan ; security-dev 
; Seán Coffey 
Subject: Re: RFR: 8222137: Remove T-Systems root CA certificate

I have created needed backports including 12-pool, 11-pool, 8-pool, and 7-pool. 
These are assigned to Sean C. for now. Please co-ordinate with him.

Thanks,
Rajan
On 5/1/19 6:19 AM, Langer, Christoph wrote:
Hi Rajan,

I’ll take this to jdk11 updates. What about jdk12 updates? I can process both 
releases, if you want.

Best regards
Christoph

From: security-dev 
<mailto:security-dev-boun...@openjdk.java.net>
 On Behalf Of Rajan Halade
Sent: Dienstag, 30. April 2019 20:39
To: Sean Mullan <mailto:sean.mul...@oracle.com>; 
security-dev 
<mailto:security-dev@openjdk.java.net>
Subject: RFR: 8222137: Remove T-Systems root CA certificate

Please review this fix for removal of T-Systems Deutsche Telekom Root CA 2 
certificate.

Webrev: http://cr.openjdk.java.net/~rhalade/8222137/webrev.00/

Release note is at - https://bugs.openjdk.java.net/browse/JDK-8223161

Thanks,
Rajan

RE: RFR: 8222137: Remove T-Systems root CA certificate

[Langer, Christoph]

Hi Rajan,

I’ll take this to jdk11 updates. What about jdk12 updates? I can process both 
releases, if you want.

Best regards
Christoph

From: security-dev  On Behalf Of Rajan 
Halade
Sent: Dienstag, 30. April 2019 20:39
To: Sean Mullan ; security-dev 

Subject: RFR: 8222137: Remove T-Systems root CA certificate

Please review this fix for removal of T-Systems Deutsche Telekom Root CA 2 
certificate.

Webrev: http://cr.openjdk.java.net/~rhalade/8222137/webrev.00/

Release note is at - https://bugs.openjdk.java.net/browse/JDK-8223161

Thanks,
Rajan

RE: RFR [backport to jdk and 11u]: 8216577: Add GlobalSign's R6 Root certificate

[Langer, Christoph]

Thank you, Rajan.

From: Rajan Halade 
Sent: Dienstag, 30. April 2019 20:08
To: Langer, Christoph 
Cc: security-dev@openjdk.java.net; jdk-updates-...@openjdk.java.net; Sean 
Mullan 
Subject: Re: RFR [backport to jdk and 11u]: 8216577: Add GlobalSign's R6 Root 
certificate

Thanks Christoph for this. I have pushed fix to jdk/jdk repository.

- Rajan
On 4/29/19 12:31 AM, Langer, Christoph wrote:
Hi,

the change to add GlobalSign's R6 Root certificate has only been pushed to 
jdk12 updates (12.0.1) so far. According to [0], it was an oversight to not 
have it added to jdk/jdk. I also want to bring it to 11u.

Taking the change to both, jdk/jdk and jdk-updates/jdk11u-dev does not apply 
cleanly. test/jdk/sun/security/lib/cacerts/VerifyCACerts.java fails to resolve 
because of changed license header and different bug ids in the @bug tag. So, 
please review the resolved change. It's the same for both repos.

Webrev jdk/jdk: http://cr.openjdk.java.net/~clanger/webrevs/8216577.jdk/
Webrev jdk11u-dev: 
http://cr.openjdk.java.net/~clanger/webrevs/8216577.jdk11u-dev/
Bug: https://bugs.openjdk.java.net/browse/JDK-8216577

Please review.

@Rajan Halade<mailto:rajan.hal...@oracle.com>: please let me know whether I 
shall push it to jdk/jdk or feel free to do it yourself.

Thanks & Best regards
Christoph

[0] https://mail.openjdk.java.net/pipermail/security-dev/2019-April/019766.html

RE: Regarding JDK-8216577: Add GlobalSign's R6 Root certificate

[Langer, Christoph]

> >> It should be in 11.0.3-oracle. The backport issue is Confidential so
> >> maybe that is why you thought it wasn't.
> > Yep, that explains it. Any particular reason that the 11.0.3-oracle 
> > backport is
> confidential? Could you make it public? Just asking...
> 
> Fixed.

Thanks!

> >> JDK 13 seems like an oversight. Rajan, any idea what happened? Can you
> >> push this to JDK 13?
> > Thanks in advance. Looking forward to see this in JDK 13.
> 
> Sure, still looking into this.

Ok, I guess you have seen the review mail that I've sent out this morning [0]. 
If this is sufficient, you can just review and I'll push it. But maybe there's 
other constraints on your end that I can't know of. Just let me know. In any 
case, it would be good if you can review my proposal for jdk11u.

Best regards
Christoph

[0] https://mail.openjdk.java.net/pipermail/security-dev/2019-April/019769.html 

RFR [backport to jdk and 11u]: 8216577: Add GlobalSign's R6 Root certificate

[Langer, Christoph]

Hi,

the change to add GlobalSign's R6 Root certificate has only been pushed to 
jdk12 updates (12.0.1) so far. According to [0], it was an oversight to not 
have it added to jdk/jdk. I also want to bring it to 11u.

Taking the change to both, jdk/jdk and jdk-updates/jdk11u-dev does not apply 
cleanly. test/jdk/sun/security/lib/cacerts/VerifyCACerts.java fails to resolve 
because of changed license header and different bug ids in the @bug tag. So, 
please review the resolved change. It's the same for both repos.

Webrev jdk/jdk: http://cr.openjdk.java.net/~clanger/webrevs/8216577.jdk/
Webrev jdk11u-dev: 
http://cr.openjdk.java.net/~clanger/webrevs/8216577.jdk11u-dev/
Bug: https://bugs.openjdk.java.net/browse/JDK-8216577

Please review.

@Rajan Halade: please let me know whether I 
shall push it to jdk/jdk or feel free to do it yourself.

Thanks & Best regards
Christoph

[0] https://mail.openjdk.java.net/pipermail/security-dev/2019-April/019766.html

RE: Regarding JDK-8216577: Add GlobalSign's R6 Root certificate

[Langer, Christoph]

> -Original Message-
> From: Sean Mullan 
> Sent: Samstag, 27. April 2019 00:54
> To: Langer, Christoph ; security-
> d...@openjdk.java.net; Rajan Halade 
> Subject: Re: Regarding JDK-8216577: Add GlobalSign's R6 Root certificate
> 
> On 4/26/19 6:04 PM, Langer, Christoph wrote:
> > Hi,
> >
> > In JBS I can find the bug JDK-8216577: Add GlobalSign's R6 Root
> > certificate [0].
> >
> > This change has gone into 12.0.1 and also 12.0.2 but it's not part of
> > JDK13 (jdk/jdk) and also not of JDK11 (e.g. 11.0.3-oracle,
> > 11.0.4-oracle). Could you please shed some light into this unusual
> > proceeding? Usually such changes would happen in jdk/jdk first, and then
> > be backported, I guess.
> >
> > Is there any reason why the certificate was only added to the jdk12
> > updates train?
> 
> It should be in 11.0.3-oracle. The backport issue is Confidential so
> maybe that is why you thought it wasn't.

Yep, that explains it. Any particular reason that the 11.0.3-oracle backport is 
confidential? Could you make it public? Just asking...

> JDK 13 seems like an oversight. Rajan, any idea what happened? Can you
> push this to JDK 13?

Thanks in advance. Looking forward to see this in JDK 13.

> > Will the certificate be brought to the other update
> > versions later on?
> 
> Which other updates?

Ok, yes, that's all the Oracle updates. I'll bring the change to OpenJDK 11 
updates then.

Thanks
Christoph

Regarding JDK-8216577: Add GlobalSign's R6 Root certificate

[Langer, Christoph]

Hi,

In JBS I can find the bug JDK-8216577: Add GlobalSign's R6 Root certificate [0].

This change has gone into 12.0.1 and also 12.0.2 but it's not part of JDK13 
(jdk/jdk) and also not of JDK11 (e.g. 11.0.3-oracle, 11.0.4-oracle). Could you 
please shed some light into this unusual proceeding? Usually such changes would 
happen in jdk/jdk first, and then be backported, I guess.

Is there any reason why the certificate was only added to the jdk12 updates 
train? Will the certificate be brought to the other update versions later on?

Thanks
Christoph

[0] https://bugs.openjdk.java.net/browse/JDK-8216577

RE: RFR(xxs): 8221407: Windows 32bit build error in libsunmscapi/security.cpp

[Langer, Christoph]

Hi Thomas,

looks good to me.

Best regards
Christoph

From: security-dev  On Behalf Of Thomas 
Stüfe
Sent: Montag, 25. März 2019 14:13
To: security-dev@openjdk.java.net
Subject: RFR(xxs): 8221407: Windows 32bit build error in 
libsunmscapi/security.cpp

Hi all,

please review this tiny fix to the windows 32 build:

issue: https://bugs.openjdk.java.net/browse/JDK-8221407
cr: 
http://cr.openjdk.java.net/~stuefe/webrevs/8221407-windows32-buildfixes-libsunmscapi/webrev.00/webrev/

Just a bunch of obvious fixes for cases where a jlong was handed into system 
APIs requiring HANDLEs, which are pointer sized.

Thanks, Thomas

RE: [8u] Is it possible to bring root certificates to OpenJDK 8 [JEP319] ?

[Langer, Christoph]

Hi Martijn,

as far as I understand the AdoptOpenJDK infrastructure, you have created a 
cacerts file from the Mozilla certificates which you are using in the 
AdoptOpenJDK 8 build via configure option [1]. Is that correct or am I missing 
something?

I was planning to bring the cacerts file from jdk/jdk down to 8 with the 
associated tests. Your build setup should still work then, I guess.

However, if somebody from AdoptOpenJDK wants to do the work of bringing it into 
OpenJDK8 updates, feel free. It’s not the very first thing on my todo list 😊

Thanks & Best regards
Christoph

[1] https://github.com/AdoptOpenJDK/openjdk-build/tree/master/security


From: Martijn Verburg 
Sent: Freitag, 22. März 2019 20:38
To: Sean Mullan 
Cc: Langer, Christoph ; jdk8u-...@openjdk.java.net; 
OpenJDK Dev list 
Subject: Re: [8u] Is it possible to bring root certificates to OpenJDK 8 
[JEP319] ?

FWIW - we backported these in the AdoptOpenJDK 8 builds and could provide a 
patch to upstream that change.

Cheers,
Martijn


On Fri, 22 Mar 2019 at 19:35, Sean Mullan 
mailto:sean.mul...@oracle.com>> wrote:
Hi Christoph,

On 3/21/19 6:20 AM, Langer, Christoph wrote:
> Hi,
>
> I recently came across a scenario where I wanted to use a self-built OpenJDK 
> 8 in a maven build and it could not download artefacts due to missing root 
> certificates. I helped myself by replacing the cacerts with some other 
> version from a later OpenJDK and came over the issue. However, I’ve asked 
> myself whether it was possible/worthwhile to get the root certificates also 
> into an OpenJDK 8 update?
>
> With JEP 319 [0], Oracle has open-sourced the root certificates into OpenJDK. 
> The initial check-in was done for jdk10, via bug JDK-8189131 [1]. After that, 
> several commits have been made to update the set of root certificates and 
> improve the tests.
>
> Now my questions are: Is it legally possible to bring these root certificates 
> also into OpenJDK 8? Since it is a JEP, can the “feature” be added to OpenJDK 
> 8 via an update release? And, last but not least, would there be interest in 
> the community for that at all?

I can answer the first two questions. I talked to one of our Product
Managers who was involved with this JEP and he said that we have
permission to release these certificates as open source at OpenJDK (much
as Mozilla has roots in Firefox).  Therefore there should be no concerns
using with OpenJDK 8 or other versions for that matter.  If you mean the
jdk8u project specifically, you should check with the current
maintainers for interest in this as I think they currently use other
means for their builds.

--Sean

>
> Just trying to start a discussion… 😊
>
> Best regards
> Christoph
>
> [0] http://openjdk.java.net/jeps/319
> [1] https://bugs.openjdk.java.net/browse/JDK-8189131
>

RE: [8u] Is it possible to bring root certificates to OpenJDK 8 [JEP319] ?

[Langer, Christoph]

Hi Sean,

> > Now my questions are: Is it legally possible to bring these root 
> > certificates
> also into OpenJDK 8? Since it is a JEP, can the “feature” be added to OpenJDK
> 8 via an update release? And, last but not least, would there be interest in
> the community for that at all?
> 
> I can answer the first two questions. I talked to one of our Product
> Managers who was involved with this JEP and he said that we have
> permission to release these certificates as open source at OpenJDK (much
> as Mozilla has roots in Firefox).  Therefore there should be no concerns
> using with OpenJDK 8 or other versions for that matter.  If you mean the
> jdk8u project specifically, you should check with the current
> maintainers for interest in this as I think they currently use other
> means for their builds.

Thank you for responding. So I'll check with the jdk8u project whether there's 
interest in this.

Best regards
Christoph

[8u] Is it possible to bring root certificates to OpenJDK 8 [JEP319] ?

[Langer, Christoph]

Hi,

I recently came across a scenario where I wanted to use a self-built OpenJDK 8 
in a maven build and it could not download artefacts due to missing root 
certificates. I helped myself by replacing the cacerts with some other version 
from a later OpenJDK and came over the issue. However, I’ve asked myself 
whether it was possible/worthwhile to get the root certificates also into an 
OpenJDK 8 update?

With JEP 319 [0], Oracle has open-sourced the root certificates into OpenJDK. 
The initial check-in was done for jdk10, via bug JDK-8189131 [1]. After that, 
several commits have been made to update the set of root certificates and 
improve the tests.

Now my questions are: Is it legally possible to bring these root certificates 
also into OpenJDK 8? Since it is a JEP, can the “feature” be added to OpenJDK 8 
via an update release? And, last but not least, would there be interest in the 
community for that at all?

Just trying to start a discussion… 😊

Best regards
Christoph

[0] http://openjdk.java.net/jeps/319
[1] https://bugs.openjdk.java.net/browse/JDK-8189131

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Lance,

thanks for the detailed explanation, sounds great. I’ll work that in in my next 
edition 😊

Best regards
Christoph


From: Lance Andersen 
Sent: Mittwoch, 13. Februar 2019 23:53
To: Langer, Christoph 
Cc: Alan Bateman ; nio-dev ; 
Java Core Libs ; OpenJDK Dev list 
; Volker Simonis 
Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Christoph
On Feb 13, 2019, at 5:30 PM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi Lance,

thanks for looking.


Just starting to take a peek at this and noticed one quick thing in your test:

Paths.get(System.getProperty("test.dir", "."), "testPosix.zip")
——

You do not need the test.dir property  or the permission added to test.policy
to access it,  just reference the jar and it will be created in user.dir which 
is
also writable.

Hm, I thought I didn't want to mess around in "user.dir" as it can be some more 
global directory where you wouldn't want to leave artefacts... To me "test.dir" 
feels cleaner. Are there other opinions about that?

user.dir points to the scratch directory that test uses, so it is where you 
want to create the tests.  Workspaces can sometimes be read only:

For example:
——

@Test
public void test000() throws IOException {
System.out.println("test.dir = " +
System.getProperty("test.dir", "."));
System.out.println("user.dir = " +
System.getProperty("user.dir", "."));
System.out.println(
Paths.get(System.getProperty("test.dir", "."), 
"basic.jar").toAbsolutePath()
);
}

-

Results in:

test.dir = .
user.dir = 
/Users/ljanders/Documents/hg-workspaces/openjdk-jdk/jdk-zip-api/build/macosx-x64/JTwork/scratch
/Users/ljanders/Documents/hg-workspaces/openjdk-jdk/jdk-zip-api/build/macosx-x64/JTwork/scratch/./basic.jar

Please see http://openjdk.java.net/jtreg/tag-spec.html for the system 
properties.  I do not see test.dir there.
—

I would just do:

—

Path foo = Path.of("test.zip");
System.out.println("test.zip path=" + foo.toAbsolutePath());

--

which results in the output:
test.zip 
path=/Users/ljanders/Documents/hg-workspaces/openjdk-jdk/jdk-zip-api/build/macosx-x64/JTwork/scratch/test.zip

Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com<mailto:lance.ander...@oracle.com>

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Lance,

thanks for looking.

> Just starting to take a peek at this and noticed one quick thing in your test:
> 
> Paths.get(System.getProperty("test.dir", "."), "testPosix.zip")
> ——
> 
> You do not need the test.dir property  or the permission added to test.policy
> to access it,  just reference the jar and it will be created in user.dir 
> which is
> also writable.

Hm, I thought I didn't want to mess around in "user.dir" as it can be some more 
global directory where you wouldn't want to leave artefacts... To me "test.dir" 
feels cleaner. Are there other opinions about that?

Thanks
Christoph

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan, all,

here comes the next proposal for POSIX support in jdk.zipfs - which hopefully 
represents the converged solution, at least in its overall design.

http://cr.openjdk.java.net/~clanger/webrevs/8213031.6/

With that patch, the behavior is the following:
a) A ZipFileSystem instance by default does NOT support the 
PosixFileAttributeView. However, it is possible to query for a known attribute 
named "zip:storedPermissions". Its value can be null to represent no  
permission information or any set of PosixFileAttribute.
b) A ZipFileSystem instance can be created with the property "posix"=true. In 
that case, PosixFileAttributeView is supported with default values.
-> owner: A UserPrincipal with the name of 
System.getProperty("user.name") if property access is allowed, 
"" otherwise
-> group: A GroupPrincipal with the name "".
-> permissions: Set.of(OWNER_READ, OWNER_WRITE, GROUP_READ)

The default values can be modified by using the properties 
"defaultOwner", "defaultGroup" and "defaultPermissions".

Implementation wise the ZipFileAttributeView always extends 
PosixFileAttributeView but behaves different, depending on how it was obtained.

Within ZipFileSystem, the Entry inner class is not static any more but always 
has a reference to the Enclosing ZipFileSystem. That's needed because default 
owner/group/permission can be set on ZipFileSystem instance level now and the 
Entry, implementing the FileAttributes needs to know where it belongs to. This 
seems somewhat logical anyway.

The new test "TestPosix" is quite extensive. I had to add 2 permissions to 
test.policy to allow testing in a restricted environment.

Module-info and CSR have been adopted, too.

Thanks in advance for reviewing.

Best regards
Christoph

> -Original Message-
> From: Langer, Christoph
> Sent: Montag, 21. Januar 2019 10:17
> To: 'Alan Bateman' 
> Cc: nio-dev ; OpenJDK Dev list  d...@openjdk.java.net>; Java Core Libs ;
> Volker Simonis 
> Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions
> 
> Hi Alan,
> 
> first of all, thank you for your input on this.
> 
> > I think the approach to explore are:
> >
> > 1. zipfs supports PosixFileAttributeView without subsetting. If
> > readAttribute(file, BasicFileAttributes.class) succeeds then
> > readAttribute(file, PosixFileAttributes.class) should also succeed, even
> > if there aren't permissions encoded in the zip entry's external file
> > attributes. It would mean that owner and group return default values,
> > and permissions may return a default value. It does mean you can't
> > distinguish the default value from "no permissions" but there is
> > precedence for that, e.g. mount a FAT32 file system on Linux or Unix
> > systems and `stat` a file to have the stat structure populated with
> > default uid, gid and mode bits.
> 
> OK, I can see the point that in a PosixFileAttributeView as it is, there's no
> place for optionality/null values. However, with this approach the benefits
> would be that Files::get/setPosixPermissions would work and that's why I
> think we should pursue this. The challenge will be to find reasonable
> defaults.
> 
> > 2. zipfs defines a new FileAttributeView that defines read and write
> > access to permissions stored in a zip entry's external file attribute.
> > As it's a new view then it can define the behavior for the case that the
> > zip entry doesn't have permissions. Furthermore it does not need to
> > extend BasicFileAttributeView so doesn't need to be concerned with bulk
> > access, nor concerned with group/owner. As you know, the attributes API
> > allows for both type safe and dynamic access so you have a choice as to
> > whether to support both or just dynamic access. With the first then
> > jdk.zipfs would export a package with a public interface that defines
> > the view. If someone wants type safe access to the permissions attribute
> > then you need to import the class. The alternative is to not export any
> > packages but just define the view in the module-info. The view its name
> > and define the name/type of the permissions attribute, it will also
> > define how it behaves when the external attributes aren't populated. In
> > usage terms it means reading the permissions will be something like
> > Files.readAttribute(file, "zip:permissions") and casting the value to
> > Set - not pretty but it avoids depending on a
> > JDK-specific API.
> 
> For this approach, there are 2 things I dislike. The first is that I don't 
> think we
> should e

RE: RFR [11u backport]: 8217579: TLS_EMPTY_RENEGOTIATION_INFO_SCSV is disabled after 8211883

[Langer, Christoph]

Thanks Sean. Pushed with the replacement as you suggested.

> -Original Message-
> From: Sean Mullan 
> Sent: Donnerstag, 31. Januar 2019 21:03
> To: Langer, Christoph ; security-
> d...@openjdk.java.net
> Subject: Re: RFR [11u backport]: 8217579:
> TLS_EMPTY_RENEGOTIATION_INFO_SCSV is disabled after 8211883
> 
> CheckCipherSuites.java
> 
> 116 // List of enabled cipher suites when the "crypto.policy" security
> 
> typo: s/enabled/supported/
> 
> (I realized that typo after I had already pushed the fix to JDK 13, but
> better to just fix it here now).
> 
> Otherwise looks good.
> 
> --Sean
> 
> On 1/31/19 8:36 AM, Langer, Christoph wrote:
> > Hi,
> >
> > please review the backport of the fix for 8217579 to jdk11u.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8217579
> >
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8217579.11u/
> >
> > Original review thread:
> > https://mail.openjdk.java.net/pipermail/security-dev/2019-
> January/019256.html
> >
> > The patch did apply cleanly but I had to remove the ChaCha ciphers to
> > make the test work with JDK 11.
> >
> > Thanks and Best regards
> >
> > Christoph
> >

RFR [11u backport]: 8217579: TLS_EMPTY_RENEGOTIATION_INFO_SCSV is disabled after 8211883

[Langer, Christoph]

Hi,

please review the backport of the fix for 8217579 to jdk11u.

Bug: https://bugs.openjdk.java.net/browse/JDK-8217579
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8217579.11u/
Original review thread: 
https://mail.openjdk.java.net/pipermail/security-dev/2019-January/019256.html

The patch did apply cleanly but I had to remove the ChaCha ciphers to make the 
test work with JDK 11.

Thanks and Best regards
Christoph

RE: 8217579: TLS_EMPTY_RENEGOTIATION_INFO_SCSV is gone after 8211883

[Langer, Christoph]

Hi Sean,

to me this looks fine. +1

The test should be really valuable in the future.

Thanks & Best regards
Christoph

> -Original Message-
> From: security-dev  On Behalf Of
> Sean Mullan
> Sent: Montag, 28. Januar 2019 22:25
> To: security-dev@openjdk.java.net
> Subject: Re: 8217579: TLS_EMPTY_RENEGOTIATION_INFO_SCSV is gone after
> 8211883
> 
> Updated webrev:
> http://cr.openjdk.java.net/~mullan/webrevs/8217579/webrev.01/
> 
> Comments inline ...
> 
> On 1/28/19 2:54 PM, Bernd Eckenfels wrote:
> > Hello Sean,
> >
> > Maybe you also want to change comment and name of the
> SUPPORTE_DDEFAULT
> > Array to „SUPPORTED_LIMITED“ since Unlimited is now Default?
> >
> >      private final static String[] ENABLED_DEFAULT
> >
> > ….
> >
> >   // supported ciphersuites using default JCE policy jurisdiction files
> >
> >   // AES/256 unavailable
> >
> >   private final static String[] SUPPORTED_DEFAULT = {
> >
> > 230 – remove „Default
> 
> Good point. I have renamed the *_UNLIMITED constants to *_DEFAULT and
> renamed the *_DEFAULT constants to *_LIMITED.
> 
> > Is the test already run with all available policies? With the new System
> > property it should be easy to run it with other/vm twice?
> 
> Good point. I have changed the test to use the crypto.policy security
> property to test the suites with the default and limited policies.
> 
> > Is Oracle considering pushing a emergency public update for this?
> 
> We are planning to backport it to all affected releases.
> 
> > The change Looks otherwise fine (I was first wondering if checking for a
> > _SVCS Family makes more sense but I guess that can be done once we
> have
> > more of those ciphers.
> 
> Ok, thanks for the review.
> 
> --Sean
> 
> >
> > Gruss
> >
> > Bernd
> >
> > --
> > http://bernd.eckenfels.net
> >
> > *Von: *Sean Mullan 
> > *Gesendet: *Montag, 28. Januar 2019 20:26
> > *An: *security Dev OpenJDK 
> > *Betreff: *RFR: 8217579: TLS_EMPTY_RENEGOTIATION_INFO_SCSV is gone
> after
> > 8211883
> >
> > This fixes a regression introduced by the recent change to disable the
> >
> > TLS NULL cipher suites [1]. This accidentally also disabled the
> >
> > TLS_EMPTY_RENEGOTIATION_INFO_SCSV cipher suite because when the
> name is
> >
> > decomposed by the algorithm constraints checking code it has NULL for
> >
> > its different parts (key exchange, etc). But this cipher suite is not
> >
> > negotiable and is only used for renegotiation purposes as defined in RFC
> >
> > 5746. It should not have been disabled.
> >
> > I also resurrected the CheckCipherSuites test which had an @ignore label
> >
> > on it. This is a good test because it checks what the expected
> >
> > enabled/supported suites should be, and will help catch issues like this
> >
> > in the future.
> >
> > webrev:
> http://cr.openjdk.java.net/~mullan/webrevs/8217579/webrev.00/
> >
> > bug: https://bugs.openjdk.java.net/browse/JDK-8217579
> >
> > Thanks,
> >
> > Sean
> >
> > [1] https://bugs.openjdk.java.net/browse/JDK-8211883
> >

RE: RFR 8217657: Move the test for default value of jdk.includeInExceptions into own test

[Langer, Christoph]

Hi Sean,

thanks for looking at this. I agree. Will remove othervm...

Best regards
Christoph

> -Original Message-
> From: Sean Mullan 
> Sent: Donnerstag, 24. Januar 2019 17:43
> To: Langer, Christoph ; OpenJDK Dev list
> ; OpenJDK Network Dev list  d...@openjdk.java.net>
> Subject: Re: RFR 8217657: Move the test for default value of
> jdk.includeInExceptions into own test
> 
> I don't think you really need to run the test with the othervm flag,
> unless you are concerned other tests may be setting this property and
> (incorrectly) not running in a separate VM, which would be a bug in my
> opinion. Well, then maybe you should run it in othervm just in case.
> Otherwise, looks ok to me.
> 
> --Sean
> 
> On 1/23/19 11:05 AM, Langer, Christoph wrote:
> > Hi,
> >
> > please review a small test update.
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8217657
> >
> > Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8217657.0/
> >
> > I'd like to move the test for the correct default value of security
> > property "jdk.includeInExceptions" into an own testcase in the
> > jdk.security area. This seems a bit more natural than to hide this check
> > in a java/net specific test and will help finding/maintaining tests when
> > the (default-)value for that property changes. For instance new values
> > get added or other OpenJDK builds have different defaults (e.g.
> SAPMachine).
> >
> > Thanks
> >
> > Christoph
> >

RE: RFR 8217657: Move the test for default value of jdk.includeInExceptions into own test

[Langer, Christoph]

Hi Goetz,

thanks for the review.

I've added the comment as you suggested: 
http://cr.openjdk.java.net/~clanger/webrevs/8217657.1/

Will run it through our nightly tests before submitting...

/Christoph


From: Lindenmaier, Goetz
Sent: Donnerstag, 24. Januar 2019 08:48
To: Langer, Christoph ; OpenJDK Dev list 
; OpenJDK Network Dev list 

Subject: RE: RFR 8217657: Move the test for default value of 
jdk.includeInExceptions into own test

Hi Christoph,

it is a good idea to keep testing these two matters separately.

Could you please document in the new test that in OpenJDK
it is decided to keep this property empty?
Something like:
@comment In OpenJDK, this property is empty by default and on purpose. This 
test assures the default is not changed.

Otherwise, looks good.

Best regards,
  Goetz.

From: net-dev 
mailto:net-dev-boun...@openjdk.java.net>> On 
Behalf Of Langer, Christoph
Sent: Mittwoch, 23. Januar 2019 17:06
To: OpenJDK Dev list 
mailto:security-dev@openjdk.java.net>>; OpenJDK 
Network Dev list mailto:net-...@openjdk.java.net>>
Subject: [CAUTION] RFR 8217657: Move the test for default value of 
jdk.includeInExceptions into own test

Hi,

please review a small test update.

Bug: https://bugs.openjdk.java.net/browse/JDK-8217657
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8217657.0/

I'd like to move the test for the correct default value of security property 
"jdk.includeInExceptions" into an own testcase in the jdk.security area. This 
seems a bit more natural than to hide this check in a java/net specific test 
and will help finding/maintaining tests when the (default-)value for that 
property changes. For instance new values get added or other OpenJDK builds 
have different defaults (e.g. SAPMachine).

Thanks
Christoph

RFR 8217657: Move the test for default value of jdk.includeInExceptions into own test

[Langer, Christoph]

Hi,

please review a small test update.

Bug: https://bugs.openjdk.java.net/browse/JDK-8217657
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8217657.0/

I'd like to move the test for the correct default value of security property 
"jdk.includeInExceptions" into an own testcase in the jdk.security area. This 
seems a bit more natural than to hide this check in a java/net specific test 
and will help finding/maintaining tests when the (default-)value for that 
property changes. For instance new values get added or other OpenJDK builds 
have different defaults (e.g. SAPMachine).

Thanks
Christoph

Shall TLS_EMPTY_RENEGOTIATION_INFO_SCSV really be disabled via jdk.tls.disabledAlgorithms=...,NULL or is it an unwanted side effect of JDK-8211883?

[Langer, Christoph]

Hi security experts,

one of our customers is running into an issue with a Tomcat application after 
JDK-8211883 [1]. It seems that because of adding NULL to 
jdk.tls.disabledAlgorithms, the pseudo cipher suite 
TLS_EMPTY_RENEGOTIATION_INFO_SCSV is disabled. Seems like, according to 
CipherSuite.java [2], it is considered a NULL cipher. The tracing/reproducer 
shows that it’s definitely disabled via jdk.tls.disabledAlgorithms=NULL.

However, with my limited knowledge of TLS and ciphersuites and googling around, 
I understand that TLS_EMPTY_RENEGOTIATION_INFO_SCSV is part of the RFC 5746 
specification [3], which is still considered secure and state of the art for 
renegotiation. Is that correct?

The effect now in the customer application is that the client sends the SCSV 
and the Tomcat SSL Engine checks for the presence of the SCSV cipher in the 
cipher suites [4]. Since it is not present, the handshake is stopped by 
removing all ciphers [5].

I also understand the Oracle readme about the renegotiation topic, that 
TLS_EMPTY_RENEGOTIATION_INFO_SCSV is a thing to have but not to disable.

Please let me know, if you agree with my analysis. If so, could you please file 
a bug or tell me to do so? Otherwise let me know what I’m missing. The 
workaround for the customer is to remove the NULL entry from 
jdk.tls.disabledAlgorithms for the time being. I guess that’s a bit more secure 
than setting “sun.security.ssl.allowUnsafeRenegotiation” 😉

Thanks
Christoph

[1] https://bugs.openjdk.java.net/browse/JDK-8211883
[2] 
http://hg.openjdk.java.net/jdk/jdk/file/1ae823617395/src/java.base/share/classes/sun/security/ssl/CipherSuite.java#l312
[3] http://www.ietf.org/rfc/rfc5746.txt
[4] 
https://github.com/apache/tomcat70/blob/600d5c81aafee7e95fe07c3b9182f37741e2d0d8/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java#L145
[5] 
https://github.com/apache/tomcat70/blob/600d5c81aafee7e95fe07c3b9182f37741e2d0d8/java/org/apache/tomcat/util/net/jsse/JSSESocketFactory.java#L293
[6] 
https://www.oracle.com/technetwork/java/javase/overview/tlsreadme2-176330.html

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan,

first of all, thank you for your input on this.

> I think the approach to explore are:
> 
> 1. zipfs supports PosixFileAttributeView without subsetting. If
> readAttribute(file, BasicFileAttributes.class) succeeds then
> readAttribute(file, PosixFileAttributes.class) should also succeed, even
> if there aren't permissions encoded in the zip entry's external file
> attributes. It would mean that owner and group return default values,
> and permissions may return a default value. It does mean you can't
> distinguish the default value from "no permissions" but there is
> precedence for that, e.g. mount a FAT32 file system on Linux or Unix
> systems and `stat` a file to have the stat structure populated with
> default uid, gid and mode bits.

OK, I can see the point that in a PosixFileAttributeView as it is, there's no 
place for optionality/null values. However, with this approach the benefits 
would be that Files::get/setPosixPermissions would work and that's why I think 
we should pursue this. The challenge will be to find reasonable defaults.

> 2. zipfs defines a new FileAttributeView that defines read and write
> access to permissions stored in a zip entry's external file attribute.
> As it's a new view then it can define the behavior for the case that the
> zip entry doesn't have permissions. Furthermore it does not need to
> extend BasicFileAttributeView so doesn't need to be concerned with bulk
> access, nor concerned with group/owner. As you know, the attributes API
> allows for both type safe and dynamic access so you have a choice as to
> whether to support both or just dynamic access. With the first then
> jdk.zipfs would export a package with a public interface that defines
> the view. If someone wants type safe access to the permissions attribute
> then you need to import the class. The alternative is to not export any
> packages but just define the view in the module-info. The view its name
> and define the name/type of the permissions attribute, it will also
> define how it behaves when the external attributes aren't populated. In
> usage terms it means reading the permissions will be something like
> Files.readAttribute(file, "zip:permissions") and casting the value to
> Set - not pretty but it avoids depending on a
> JDK-specific API.

For this approach, there are 2 things I dislike. The first is that I don't 
think we should export named packages from module jdk.zipfs that people would 
develop Java code against while not being in the Java API. And secondly, this 
way would not support using Files::set/getPosixPermissions since the 
specification/implementation of that utility method explicitly refers to 
PosixFileAttributeView.

I can imagine something like this:
Zipfs by default implements an own view that offers dynamic, not type safe 
access to "zip:permissions" and we'll document this. If a user of zipfs wants 
to see full PosixFileAttributeView support with default values, then we should 
allow for a creation attribute for the zipfs that can control this. Maybe we 
can even allow specifying default values for user, group and permissions via 
zipfs attributes.

I'll work to develop the patch into this direction unless you tell me that this 
idea is bogus (if so, then I hope it be soon 😊)

Thanks
Christoph

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan,

> I will try to get time next week to reply to you on this. Part of the
> issue with your current approach is that it breaks PosixFileAttribtues.
> There are also issues trying to force the API to optionally support a
> subset of POSIX attributes on some zip entries and not on others. So
> several issues that will need consideration.

Thanks in advance for your work. Well, I can see that we could also support 
users/groups with zip files, which is also implemented in InfoZIP. Or we might 
add a Sub-Interface like 
PosixPermissionAttributes/PosixPermissionAttributesView. However, in any case, 
we have to find a way to deal with the optionality of any POSIX attributes in 
zip files.

So, let's see what you come up with 😊

/Christoph

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Lance,

I was not aware of JDK-8182117 and by its description it does not fit exactly 
to the updates to jdk.zipfs module documentation that I propose. However, yes, 
it is probably a bit more natural to include that part in a potential patch for 
JDK-8182117. So, feel free to take this over into your work. Seeing how things 
are going with this POSIX permission topic, I’m sure you’ll be done with a 
patch for 8182117 much earlier than me 😉.

Best
Christoph

From: Lance Andersen 
Sent: Samstag, 12. Januar 2019 15:01
To: Langer, Christoph 
Cc: Alan Bateman ; nio-dev ; 
OpenJDK Dev list ; Java Core Libs 

Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Christoph
On Jan 12, 2019, at 8:02 AM, Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:

Hi Alan,

as I did not hear back from you I continued to work on the POSIX file 
permission support for zipfs and specified/implemented the value of 'null' for 
zip entries with no permission information associated (vs. 
UnsupportedOperationException).

I updated the CSR: https://bugs.openjdk.java.net/browse/JDK-8213082
And here is an updated webrev for the change: 
http://cr.openjdk.java.net/~clanger/webrevs/8213031.5/

I also changed the first part of the module-info documentation for jdk.zipfs, 
mentioning the URI scheme 'jar' and  corrected the information about how the 
zip file system provider can be accessed and new zip filesystems can be
created.

I think the above should be addressed via JDK-8182117 which I will be 
addressing and keep this focused on the POSIX  file permission enhancements as 
I think it should be in its own CSR.

Best
Lance


Please kindly review this.

Thank you and best regards
Christoph


-Original Message-----
From: Langer, Christoph
Sent: Dienstag, 8. Januar 2019 09:27
To: 'Alan Bateman' mailto:alan.bate...@oracle.com>>; 
Volker Simonis
mailto:volker.simo...@gmail.com>>
Cc: nio-dev mailto:nio-...@openjdk.java.net>>; 
OpenJDK Dev list mailto:d...@openjdk.java.net>>; Java Core Libs 
mailto:core-libs-...@openjdk.java.net>>
Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Alan, Volker,

thanks for bringing up these discussion points. I agree that we shall carefully
revisit that.

First of all, I'd like to point out that PosixFileAttributeView::readAttributes
won't ever throw UOE with the proposed changes to zipfs. It should always
return an object of type PosixFileAttributes which will be an incarnation of
ZipFileSystem.Entry.

The returned PosixFileAttributes(ZipFileSystem.Entry) object now
implements 3 additional methods: owner(), group() and permissions(). I can
see that UOE should only be thrown if something is not supported by an
implementation at all. So it perfectly fits to owner() and group() because this
is simply not implemented in zipfs (yet...). As for permissions, I then agree,
UOE probably isn't the right thing to do because permissions will now be
supported by zipfs.
Still, we need to distinguish between the case where no permission
information is present vs. an empty set of permissions that was explicitly
stored. You brought up IOException as an alternative. But I think this is not
really feasible for the following main reason: IOE is no RuntimeException, so
it has to be declared for a method when it is thrown.
PosixFileAttributes::permissions, however, does not declare IOE so far. And I
don't think we can/want to modify the PosixFileAttributes interface for the
sake of that zipfs implementation change.

I think, we should look into using/returning null for "no permission
information present".

With that, the point is: What happens if we pass null to another
PosixFileAttributeView::setPermissions implementation (or to
Files::setPosixFilePermissions, which ends up there). For zipfs, with the
proposed changeset, this would work perfectly fine. We translate the null
value into (int)-1 for the "posixPerms" field of "Entry" and will then not set
permission information in the zip file. But other places in the JDK that
implement PosixFileAttributeView need some rework. Those are:
src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.Posix
src/java.base/unix/classes/sun/nio/fs/UnixSecureDirectoryStream.PosixFile
AttributeViewImpl

And we should amend the specs/doc for the following methods to define
the handling of the null value as a noop:
PosixFileAttributeView::setPermissions
PosixFileAttributes::permissions
Files::setPosixFilePermissions
Files::getPosixFilePermissions

In the implementation I can see that we modify the aforementioned places
to handle null by translating it to (int)-1 in
UnixFileModeAttribute::toUnixMode and then using this value as noop in
'setMode' resp. 'fchmod'.

Do you think this is the right way to go? Should we maybe do the spec
update of the permission methods in a separate change?

Best 

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan,

as I did not hear back from you I continued to work on the POSIX file 
permission support for zipfs and specified/implemented the value of 'null' for 
zip entries with no permission information associated (vs. 
UnsupportedOperationException).

I updated the CSR: https://bugs.openjdk.java.net/browse/JDK-8213082
And here is an updated webrev for the change: 
http://cr.openjdk.java.net/~clanger/webrevs/8213031.5/

I also changed the first part of the module-info documentation for jdk.zipfs, 
mentioning the URI scheme 'jar' and  corrected the information about how the 
zip file system provider can be accessed and new zip filesystems can be created.

Please kindly review this.

Thank you and best regards
Christoph

> -Original Message-
> From: Langer, Christoph
> Sent: Dienstag, 8. Januar 2019 09:27
> To: 'Alan Bateman' ; Volker Simonis
> 
> Cc: nio-dev ; OpenJDK Dev list  d...@openjdk.java.net>; Java Core Libs 
> Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions
> 
> Hi Alan, Volker,
> 
> thanks for bringing up these discussion points. I agree that we shall 
> carefully
> revisit that.
> 
> First of all, I'd like to point out that 
> PosixFileAttributeView::readAttributes
> won't ever throw UOE with the proposed changes to zipfs. It should always
> return an object of type PosixFileAttributes which will be an incarnation of
> ZipFileSystem.Entry.
> 
> The returned PosixFileAttributes(ZipFileSystem.Entry) object now
> implements 3 additional methods: owner(), group() and permissions(). I can
> see that UOE should only be thrown if something is not supported by an
> implementation at all. So it perfectly fits to owner() and group() because 
> this
> is simply not implemented in zipfs (yet...). As for permissions, I then agree,
> UOE probably isn't the right thing to do because permissions will now be
> supported by zipfs.
> Still, we need to distinguish between the case where no permission
> information is present vs. an empty set of permissions that was explicitly
> stored. You brought up IOException as an alternative. But I think this is not
> really feasible for the following main reason: IOE is no RuntimeException, so
> it has to be declared for a method when it is thrown.
> PosixFileAttributes::permissions, however, does not declare IOE so far. And I
> don't think we can/want to modify the PosixFileAttributes interface for the
> sake of that zipfs implementation change.
> 
> I think, we should look into using/returning null for "no permission
> information present".
> 
> With that, the point is: What happens if we pass null to another
> PosixFileAttributeView::setPermissions implementation (or to
> Files::setPosixFilePermissions, which ends up there). For zipfs, with the
> proposed changeset, this would work perfectly fine. We translate the null
> value into (int)-1 for the "posixPerms" field of "Entry" and will then not set
> permission information in the zip file. But other places in the JDK that
> implement PosixFileAttributeView need some rework. Those are:
> src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.Posix
> src/java.base/unix/classes/sun/nio/fs/UnixSecureDirectoryStream.PosixFile
> AttributeViewImpl
> 
> And we should amend the specs/doc for the following methods to define
> the handling of the null value as a noop:
> PosixFileAttributeView::setPermissions
> PosixFileAttributes::permissions
> Files::setPosixFilePermissions
> Files::getPosixFilePermissions
> 
> In the implementation I can see that we modify the aforementioned places
> to handle null by translating it to (int)-1 in
> UnixFileModeAttribute::toUnixMode and then using this value as noop in
> 'setMode' resp. 'fchmod'.
> 
> Do you think this is the right way to go? Should we maybe do the spec
> update of the permission methods in a separate change?
> 
> Best regards
> Christoph
> 
> > -Original Message-
> > From: Alan Bateman 
> > Sent: Montag, 7. Januar 2019 21:46
> > To: Volker Simonis 
> > Cc: Langer, Christoph ; nio-dev  > d...@openjdk.java.net>; OpenJDK Dev list  > d...@openjdk.java.net>; Java Core Libs 
> > Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions
> >
> > On 07/01/2019 19:26, Volker Simonis wrote:
> > > :
> > > We considered this, but it is problematic because it is perfectly
> > > valid to have a file with external file attributes where none of the
> > > Posix attributes is actually set (i.e. an empty set of Posix files
> > > attributes). This wouldn't be distinguishable from the case where a
> > > file has no exter

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan, Volker,

thanks for bringing up these discussion points. I agree that we shall carefully 
revisit that.

First of all, I'd like to point out that PosixFileAttributeView::readAttributes 
won't ever throw UOE with the proposed changes to zipfs. It should always 
return an object of type PosixFileAttributes which will be an incarnation of 
ZipFileSystem.Entry.

The returned PosixFileAttributes(ZipFileSystem.Entry) object now implements 3 
additional methods: owner(), group() and permissions(). I can see that UOE 
should only be thrown if something is not supported by an implementation at 
all. So it perfectly fits to owner() and group() because this is simply not 
implemented in zipfs (yet...). As for permissions, I then agree, UOE probably 
isn't the right thing to do because permissions will now be supported by zipfs.
Still, we need to distinguish between the case where no permission information 
is present vs. an empty set of permissions that was explicitly stored. You 
brought up IOException as an alternative. But I think this is not really 
feasible for the following main reason: IOE is no RuntimeException, so it has 
to be declared for a method when it is thrown. 
PosixFileAttributes::permissions, however, does not declare IOE so far. And I 
don't think we can/want to modify the PosixFileAttributes interface for the 
sake of that zipfs implementation change.

I think, we should look into using/returning null for "no permission 
information present".

With that, the point is: What happens if we pass null to another 
PosixFileAttributeView::setPermissions implementation (or to 
Files::setPosixFilePermissions, which ends up there). For zipfs, with the 
proposed changeset, this would work perfectly fine. We translate the null value 
into (int)-1 for the "posixPerms" field of "Entry" and will then not set 
permission information in the zip file. But other places in the JDK that 
implement PosixFileAttributeView need some rework. Those are:
src/java.base/unix/classes/sun/nio/fs/UnixFileAttributeViews.Posix
src/java.base/unix/classes/sun/nio/fs/UnixSecureDirectoryStream.PosixFileAttributeViewImpl

And we should amend the specs/doc for the following methods to define the 
handling of the null value as a noop:
PosixFileAttributeView::setPermissions
PosixFileAttributes::permissions
Files::setPosixFilePermissions
Files::getPosixFilePermissions

In the implementation I can see that we modify the aforementioned places to 
handle null by translating it to (int)-1 in UnixFileModeAttribute::toUnixMode 
and then using this value as noop in 'setMode' resp. 'fchmod'.

Do you think this is the right way to go? Should we maybe do the spec update of 
the permission methods in a separate change?
 
Best regards
Christoph

> -Original Message-
> From: Alan Bateman 
> Sent: Montag, 7. Januar 2019 21:46
> To: Volker Simonis 
> Cc: Langer, Christoph ; nio-dev  d...@openjdk.java.net>; OpenJDK Dev list  d...@openjdk.java.net>; Java Core Libs 
> Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions
> 
> On 07/01/2019 19:26, Volker Simonis wrote:
> > :
> > We considered this, but it is problematic because it is perfectly
> > valid to have a file with external file attributes where none of the
> > Posix attributes is actually set (i.e. an empty set of Posix files
> > attributes). This wouldn't be distinguishable from the case where a
> > file has no external file attributes. So it seems we have to resort to
> > throwing an IOE?
> >
> Maybe although it would be a bit awkward to deal with. The issues around
> this remind me a bit about mounting fat32 file systems on Linux or Unix
> systems where the fields in the stat structure are populated with
> default values. PosixFileAttributeView::readAttributes is  essentially
> the equivalent of a stat call. This might be something to look at for
> the file owner at least.
> 
> -Alan

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi,

I’ve amended the jdk.zipfs module documentation in 
src/jdk.zipfs/share/classes/module-info.java to document the new behavior (e.g. 
support of PosixFileAttributeView) as requested by Alan. I’ve also updated the 
CSR.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.4/
CSR: https://bugs.openjdk.java.net/browse/JDK-8213082

Please review both, CSR and changeset and let me know if this is good now or 
what else needs to be addressed.

Thanks and best regards,
Christoph

From: Volker Simonis 
Sent: Freitag, 21. Dezember 2018 17:34
To: Langer, Christoph 
Cc: Java Core Libs ; security-dev 
; SHEN, XUEMING ; Alan 
Bateman ; nio-dev 
Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

Hi Christoph,

thanks for updating the change. I think it is in a good state now and ready to 
go!

Also the documentation in the CSR for this issue 
(https://bugs.openjdk.java.net/browse/JDK-8213082) is greatly appreciated and 
answers all the questions which have been raised so far. So if there are still 
any open questions I'd recommend that any potential reviewer consults the CSR 
at https://bugs.openjdk.java.net/browse/JDK-8213082 first.

Thank you and best regards,
Volker


On Fri, Dec 21, 2018 at 3:31 PM Langer, Christoph 
mailto:christoph.lan...@sap.com>> wrote:
Hi all,

here comes the updated webrev: 
http://cr.openjdk.java.net/~clanger/webrevs/8213031.3/

I've rebased the change to the current state of the JDK depot. Thanks to 
Volker, the test has been enhanced and now also tests more copy operations 
(from zip file system to zip file system and from zip file system to default 
file system and back).

The points below were also addressed:

> ZipFileAttributeView.java
>  - can you please throw an AssertionError() for the default branch in
> the switch expression in the "ZipFileAttributeView.name()".

The default branch will return "basic". Looking at the code it should not be 
jumped to anyway.

> ZipFileSystem.java
>  - please also put @Override annotations on the method implementations
> from ZipFileAttributes (e.g. "compressedSize()", "crc()", etc...) and
> a comment at the place where the implementation of the
> "PosixFileAttributes" methods starts.

Done, I also reordered the methods - first "basic" then "posix" then "zip".

> ZipUtils.java
> - just a question: isn't it possible to reuse
> sun.nio.fs.UnixFileModeAttribute.toUnixMode() and the corresponding
> constants from sun.nio.fs.UnixConstants instead of redefining them
> here? You could export them from java.base to jdk.zipfs but the
> problem is probably that at least sun.nio.fs.UnixConstants is a
> generated class which only gets generated on Unix systems, right ?

You've already answered yourself 😊 These classes only exist on Unix type JDKs.

> ZipFileAttributes.java
> - it seems a little odd that you've added "setPermissions()" to
> ZipFileAttributes. All the attribute classes (i.e.
> BasicFileAttributes, PosixFileAttributes) have no setters. Isn't it
> possible to implement "ZipFileAttributeView.setPermissions()" by
> calling "path.setPermissions()" (as this is done in
> "ZipFileAttributeView.setTimes()") and handle everything in
> ZipFileSyste (as this is done with "setTimes()").

Good catch & thanks for providing the better implementation.


I think this version is quite final now and thoroughly tested. I hope to get 
some valid reviews soon...

Thanks
Christoph

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi all,

here comes the updated webrev: 
http://cr.openjdk.java.net/~clanger/webrevs/8213031.3/

I've rebased the change to the current state of the JDK depot. Thanks to 
Volker, the test has been enhanced and now also tests more copy operations 
(from zip file system to zip file system and from zip file system to default 
file system and back).

The points below were also addressed:

> ZipFileAttributeView.java
>  - can you please throw an AssertionError() for the default branch in
> the switch expression in the "ZipFileAttributeView.name()".

The default branch will return "basic". Looking at the code it should not be 
jumped to anyway.

> ZipFileSystem.java
>  - please also put @Override annotations on the method implementations
> from ZipFileAttributes (e.g. "compressedSize()", "crc()", etc...) and
> a comment at the place where the implementation of the
> "PosixFileAttributes" methods starts.

Done, I also reordered the methods - first "basic" then "posix" then "zip".

> ZipUtils.java
> - just a question: isn't it possible to reuse
> sun.nio.fs.UnixFileModeAttribute.toUnixMode() and the corresponding
> constants from sun.nio.fs.UnixConstants instead of redefining them
> here? You could export them from java.base to jdk.zipfs but the
> problem is probably that at least sun.nio.fs.UnixConstants is a
> generated class which only gets generated on Unix systems, right ?

You've already answered yourself 😊 These classes only exist on Unix type JDKs.

> ZipFileAttributes.java
> - it seems a little odd that you've added "setPermissions()" to
> ZipFileAttributes. All the attribute classes (i.e.
> BasicFileAttributes, PosixFileAttributes) have no setters. Isn't it
> possible to implement "ZipFileAttributeView.setPermissions()" by
> calling "path.setPermissions()" (as this is done in
> "ZipFileAttributeView.setTimes()") and handle everything in
> ZipFileSyste (as this is done with "setTimes()").

Good catch & thanks for providing the better implementation.


I think this version is quite final now and thoroughly tested. I hope to get 
some valid reviews soon...

Thanks
Christoph

RE: RFR - CSR: 8213082: (zipfs) Add support for POSIX file permissions (was: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions)

[Langer, Christoph]

Hi Alan,

> Adding support for POSIX file permissions to the zip APIs is problematic
> as we've been discussing here. There are security concerns and also
> concerns that how it interacts with JAR files and signed JAR in
> particular. I don't disagree that we can come to agreement on zipfs
> supporting a solution but I think we need to get the bigger picture on
> where this is going first. If the piece to change the java.util.zip APIs
> is dropped then it would make these discussions a lot simpler as it
> removes most of the security issues from the table.

Yes, please consider changes to java.util.zip APIs as dropped. At least for the 
moment. I'm not saying I won't ever get back to that topic but maybe an 
enhancement of jdk.zipfs is already sufficient to provide the required Posix 
permission support for the Java platform.

Best regards
Christoph

RFR - CSR: 8213082: (zipfs) Add support for POSIX file permissions (was: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions)

[Langer, Christoph]

Hi folks,

getting back to the topic of adding POSIX file permission support to 
jdk.zipfs... I think as we are now in the early stages of JDK13, it's a good 
point in time to get some (hopefully final) activity on that one.

In the last review discussions you were asking me to provide some write-up of 
the proposal.
Therefore I updated the CSR. It should now be a valid document for discussing 
the whole proposal, comprising the problem to solve, the proposed solution and 
its specification as well as addressing some concerns.

And to get it clear: This item is only about jdk.zipfs. It is really 
independent of potential enhancements for java.util.zip or the jartool. So, I 
gently ask you to review the CSR. 

As for the implementation: I've worked on it together with Volker and will post 
an update soon.

Thanks and Best regards
Christoph

> -Original Message-
> From: Chris Hegarty 
> Sent: Montag, 5. November 2018 17:19
> To: Langer, Christoph ; core-libs-dev  d...@openjdk.java.net>; security-dev@openjdk.java.net; Xueming Shen
> 
> Cc: nio-dev 
> Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions
> 
> 
> On 05/11/18 15:59, Alan Bateman wrote:
> > On 05/11/2018 13:05, Langer, Christoph wrote:
> >>
> >>...
> >>
> > I think you'll need to do a write-up of the overall proposal so that
> > folks can jump in and point out the implications. It's not easy to do
> > this in a code review of a small piece of the solution.
> 
> Right, that's the main motivation for my previous questions. It is good
> to get a *complete* view of what is intended before reviewing the code.
> ( Sorry, if I've missed some of the previous context ).
> 
> -Chris.

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Chris

> The reason I asked about the CSR scope clarification is that it was
> unclear to me what the ultimate intentions are, given that the previous
> mails ( linked to from the CSR ) did have Java SE API changes ( in the
> java.util.zip package ).
> 
> Are you now happy to reduce the scope of the proposal to be confined to
> the "jar" filesystem provider, or is this more of an initial step
> towards ultimately adding new Java SE APIs, to zip, to access these
> permissions ( as was in previous emails )? As well as possibly updating
> the tools to add such?

The latter thing is true. I reduced the scope of my initial proposal to the 
"jar" filesystem provider here with the intention to get in this piece more 
quickly, if not at all. Maybe changes to java.util.zip and tools aren't 
feasible - we'll see.

Furthermore, assuming changes can be done with JDK12, I consider the jdk.zipfs 
piece "backportable", at least for our OpenJDK build SapMachine 
(https://sapmachine.io). So having it separate would definitely ease the 
backporting into SapMachine 11 - which is the target for a customer of ours.

Best
Christoph

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Alan, all,

I'd welcome a discussion, for sure. Unfortunately there hasn't been so much 
participation in this yet. I think this is an item where it's hard to have a 
clear opinion and where it's difficult to oversee all implications it might 
have.

Who'd be willing to have a look from security perspective?

Thanks & Best regards
Christoph

From: Alan Bateman 
Sent: Montag, 5. November 2018 11:23
To: Langer, Christoph ; core-libs-dev 
; security-dev@openjdk.java.net; Xueming Shen 

Cc: Volker Simonis ; nio-dev 

Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions

On 05/11/2018 07:32, Langer, Christoph wrote:

Hi,

Ping.

May I get reviews/substantial feedback on this zipfs enhancement?

It might be bit early to be asking for a code review on just one piece of this. 
I think the first step on this feature has to be to put all the issues on the 
table. There are several discussion points around ZIP vs. JAR, the impact on 
signed JARs, carrying permissions without a file owner, and the impact on 
tools. I also think that there will need to be discussion on security-dev as 
some of these issues around this feature have security concerns.

-Alan

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi Chris,

yes, there's no impact on Java SE with this item. No API is changed. I've set 
the scope to JDK, as it affects the features that are available with the "jar" 
filesystem provider from module jdk.zipfs.

Best regards
Christoph

> -Original Message-
> From: Chris Hegarty 
> Sent: Montag, 5. November 2018 13:20
> To: Langer, Christoph ; core-libs-dev  d...@openjdk.java.net>; security-dev@openjdk.java.net
> Cc: nio-dev 
> Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions
> 
> Hi Christoph,
> 
> On 05/11/18 07:32, Langer, Christoph wrote:
> > ..
> >
> > CSR: https://bugs.openjdk.java.net/browse/JDK-8213082
> 
> Can you please add a `Scope` value to the CSR?
> 
> I can't quite tell, but I assume it should be `JDK` or
> `Implementation`, right? I want to clarify that there is
> no impact on Java SE.
> 
> -Chris.

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions

[Langer, Christoph]

Hi,

Ping.

May I get reviews/substantial feedback on this zipfs enhancement?

Bug: https://bugs.openjdk.java.net/browse/JDK-8213031
CSR: https://bugs.openjdk.java.net/browse/JDK-8213082
Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.2/

Thanks
Christoph

From: Langer, Christoph
Sent: Montag, 29. Oktober 2018 15:55
To: 'Alan Bateman' ; core-libs-dev 
; security-dev@openjdk.java.net; Xueming Shen 

Cc: Volker Simonis ; nio-dev 

Subject: RE: RFR 8213031: (zipfs) Add support for POSIX file permissions (was: 
Enhance jdk.nio.zipfs to support Posix File Permissions)

Hi Alan, security guys,

I've proposed a CSR for this change now: 
https://bugs.openjdk.java.net/browse/JDK-8213082.

I also updated the webrev, simplifying jdk.nio.zipfs.ZipUtils.permsFromFlags 
and eliminating the WeakHashMap: 
http://cr.openjdk.java.net/~clanger/webrevs/8213031.2/

Since I've decoupled the changes to java.util.zip and jartool from those in 
jdk.zipfs, we're discussing only jdk.zipfs here.

The implication of my change is that when working with files backed by the nio 
FileSystemProvider (java.nio.file.spi.FileSystemProvider) named "jar", which is 
the alias for zipfs, the files will support attributes of type 
java.nio.file.attribute.PosixFilePermissions ("posix:permissions").

It basically means that some methods of java.nio.file.Files that would return 
null or UnsupportedOperationException before would find an implementation now.

Examples:
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#getPosixFilePermissions(java.nio.file.Path,java.nio.file.LinkOption...)
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#setPosixFilePermissions(java.nio.file.Path,java.util.Set)
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#readAttributes(java.nio.file.Path,java.lang.Class,java.nio.file.LinkOption...)

  *   With class 
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/attribute/PosixFileAttributes.html
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#getFileAttributeView(java.nio.file.Path,java.lang.Class,java.nio.file.LinkOption...)

  *   With class 
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/attribute/PosixFileAttributeView.html

Thanks in advance for reviewing.

Best regards
Christoph


From: Alan Bateman mailto:alan.bate...@oracle.com>>
Sent: Montag, 29. Oktober 2018 13:06
To: Langer, Christoph 
mailto:christoph.lan...@sap.com>>; core-libs-dev 
mailto:core-libs-...@openjdk.java.net>>; 
security-dev@openjdk.java.net<mailto:security-dev@openjdk.java.net>; Xueming 
Shen mailto:xueming.s...@oracle.com>>
Cc: Volker Simonis mailto:volker.simo...@gmail.com>>; 
Andrew Luo 
mailto:andrewluotechnolog...@outlook.com>>; 
nio-dev mailto:nio-...@openjdk.java.net>>
Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions (was: 
Enhance jdk.nio.zipfs to support Posix File Permissions)

On 29/10/2018 09:26, Langer, Christoph wrote:
:

As per request from Alan, I'm adding security-dev to get a review from security 
perspective.

For security-dev then I think it would be better to write-up a summary of the 
overall proposal and the implications for applications/libraries that use the 
APIs and the jar tool. The security discussion points all relate to capture and 
propagation of file permissions.

-Alan

RE: RFR 8213031: (zipfs) Add support for POSIX file permissions (was: Enhance jdk.nio.zipfs to support Posix File Permissions)

[Langer, Christoph]

Hi Alan, security guys,

I've proposed a CSR for this change now: 
https://bugs.openjdk.java.net/browse/JDK-8213082.

I also updated the webrev, simplifying jdk.nio.zipfs.ZipUtils.permsFromFlags 
and eliminating the WeakHashMap: 
http://cr.openjdk.java.net/~clanger/webrevs/8213031.2/

Since I've decoupled the changes to java.util.zip and jartool from those in 
jdk.zipfs, we're discussing only jdk.zipfs here.

The implication of my change is that when working with files backed by the nio 
FileSystemProvider (java.nio.file.spi.FileSystemProvider) named "jar", which is 
the alias for zipfs, the files will support attributes of type 
java.nio.file.attribute.PosixFilePermissions ("posix:permissions").

It basically means that some methods of java.nio.file.Files that would return 
null or UnsupportedOperationException before would find an implementation now.

Examples:
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#getPosixFilePermissions(java.nio.file.Path,java.nio.file.LinkOption...)
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#setPosixFilePermissions(java.nio.file.Path,java.util.Set)
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#readAttributes(java.nio.file.Path,java.lang.Class,java.nio.file.LinkOption...)

  *   With class 
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/attribute/PosixFileAttributes.html
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/Files.html#getFileAttributeView(java.nio.file.Path,java.lang.Class,java.nio.file.LinkOption...)

  *   With class 
https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/nio/file/attribute/PosixFileAttributeView.html

Thanks in advance for reviewing.

Best regards
Christoph


From: Alan Bateman 
Sent: Montag, 29. Oktober 2018 13:06
To: Langer, Christoph ; core-libs-dev 
; security-dev@openjdk.java.net; Xueming Shen 

Cc: Volker Simonis ; Andrew Luo 
; nio-dev 
Subject: Re: RFR 8213031: (zipfs) Add support for POSIX file permissions (was: 
Enhance jdk.nio.zipfs to support Posix File Permissions)

On 29/10/2018 09:26, Langer, Christoph wrote:

:

As per request from Alan, I'm adding security-dev to get a review from security 
perspective.

For security-dev then I think it would be better to write-up a summary of the 
overall proposal and the implications for applications/libraries that use the 
APIs and the jar tool. The security discussion points all relate to capture and 
propagation of file permissions.

-Alan

RFR 8213031: (zipfs) Add support for POSIX file permissions (was: Enhance jdk.nio.zipfs to support Posix File Permissions)

[Langer, Christoph]

Hi,

here's an update of my webrev: 
http://cr.openjdk.java.net/~clanger/webrevs/8213031.1/

I added synchronization to the updating of permCache in ZipUtils.java to avoid 
concurrent modifications.

As per request from Alan, I'm adding security-dev to get a review from security 
perspective.

Thanks
Christoph

From: Langer, Christoph
Sent: Freitag, 26. Oktober 2018 17:13
To: core-libs-dev ; nio-dev 
; 'Xueming Shen' 
Cc: Schmelter, Ralf ; 'Volker Simonis' 

Subject: RFR 8213031: Enhance jdk.nio.zipfs to support Posix File Permissions

Hi,

please review this enhancement of jdk.nio.zipfs to support Posix file 
permissions.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8213031.0/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213031

I had already posted this change as part of a larger fix for "6194856: Zip 
Files lose ALL ownership and permissions of the files" [1]. With the original 
proposal I was also addressing the java.util.zip API and the jar tool. However, 
I've decided to split this endeavor into 2 parts. While I still want to follow 
up on java.util.zip and jar, I'd like to bring the enhancement to jdk.zipfs in 
first. Both places don't have dependencies to each other and since zipfs does 
not change an external API, I guess the bar here is lower. Maybe it is even a 
candidate for down-porting to jdk11u in the future.

After my fix, zipfs will support the PosixFileAttributeView. Posix file 
attributes can't be fully supported since owner and group are not handled in 
zip files. So methods supposed to get these attributes will return an 
UnsupportedOperationException. But the posix permissions will now be correctly 
handled, that is stored into and read from the zip file.

@Sherman:
Following suggestions from your review, I removed the test with the binary zip 
file. I initially thought it is a good idea to test on a well-known file. 
However, testWriteAndReadArchiveWithPosixPerms tests both, writing a zip file 
and reading it again so I guess test coverage is quite complete here.
Furthermore, I made some public declarations in ZipUtils package private which 
should suffice.
I also tried to address your performance concerns with permsToFlags and 
permsFromFlags. In permsToFlags I will now simply iterate to the set of 
permissions and add the flags to the return value. It's probably more 
performant than the streaming approach and doesn't look much worse in the code. 
In permsFromFlags I added a cache of permission sets which should avoid 
constant calls to the streaming. However, as the return value needs to be 
mutable, I have to clone the cached permissions. Maybe it would have the same 
or even better performance if we iteratively fill a new set of permissions each 
time permsToFlags gets called. What do you think?

Do we need a CSR for this patch?

Thanks & Best regards
Christoph

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-October/055971.html

RE: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

[Langer, Christoph]

Hi Matthias,

I generally support this enhancement of IOExceptions to include path 
information.

I also think that we should protect this with the java.security property 
"jdk.includeInExceptions" and agree that "path" would be a good choice since it 
is generic enough to be used in other places where Exceptions are thrown that 
might include path names. As for some comment from security-folks: Sean, what 
is your take on this?

As for the implementation: We should definitely get rid of the upcall into Java 
from JNU_ThrowIOExceptionWithLastErrorAndPath and build the exception message 
in that function only, just using the " jstring path" argument.

The reason for having the current implementation of ExceptionUtils. 
createExceptionTextWithPath, as you suggested in your webrev was that we were 
chasing a very specific customer issue where cwd and user.dir diverged (e.g. 
because of some 3rd party native library which changed the cwd). And this code 
is somehow still around.
So, user.dir should definitely not be part of the exception message. And 
whether the current cwd should be contained should be reviewed in detail again. 
Maybe at some places it is interesting but on other places it isn't at all.

Best regards
Christoph

> -Original Message-
> From: core-libs-dev  On Behalf
> Of Baesken, Matthias
> Sent: Freitag, 12. Oktober 2018 15:19
> To: Alan Bateman ; security-
> d...@openjdk.java.net; core-libs-...@openjdk.java.net
> Subject: [CAUTION] RE: RFR: 8211752:
> JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions
> with path causing the issue
> 
> Hi Alan, Goetz,
> 
> >There are several issues with this proposal. The security concern is the main
> one and I think we should wait for comment from security-dev.
>   .
> 
> >If this is required, I would choose a more generic tag
> 
> >so it can be reused in other places.
> 
> >I would just use "path".
> 
> >
> 
> >Best regards,
> 
> >  Goetz.
> 
> Thanks for  the comments .   Sure,  I can use path  for the category naming  
> as
> well, but would be good to hear the  opinions of  the security-dev guys on
> this .
> 
> > The second concern is that the patch is incomplete and inconsistent in that
> it changes the exception throw by two methods,
> >it ignores other file I/O methods that throw exceptions.
> 
> We  have these extended exceptions for quite some time in our JVM;   we
> decided to   enhance exceptions where we have a path already "at hand" ,
>   so it was easy to extend the exceptions .
> In case you think this can be done with some others , that's fine with me -  
> do
> you have some concrete examples ?
> 
> A lot of IO exceptions currently thrown that use
> JNU_ThrowIOExceptionWithLastError  do not have a path (at least not where
> the exception is thrown).
> 
> 
> Best regards ,
> Matthias
> 
> 
> 
> From: Alan Bateman 
> Sent: Freitag, 12. Oktober 2018 11:18
> To: Baesken, Matthias ; security-
> d...@openjdk.java.net; core-libs-...@openjdk.java.net
> Subject: Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath -
> enhance some IOExceptions with path causing the issue
> 
> On 12/10/2018 09:12, Baesken, Matthias wrote:
> 
> Ping ...  any reviews / comments ?
> 
> Should I add  a category  , for example"ioExceptionsWithPath" to the
> java.security - file  to control  the enabling/disabling of the  enhanced
> exception ?
> 
>   java.security
> 
> #
> # Enhanced exception message information
> ...
> #jdk.includeInExceptions=hostInfo,jar,ioExceptionsWithPath
> 
> There are several issues with this proposal. The security concern is the main
> one and I think we should wait for comment from security-dev. The second
> concern is that the patch is incomplete and inconsistent in that it changes 
> the
> exception throw by two methods, it ignores other file I/O methods that
> throw exceptions. Finally there is the concerns with the patch itself that 
> both
> Roger and I commented on.
> 
> -Alan

RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Langer, Christoph]

Looks good to me.

> -Original Message-
> From: Baesken, Matthias
> Sent: Mittwoch, 12. September 2018 11:27
> To: Sean Mullan ; Langer, Christoph
> ; Weijun Wang 
> Cc: security-dev@openjdk.java.net; core-libs-...@openjdk.java.net
> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
> 
> Hello I changed it to jar , also added  some minor adjustments suggested by
> Christoph .
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.12/
> 
> 
> Regards, Matthias
> 
> 
> > -Original Message-
> > From: Sean Mullan 
> > Sent: Dienstag, 11. September 2018 20:44
> > To: Langer, Christoph ; Baesken, Matthias
> > ; Weijun Wang 
> > Cc: security-dev@openjdk.java.net; core-libs-...@openjdk.java.net
> > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> > parsing of jar archives
> >
> > On 9/11/18 8:14 AM, Langer, Christoph wrote:
> > > Hi,
> > >
> > > first of all, I suggest to use "jarDetails" instead of "jarPath" as 
> > > category
> > name. Because with this contribution we add the notion of jar file plus line
> of
> > manifest to Exceptions occurring when parsing jar manifests. And if there
> > were further Exception details to be added in the area of jar files, they
> could
> > go under a category "jarDetails" as well. Would you agree? Then, in
> > Manifest.java, the field "jarPathInExceptionText" could be renamed to
> > "detailedExceptions".
> >
> > Or just "jar" would be my preference. I don't like "jarPath" as that
> > sounds too much like a file pathname to me, which we have now removed.
> >
> > --Sean

RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Langer, Christoph]

Hi,

first of all, I suggest to use "jarDetails" instead of "jarPath" as category 
name. Because with this contribution we add the notion of jar file plus line of 
manifest to Exceptions occurring when parsing jar manifests. And if there were 
further Exception details to be added in the area of jar files, they could go 
under a category "jarDetails" as well. Would you agree? Then, in Manifest.java, 
the field "jarPathInExceptionText" could be renamed to "detailedExceptions".

As for the code, I have the following remarks:

src/java.base/share/classes/java/util/jar/Manifest.java:
You could further clean up the ordering of includes by sorting them 
alphabetically and add a blank line between lines 34/35.
Line 52: I suggest an indentation of 8 chars
Please use jarFilename as final. You'll have to initialize jarFilename in each 
constructor then or initialize it to null in the default constructor and call 
this constructor from all the other ctors except for the one taking the 
jarFilename as param.

src/java.base/share/classes/sun/net/util/SocketExceptions.java
please add an empty line between 32 and 33
Line 39: I suggest an indentation of 8 chars

src/java.base/share/classes/sun/security/util/SecurityProperties.java
Line 35: change comment opener to /** (from /*), then it's a real Javadoc
Furthermore I suggest to change the wording to "Returns the value of the 
security property propName, which can be overridden by a system property of the 
same name"

Best regards
Christoph

> -Original Message-
> From: Baesken, Matthias
> Sent: Dienstag, 11. September 2018 13:29
> To: Weijun Wang 
> Cc: Langer, Christoph ; Sean Mullan
> ; security-dev@openjdk.java.net; core-libs-
> d...@openjdk.java.net
> Subject: RE: [RFR] 8205525 : Improve exception messages during manifest
> parsing of jar archives
> 
> > I don't have a strong opinion on making Manifest.jarFilename final
> 
> Hi, just making it final  leads to compile errors anyway.
> 
> Best regards, Matthias
> 
> 
> > -----Original Message-
> > From: Weijun Wang 
> > Sent: Dienstag, 11. September 2018 13:04
> > To: Baesken, Matthias 
> > Cc: Langer, Christoph ; Sean Mullan
> > ; security-dev@openjdk.java.net; core-libs-
> > d...@openjdk.java.net
> > Subject: Re: [RFR] 8205525 : Improve exception messages during manifest
> > parsing of jar archives
> >
> > Attributes.java:
> >
> > - Line 377: Too long, add a break.
> >
> > Otherwise fine.
> >
> > I don't have a strong opinion on making Manifest.jarFilename final or a
> > different property name.
> >
> > Thanks
> > Max
> >
> > > On Sep 11, 2018, at 5:07 PM, Baesken, Matthias
> >  wrote:
> > >
> > > Hello, please check the new webrev  :
> > >
> > > http://cr.openjdk.java.net/~mbaesken/webrevs/8205525.10/
> > >
> > > I kept the jarPath   category name .
> > >
> > > Best regards, Matthias
> > >
> > >
> > >> -Original Message-
> > >> From: Langer, Christoph
> > >> Sent: Montag, 10. September 2018 21:30
> > >> To: Weijun Wang ; Baesken, Matthias
> > >> 
> > >> Cc: Sean Mullan ; security-
> > >> d...@openjdk.java.net; core-libs-...@openjdk.java.net
> > >> Subject: RE: [RFR] 8205525 : Improve exception messages during
> manifest
> > >> parsing of jar archives
> > >>
> > >> Hi,
> > >>
> > >>>> do you think we need property
> jdk.includeInExceptions=jar
> > at
> > >>> all, if we don't resolve the absolute path?
> > >>>
> > >>> I think so. File path is still sensitive.
> > >>>
> > >>> In fact, I tend to believe people usually use absolute paths for JAR 
> > >>> files
> > (or
> > >>> maybe made absolute by using a file:// URL somewhere inside JDK).
> Do
> > >> you
> > >>> really see relative jar paths while testing this code change?
> > >>
> > >> I agree.
> > >>
> > >>>> small remark to the code:
> > >>>> src/java.base/share/classes/sun/security/util/SecurityProperties.java
> > >>>> 36 public static String privilegeGetOverridable(String propName) {
> > >>>>
> > >>>> Should that method really be public? At the moment it doesn't seem
> to
> > >> be
> > >>> used outside of SecurityProperties.
> > >>>
> > >>> I like it to be public. There are quite some other such system/security
> > >>> properties (Ex: jdk.serialFilter) that can make use of this method.
> > >>
> > >> Ok, maybe it should be named "priviledgedGetOverridable" then.
> > >>
> > >> @Matthias:
> > >> Further small cleanups, as you touch the files:
> > >> src/java.base/share/classes/java/util/jar/Manifest.java: line 35: you can
> > >> remove "import java.util.Iterator;"
> > >>
> > >> src/java.base/share/classes/sun/net/util/SocketExceptions.java:
> > >> line 41: indentation is ridiculously long, I think it should be 8 chars
> > >>
> > >> src/java.base/share/classes/sun/security/util/SecurityProperties.java:
> > >> Indentation of lines 38-45 is too deep, should be 12.
> > >> The 2 methods could use a brief Javadoc.
> > >>
> > >> Best regards
> > >> Christoph
> > >
> 

RE: [RFR] 8205525 : Improve exception messages during manifest parsing of jar archives

[Langer, Christoph]

Hi,

> > do you think we need property jdk.includeInExceptions=jar at
> all, if we don't resolve the absolute path?
> 
> I think so. File path is still sensitive.
> 
> In fact, I tend to believe people usually use absolute paths for JAR files (or
> maybe made absolute by using a file:// URL somewhere inside JDK). Do you
> really see relative jar paths while testing this code change?

I agree.

> > small remark to the code:
> > src/java.base/share/classes/sun/security/util/SecurityProperties.java
> > 36 public static String privilegeGetOverridable(String propName) {
> >
> > Should that method really be public? At the moment it doesn't seem to be
> used outside of SecurityProperties.
> 
> I like it to be public. There are quite some other such system/security
> properties (Ex: jdk.serialFilter) that can make use of this method.

Ok, maybe it should be named "priviledgedGetOverridable" then.

@Matthias:
Further small cleanups, as you touch the files:
src/java.base/share/classes/java/util/jar/Manifest.java: line 35: you can 
remove "import java.util.Iterator;"

src/java.base/share/classes/sun/net/util/SocketExceptions.java:
line 41: indentation is ridiculously long, I think it should be 8 chars

src/java.base/share/classes/sun/security/util/SecurityProperties.java:
Indentation of lines 38-45 is too deep, should be 12.
The 2 methods could use a brief Javadoc.

Best regards
Christoph