Re: Review Request for 9000142: PlatformPCSC.java loading unversioned native shared library

2013-04-24 Thread Severin Gehwolf
On Fri, 2013-03-01 at 11:30 +0100, Severin Gehwolf wrote:
> Hi,
> 
> The bug for this review request is at:
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=9000142
> 
> In PlatformPCSC.java unversioned native libraries are loaded by default
> if no system property is specified. This could lead to a JVM crash if
> the API of the native library changes, but the Java code still relies on
> old API. The fix is to load versioned shared libraries instead.
> 
> See also:
> https://bugzilla.redhat.com/show_bug.cgi?id=910107
> 
> The webrev is here:
> http://jerboaa.fedorapeople.org/bugs/openjdk/9000142/webrev.0/

Any thoughts at all?

Thanks,
Severin





Re: Review Request for 9000142: PlatformPCSC.java loading unversioned native shared library

2013-04-25 Thread Severin Gehwolf
On Wed, 2013-04-24 at 13:05 +0200, Florian Weimer wrote:
> On 03/01/2013 11:30 AM, Severin Gehwolf wrote:
> > Hi,
> >
> > The bug for this review request is at:
> > http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=9000142
> >
> > In PlatformPCSC.java unversioned native libraries are loaded by default
> > if no system property is specified. This could lead to a JVM crash if
> > the API of the native library changes, but the Java code still relies on
> > old API. The fix is to load versioned shared libraries instead.
> 
> Hmm.  Why doesn't the "j2pcsc" library link against the right version of 
> libpcsclite.so?

Good question. Anyone who was involved with creating the PlatformPCSC
API still around who would be able to answer this question?

Cheers,
Severin





Re: Review Request for 9000142: PlatformPCSC.java loading unversioned native shared library

2013-04-25 Thread Severin Gehwolf
Hi Valerie,

Thanks for the review!

On Wed, 2013-04-24 at 15:11 -0700, Valerie (Yu-Ching) Peng wrote:
> Won't this change break systems which don't have libpcsclite.so.1?

Yes. However, currently it breaks systems which don't have
libpcsclite.so. [1] would be an example. There is a system property,
sun.security.smartcardio.library, which could be used to specify the
library path. *If* people really want to use libpcsclite.so (no matter
the version), they could use that property.

What I think is bad, is that it currently tries to use unversioned
libpcsclite.so by default. I mean even when no property has been set.
While the static initializer might work in that case it could
potentially fail later on when it's used and libpcsclite.so is actually
a newer version the Java API does not know how to handle.

> Changes like this need to be thought through. What happens when 
> libpcsclite.so.2 comes out?

Exactly. Right now there is the potential to accept a libpcsclite.so.2
if that library providing libpcsclite.so.2 also provides a symlink to
libpcsclite.so. My proposed change to PlatformPCSC would make that fail
with an IOException right in the static initializer. Failing early seems
to be a better approach in this case.

> As for API changes, shouldn't there be some compatibility requirement on 
> APIs as libpcsclite.so evolves?

Maybe. It looks to me that PlatformPCSC seems to rely on that. What if
API compatibility has been assumed, but does not hold once
libpcsclite.so.2 comes out?

Cheers,
Severin

[1] https://bugzilla.redhat.com/show_bug.cgi?id=910107

> On 04/24/13 04:05, Florian Weimer wrote:
> > On 03/01/2013 11:30 AM, Severin Gehwolf wrote:
> >> Hi,
> >>
> >> The bug for this review request is at:
> >> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=9000142
> >>
> >> In PlatformPCSC.java unversioned native libraries are loaded by default
> >> if no system property is specified. This could lead to a JVM crash if
> >> the API of the native library changes, but the Java code still relies on
> >> old API. The fix is to load versioned shared libraries instead.
> >
> > Hmm.  Why doesn't the "j2pcsc" library link against the right version 
> > of libpcsclite.so?
> >
> 





[PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS

2012-11-06 Thread Severin Gehwolf
Hi,

In Config.java, line 1234 in method getKDCFromDNS(String realm) there is
a loop which discards earlier values of KDCs returned rather than
concatenating them. This results in a behaviour where only one KDC in a
seemingly random fashion is returned. In fact, the KDC returned depends
on the order which KrbServiceLocator.getKerberosService(realm, "_udp")
returns the servers. The correct behaviour should be to return a String
containing ALL KDCs available via DNS (separated by spaces).

The webrev is here:
http://jerboaa.fedorapeople.org/bugs/openjdk/2376501/webrev/

Comments and suggestions very welcome!

Thanks,
Severin



Re: [PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS

2012-11-08 Thread Severin Gehwolf
Hi Max,

Thanks for the review!

On Wed, 2012-11-07 at 08:52 +0800, Weijun Wang wrote:
> The fix looks fine. There is one place it might get enhanced:
> 
>  if (value.charAt(j) == ':') {
>  kdcs = (value.substring(0, j)).trim();
>  }
> 
> So this changes a.com:88 to a.com. If the port is really 88, it's OK. 
> Otherwise, info gets lost. Maybe we can keep the whole string.

I've removed the entire loop which strips the port from the returned
record. Updated webrev is here:

http://jerboaa.fedorapeople.org/bugs/openjdk/2376501/webrev.1/

> BTW, are you OK with contributing the fix into OpenJDK main repo?

Yes, of course :) Just let me know what's to be done to get it pushed.

Cheers,
Severin

> On 11/06/2012 11:08 PM, Severin Gehwolf wrote:
> > Hi,
> >
> > In Config.java, line 1234 in method getKDCFromDNS(String realm) there is
> > a loop which discards earlier values of KDCs returned rather than
> > concatenating them. This results in a behaviour where only one KDC in a
> > seemingly random fashion is returned. In fact, the KDC returned depends
> > on the order which KrbServiceLocator.getKerberosService(realm, "_udp")
> > returns the servers. The correct behaviour should be to return a String
> > containing ALL KDCs available via DNS (separated by spaces).
> >
> > The webrev is here:
> > http://jerboaa.fedorapeople.org/bugs/openjdk/2376501/webrev/
> >
> > Comments and suggestions very welcome!
> >
> > Thanks,
> > Severin
> >





Re: 8002344 code review request: (was Re: [PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS)

2012-11-13 Thread Severin Gehwolf
Hi Max,

On Fri, 2012-11-09 at 08:38 +0800, Weijun Wang wrote:
> Hi Severin
> 
> I've created an OpenJDK bug and created a new webrev:
> 
> http://cr.openjdk.java.net/~weijun/8002344/webrev.00/
> 
> The Config.java change is identical to yours, and I added a small tweak 
> in KrbServiceLocator, and a quite ugly regression test.

Cool. Thanks! FWIW, I've created a bug with ID 2376501 for this (even
before I submitted a patch). Unfortunately, this bug is not accessible
to me. All it let me do was creating it :)

Cheers,
Severin



Re: 8002344 code review request: (was Re: [PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS)

2012-11-13 Thread Severin Gehwolf
On Tue, 2012-11-13 at 19:00 +0800, Weijun Wang wrote:
> Where did you create this bug? Every new OpenJDK bug should starts with 
> 7 or 8. I've been wondering what 2376501 means and thought it's 
> something RedHat internal.

:) All I know is that it's nothing internal to Red Hat and I've created
it via bugs.sun.com and clicking on "submit new bug"[1]. The bug ID came
from the email I've received after it was submitted. Anywho, just wanted
to let you know that there's another bug open *somewhere* within Oracle
for the same issue and I can't access it.

Cheers,
Severin

[1] Forwarded this bug email to you privately.

> On 11/13/2012 06:43 PM, Severin Gehwolf wrote:
> > Hi Max,
> >
> > On Fri, 2012-11-09 at 08:38 +0800, Weijun Wang wrote:
> >> Hi Severin
> >>
> >> I've created an OpenJDK bug and created a new webrev:
> >>
> >>  http://cr.openjdk.java.net/~weijun/8002344/webrev.00/
> >>
> >> The Config.java change is identical to yours, and I added a small tweak
> >> in KrbServiceLocator, and a quite ugly regression test.
> >
> > Cool. Thanks! FWIW, I've created a bug with ID 2376501 for this (even
> > before I submitted a patch). Unfortunately, this bug is not accessible
> > to me. All it let me do was creating it :)
> >
> > Cheers,
> > Severin
> >





Re: 8002344 code review request: (was Re: [PATCH] for bug 2376501: Krb5LoginModule config class does not return proper KDC list from DNS)

2012-11-19 Thread Severin Gehwolf
Max,

Can we get the fix[1] backported to JDK7? Patch applies cleanly on top
of JDK 7 sources for me.

Thanks,
Severin

[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f740a9ac6eb6

On Wed, 2012-11-14 at 17:04 +0400, Dmitry Samersoff wrote:
> Weijun,
> 
> On 2012-11-14 12:08, Weijun Wang wrote:
> >> On 2012-11-14 04:36, Weijun Wang wrote:
> >>> Yes, but with line 288, hostport will be "host:1". Isn't that expected?
> >>
> >> I'm OK with ll. 288 but against ll.285. I'm against comparing integers
> >> as strings - port could (imaginary) be specified as 0088 and comparison
> >> fails.
> > 
> > I see.
> > 
> > Actually, in KdcComm.java when the string "host:port" is really been
> > used, Integer.parseInt(port) is called and when an exception is thrown
> > host:88 is tried, although this might not be the correct guess.
> > 
> > Are you ok with this "delayed" check?
> 
> Yes.
> 
> > 
> > As for the possibility of 0088, I'll remove that equals check and stick
> > with
> > 
> >   hostport = tokenizer.nextToken() + ":" + port;
> 
> I'm OK with it.
> 
> -Dmitry
> 
> 
> > 
> > You can see my DNS.java test already prepared for this by comparing to
> > both a.com.:88 and a.com. :)
> > 
> > Thanks
> > Max
> > 
> >>
> >>
> >>>> Customer allowed to use service name here instead of numeric port
> >>>> number
> >>>> if my memory is not bogus.
> >>>
> >>> rfc2782 [1] says it can be only a number:
> >>
> >> Thank you for clarification.
> >> -Dmitry
> >>
> >>>
> >>> Port
> >>>  The port on this target host of this service.  The range is 0-
> >>>  65535.  This is a 16 bit unsigned integer in network byte
> >>> order.
> >>>  This is often as specified in Assigned Numbers but need not be.
> >>>
> >>> If it's really a service name, I might have to ignore it at the moment
> >>> because I don't know an API to perform getportbyname().
> >>>
> >>>>
> >>>> 2.
> >>>>> shadow the real one by prepending it to the bootclasspath.
> >>>> jtreg allows you to change boot classpath in othervm mode
> >>>>
> >>>> e.g. :
> >>>>
> >>>> @run main/othervm -Xbootclasspath/a:../classes/serviceability
> >>>> -XX:+UnlockDiagnosticVMOptions ... ParserTest
> >>>
> >>> Good suggestion, but the class is built into test.classes and test run
> >>> in scratch. I cannot find a way to get the relative path to the class. I
> >>> reply on "javac -d ." to output the class into scratch.
> >>>
> >>> Thanks
> >>> Max
> >>>
> >>> [1] http://tools.ietf.org/html/rfc2782
> >>>
> >>>>
> >>>>
> >>>> -Dmitry
> >>>>
> >>>> On 2012-11-13 16:07, Weijun Wang wrote:
> >>>>>
> >>>>>
> >>>>> On 11/13/2012 07:44 PM, Dmitry Samersoff wrote:
> >>>>>> Weijun,
> >>>>>>
> >>>>>> Config.java:1162
> >>>>>> This code is unclear to me.
> >>>>>> if srvs[i] could be "" this code could insert extra space in
> >>>>>> the middle of kdcs string.
> >>>>>
> >>>>> It should never be empty. KrbServiceLocator.java:281 makes sure it's a
> >>>>> valid DNS SRV record.
> >>>>>
> >>>>>>
> >>>>>> if srvc[i] couldn't be empty we can return null just
> >>>>>> after line 1160  if srvs.length == 0
> >>>>>
> >>>>> Yes, we can.
> >>>>>
> >>>>>>
> >>>>>> KrbServiceLocator.java:285
> >>>>>>
> >>>>>>Kerberos could be run on non standard port - rare case but
> >>>>>>AFAIK we don't limit it. So it's better to use parseInt here.
> >>>>>
> >>>>> That's line 288. Are you suggesting that port string can be
> >>>>> non-numeric
> >>>>> and need a check?
> >>>>>
> >>>>>>
&g

Review Request for 9000142: PlatformPCSC.java loading unversioned native shared library

2013-03-01 Thread Severin Gehwolf
Hi,

The bug for this review request is at:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=9000142

In PlatformPCSC.java unversioned native libraries are loaded by default
if no system property is specified. This could lead to a JVM crash if
the API of the native library changes, but the Java code still relies on
old API. The fix is to load versioned shared libraries instead.

See also:
https://bugzilla.redhat.com/show_bug.cgi?id=910107

The webrev is here:
http://jerboaa.fedorapeople.org/bugs/openjdk/9000142/webrev.0/

Thanks,
Severin



DSA default algorithm for keytool -genkeypair. Bad choice?

2018-10-10 Thread Severin Gehwolf
Hi,

What is the rationale of using DSA keys (2048 bit) as default for
genkeypair command?
http://hg.openjdk.java.net/jdk/jdk/file/c4a39588a075/src/java.base/share/classes/sun/security/tools/keytool/Main.java#l1120

It seems a bad choice given that DSA keys are disabled via Fedora's
crypto policy (not just OpenJDK, but other crypto providers too).

Here the explanation from Nikos Mavrogiannopoulos from a Fedora bug[1]
as to why that's a bad choice:

"""
DSA is not used by new security protocols any more (doesn't exist as a
negotiation option under TLS1.3), and was a very rarely used option
under previous protocols (TLS1.2 or earlier). In fact only DSA-1024 is
documented under these protocols. DSA-2048 may or may not work
depending on the implementation (and even worse may not interoperate).
"""

Could the default choice of keyalg for genkeypair be reconsidered? If
not, why not?

Thanks,
Severin

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1582253



Re: DSA default algorithm for keytool -genkeypair. Bad choice?

2018-10-10 Thread Severin Gehwolf
Hi Sean,

On Wed, 2018-10-10 at 07:59 -0400, Sean Mullan wrote:
> On 10/10/18 6:23 AM, Severin Gehwolf wrote:
> > Hi,
> > 
> > What is the rationale of using DSA keys (2048 bit) as default for
> > genkeypair command?
> > http://hg.openjdk.java.net/jdk/jdk/file/c4a39588a075/src/java.base/share/classes/sun/security/tools/keytool/Main.java#l1120
> 
> There is really no other reason other than DSA keys have been the 
> default keypairs generated by keytool for a long time, so there are some 
> compatibility issues we would have to think through before changing it 
> to another algorithm such as RSA. Weijun might have more insight into that.
> > It seems a bad choice given that DSA keys are disabled via Fedora's
> > crypto policy (not just OpenJDK, but other crypto providers too).
> 
> Actually, only DSA keys < 1024-bit are disabled by default in OpenJDK.

Thanks. I should have perhaps clarified. Not sure whether that was
clear. In Fedora a global crypto policy is in place. The policy affects
OpenSSL, GnuTLS, (patched) OpenJDK etc. It's that global policy which
disables DSA unconditionally.

> > Here the explanation from Nikos Mavrogiannopoulos from a Fedora bug[1]
> > as to why that's a bad choice:
> > 
> > """
> > DSA is not used by new security protocols any more (doesn't exist as a
> > negotiation option under TLS1.3), and was a very rarely used option
> > under previous protocols (TLS1.2 or earlier). In fact only DSA-1024 is
> > documented under these protocols. DSA-2048 may or may not work
> > depending on the implementation (and even worse may not interoperate).
> > """
> > 
> > Could the default choice of keyalg for genkeypair be reconsidered? 
> 
> Yes, I think it should be considered since DSA is rarely used anymore 
> and not supported by newer security protocols such as TLS 1.3. I have 
> filed: https://bugs.openjdk.java.net/browse/JDK-8212003

Great, thanks!

Cheers,
Severin

> --Sean
> 
> > If not, why not?
> > 
> > Thanks,
> > Severin
> > 
> > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1582253
> > 



Re: Problems with AES-GCM native acceleration

2018-11-14 Thread Severin Gehwolf
Dropping hotspot-dev and adding security-dev.

On Wed, 2018-11-14 at 14:39 +0200, Gidon Gershinsky wrote:
> Hi,
> 
> We are working on an encryption mechanism at the Apache Parquet -
> that will enable efficient analytics on encrypted data by frameworks
> such as Apache Spark.  
> https://github.com/apache/parquet-format/blob/encryption/Encryption.md
> https://www.slideshare.net/databricks/efficient-spark-analytics-on-encrypted-data-with-gidon-gershinsky
> 
> We came across an AES-related issue in the Java HostSpot engine that
> looks like a substantial problem for us in both Spark and Parquet
> workloads. The bug report had been accepted a while ago:
> https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8201633
> 
> The fix should hopefully be rather straightforward though.
> Could you help us with that? I have a couple of small samples
> reproducing the problem.
> 
> (If I'm writing to a wrong mailing list - I apologize, please point
> me in the right direction).
> 
> Cheers, Gidon.



Re: JDK-8215102 (Follow-up)

2019-01-18 Thread Severin Gehwolf
On Thu, 2019-01-17 at 10:00 -0700, Dennis Gesker wrote:
[...]
> Added the  -Djavax.net.debug=all option to my Wildfly startup and
> waited for the pool to close a connection to MySql at AWS.
> 
> TXT file attached.
> 
> javac 11.0.1
> mysql jdbc driver 8.0.13
> wildfly 15.0.1
> 
> --drg

Unfortunately the txt file got stripped by the mailing list. Would you
be able to upload it somewhere and post a link?

Thanks,
Severin



Re: JDK-8215102 (Follow-up)

2019-01-21 Thread Severin Gehwolf
Hi Dennis,

On Sat, 2019-01-19 at 12:08 -0700, Dennis Gesker wrote:
> Hi Severin:
> 
> A link to the txt file via Google Drive his here.

"Sorry, the file you have requested does not exist."

Could you please upload it somewhere less restricted? Maybe 
https://paste.fedoraproject.org/ or something similar?

I guess if you include me directly, a file attachment would work too...
It's the mailing lists which strip attachments.

> I appreciate you and Alan taking a look. Especially, information
> submitted from someone who is not a part of openjdk project.
> I do hope the debug info helps. Let me know anything else you need
> and I will do my best to provide it.

Sure. I'll be mostly doing the intermediary work: getting the info
added to the bug, though.

> And, should your team decide to open a new ticket or reopen this
> original ticket in the JIRA could you add me to the ticket?

You'd have to become OpenJDK author for this[1]. It's not terribly
difficult, but it's somewhat of an entry barrier I understand.

> BTW, (off topic), would your recommend submitting a contributor
> application to the openjdk project so bug reports can be submitted
> directly? 

If you intend to submit the occasional bug report and fix it's easier
for you long-term to attempt to become OpenJDK author (which requires
signing the OCA[2]).

> The dev group at my company is VERY small (and this message to your
> group at the project is from my personal email) but I'd be glad to
> submit bug reports as we come across them in our day to day use of
> Java.

If there are good reproducers for bugs this would be very welcome.
Thanks for investing some time in this!

Cheers,
Severin

[1] http://openjdk.java.net/bylaws#author
http://openjdk.java.net/projects/#project-author
[2] http://oss.oracle.com/oca.pdf

> Cordially,
> Dennis
> [email protected]
> 
> On Fri, Jan 18, 2019 at 10:07 AM Severin Gehwolf  > wrote:
> > On Thu, 2019-01-17 at 10:00 -0700, Dennis Gesker wrote:
> > [...]
> > > Added the  -Djavax.net.debug=all option to my Wildfly startup and
> > > waited for the pool to close a connection to MySql at AWS.
> > > 
> > > TXT file attached.
> > > 
> > > javac 11.0.1
> > > mysql jdbc driver 8.0.13
> > > wildfly 15.0.1
> > > 
> > > --drg
> > 
> > Unfortunately the txt file got stripped by the mailing list. Would
> > you
> > be able to upload it somewhere and post a link?
> > 
> > Thanks,
> > Severin
> > 
> 
> 



Re: JDK-8215102 (Follow-up)

2019-01-21 Thread Severin Gehwolf
On Mon, 2019-01-21 at 07:57 -0700, Dennis Gesker wrote:
> 
> Pasted:
> 
> https://paste.fedoraproject.org/paste/vEvp9RwN2rVvIKGiC0IvEQ

Is this the full trace? I don't see any exceptions happening in the
log. Am I missing something?

Thanks,
Severin

> On Mon, Jan 21, 2019 at 2:10 AM Severin Gehwolf 
> wrote:
> > Hi Dennis,
> > 
> > On Sat, 2019-01-19 at 12:08 -0700, Dennis Gesker wrote:
> > > Hi Severin:
> > > 
> > > A link to the txt file via Google Drive his here.
> > 
> > "Sorry, the file you have requested does not exist."
> > 
> > Could you please upload it somewhere less restricted? Maybe 
> > https://paste.fedoraproject.org/ or something similar?
> > 
> > I guess if you include me directly, a file attachment would work
> > too...
> > It's the mailing lists which strip attachments.
> > 
> > > I appreciate you and Alan taking a look. Especially, information
> > > submitted from someone who is not a part of openjdk project.
> > > I do hope the debug info helps. Let me know anything else you
> > need
> > > and I will do my best to provide it.
> > 
> > Sure. I'll be mostly doing the intermediary work: getting the info
> > added to the bug, though.
> > 
> > > And, should your team decide to open a new ticket or reopen this
> > > original ticket in the JIRA could you add me to the ticket?
> > 
> > You'd have to become OpenJDK author for this[1]. It's not terribly
> > difficult, but it's somewhat of an entry barrier I understand.
> > 
> > > BTW, (off topic), would your recommend submitting a contributor
> > > application to the openjdk project so bug reports can be
> > submitted
> > > directly? 
> > 
> > If you intend to submit the occasional bug report and fix it's
> > easier
> > for you long-term to attempt to become OpenJDK author (which
> > requires
> > signing the OCA[2]).
> > 
> > > The dev group at my company is VERY small (and this message to
> > your
> > > group at the project is from my personal email) but I'd be glad
> > to
> > > submit bug reports as we come across them in our day to day use
> > of
> > > Java.
> > 
> > If there are good reproducers for bugs this would be very welcome.
> > Thanks for investing some time in this!
> > 
> > Cheers,
> > Severin
> > 
> > [1] http://openjdk.java.net/bylaws#author
> > http://openjdk.java.net/projects/#project-author
> > [2] http://oss.oracle.com/oca.pdf
> > 
> > > Cordially,
> > > Dennis
> > > [email protected]
> > > 
> > > On Fri, Jan 18, 2019 at 10:07 AM Severin Gehwolf <
> > [email protected]
> > > > wrote:
> > > > On Thu, 2019-01-17 at 10:00 -0700, Dennis Gesker wrote:
> > > > [...]
> > > > > Added the  -Djavax.net.debug=all option to my Wildfly startup
> > and
> > > > > waited for the pool to close a connection to MySql at AWS.
> > > > > 
> > > > > TXT file attached.
> > > > > 
> > > > > javac 11.0.1
> > > > > mysql jdbc driver 8.0.13
> > > > > wildfly 15.0.1
> > > > > 
> > > > > --drg
> > > > 
> > > > Unfortunately the txt file got stripped by the mailing list.
> > Would
> > > > you
> > > > be able to upload it somewhere and post a link?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > 
> > > 
> > 
> 
> 



Re: JDK-8215102 (Follow-up)

2019-01-24 Thread Severin Gehwolf
Hi,

Thanks for your feedback!

I've tried to reproduce this on my end too, but failed. At least with
MariaDB 10.2.21 and their recent jdbc driver and JDK 11.

This looks like a JDBC driver issue on newer JDKs. Hence, I've noted
that in the bug and closed it:
https://bugs.openjdk.java.net/browse/JDK-8215102

If somebody manages to reproduce this with recent JDBC drivers I'd be
happy to re-open it. mysql-connector-java-5.1.43.jar seems rather old.
Current is 8.0.14.

Thanks,
Severin

On Tue, 2019-01-22 at 18:14 +0530, Jaikiran Pai wrote:
> FWIW - I don't think this is related to WildFly server. So I decided
> to try and reproduce this in a trivial standalone program and I was
> able to reproduce this. Here's the code to reproduce this issue:
> 
> import java.sql.*;
> 
> public class ConnectionCloseTest {
> 
> public static void main(final String[] args) throws Exception {
> final String url = "jdbc:mysql://localhost/?requireSSL=true";
> final String user = "youruser"; // set the right user
> final String pass = "yourpassword"; // set the right password
> Class.forName("com.mysql.jdbc.Driver");
> final Connection conn = DriverManager.getConnection(url,
> user, pass);
> System.out.println("Got connection");
> conn.close();
> System.out.println("Closed connection");
> }
> 
> }
> It's important to start the MySQL server with ssl enabled. For that I
> just had to set:
> [mysqld]
> ssl=1
> in my MySQL server configuration. On the client side you will need
> the mysql JDBC driver jar in the classpath. The one I used for this
> test was mysql-connector-java-5.1.43.jar.
> Running this with Java 8 doesn't throw any exceptions or WARN logs.
> However, running it against Java 11 and even Java 12 latest EA build,
> throws an exception, which gets logged as a WARN by the driver (and
> things move on) on connection close:
> 
> WARN: Caught while disconnecting...
> 
> EXCEPTION STACK TRACE:
> 
> 
> 
> ** BEGIN NESTED EXCEPTION ** 
> 
> javax.net.ssl.SSLException
> MESSAGE: closing inbound before receiving peer's close_notify
> 
> STACKTRACE:
> 
> javax.net.ssl.SSLException: closing inbound before receiving peer's
> close_notify
> at
> java.base/sun.security.ssl.Alert.createSSLException(Alert.java:133)
> at
> java.base/sun.security.ssl.Alert.createSSLException(Alert.java:117)
> at
> java.base/sun.security.ssl.TransportContext.fatal(TransportContext.ja
> va:307)
> at
> java.base/sun.security.ssl.TransportContext.fatal(TransportContext.ja
> va:263)
> at
> java.base/sun.security.ssl.TransportContext.fatal(TransportContext.ja
> va:254)
> at
> java.base/sun.security.ssl.SSLSocketImpl.shutdownInput(SSLSocketImpl.
> java:650)
> at
> java.base/sun.security.ssl.SSLSocketImpl.shutdownInput(SSLSocketImpl.
> java:629)
> at com.mysql.jdbc.MysqlIO.quit(MysqlIO.java:2246)
> at
> com.mysql.jdbc.ConnectionImpl.realClose(ConnectionImpl.java:4234)
> at com.mysql.jdbc.ConnectionImpl.close(ConnectionImpl.java:1472)
> at ConnectionCloseTest.main(ConnectionCloseTest.java:13)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Nativ
> e Method)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Native
> MethodAccessorImpl.java:62)
> at
> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(De
> legatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:567)
> at
> jdk.compiler/com.sun.tools.javac.launcher.Main.execute(Main.java:415)
> at
> jdk.compiler/com.sun.tools.javac.launcher.Main.run(Main.java:192)
> at
> jdk.compiler/com.sun.tools.javac.launcher.Main.main(Main.java:132)
> 
> 
> ** END NESTED EXCEPTION **
> -Jaikiran
> 
> On 22/01/19 7:43 AM, Dennis Gesker wrote:
> > Hi Severing:
> > 
> > I'll post the generic error when I get to the office. It seems to
> > throw the complaints when it closes a connection.
> > 
> > Here is the thing...
> > 
> > 1. I'm glad this found its way to you (being Red Hat guy) as we
> > first noticed it in WildFly 15.0.1.  (But, wasn't looking for it
> > before as we only need it for a few XA migration transactions.)
> > 
> > 2. It MIGHT be the driver as we use PostgreSQL driver mostly -- in
> > the same container -- and no errors there on WildFly 15.0.1 and JDK
> > 11.
> > 
> > 3. I will also try to fall back to JDK 8 and see if it continues in
> > WildFly 15.0.1.
> > 
> >

Re: JDK-8215102 (Follow-up)

2019-01-25 Thread Severin Gehwolf
Hi Jaikiran,

On Fri, 2019-01-25 at 09:47 +0530, Jaikiran Pai wrote:
> Hello Severin,
> 
> Thank you for spending time on this. Although that JIRA was raised for
> in context of MySQL driver, having watched this discussion and looked a
> bit into the exception stacktrace, I think it's not really specific to
> MySQL.
> 
> So I decided to reproduce this with just plain Java SSLSocket APIs and
> was able to reproduce this. I have attached a very simple jtreg test
> case which fails against the latest upstream jdk source repo. The test
> case has details about what it does, but in short, the test creates a
> SSLServerSocket (server) and a SSLSocket (client) and the client sends
> some random data to the server and then calls SSLSocket.shutdownInput().
> That call triggers the exception that's being discussed here. Reading
> the javadoc of SSLSocket.shutdownInput() I don't think the usage of this
> API is incorrect (this is of course based on my limited knowledge of
> that API).
> 
> -Jaikiran

Thanks, I've added that info to the bug and re-opened it:
https://bugs.openjdk.java.net/browse/JDK-8215102?focusedCommentId=14240050&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14240050

It's still not entirely clear to me whether this is wrong API usage or
a JDK bug.

Thanks,
Severin



Re: TLSv1.3 HttpsServer endless loop based on client socket i/o shutdown

2019-03-06 Thread Severin Gehwolf
On Mon, 2019-02-11 at 10:58 +0100, Daniel Fuchs wrote:
> It looks like this is JDK-8214418 - which has been fixed
> in 12.0.1 b03 and 13-ea b04.

Is there any reason why JDK-8214418 is not public?

"You can't view this issue"

Thanks,
Severin



Re: [8u] [RFR] 8175120: Remove old tests on kdc timeout policy

2019-03-12 Thread Severin Gehwolf
Adding security-dev as reviews should happen on the corresponding area
lists. Even for 8.

On Mon, 2019-03-11 at 07:50 +, Andrew John Hughes wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8175120
> Webrev: https://cr.openjdk.java.net/~andrew/openjdk8/8175120/webrev.01/

This looks OK to me. It removes the same tests as was done for JDK 9.
I've verified that KdcPolicy.java passes after your changes. Not a
Reviewer, though.

Thanks,
Severin

> Original review:
> https://mail.openjdk.java.net/pipermail/security-dev/2017-February/015625.html
> 
> This is not a clean backport because the tests in 8u differ slightly
> from those in OpenJDK 10:
> 
> 1. 8u has "JDK-8190690: Impact on krb5 test cases in the 8u nightly".
> That same change (adding a property to the @run line) also needs to be
> applied to the new KdcPolicy.java test for it to succeed.
> 
> 2. 10u contains "JDK-8006690: sun/security/krb5/auto/BadKdc* tests fails
> intermittently" which I assume is obsoleted by the more reliable
> KdcPolicy.java test
> 
> Strangely enough, this & JDK-8164656 were already approved last July,
> but never pushed [0].  I can't see how this was a straightforward
> backport then though either.
> 
> [0] https://mail.openjdk.java.net/pipermail/jdk8u-dev/2018-July/007667.html



Re: [RFR] [8u] 8220641, , New test KdcPolicy.java introduced by JDK-8164656 needs same change as JDK-8190690

2019-03-15 Thread Severin Gehwolf
Hi Andrew,

On Fri, 2019-03-15 at 04:55 +, Andrew John Hughes wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8220641
> Webrev: 
> https://cr.openjdk.java.net/~andrew/openjdk8/8220641/webrev.01/
> 
> This is the patch we split out from my original post for 8175120. It
> applies the same change to the @run command in KdcPolicy.java as was
> applied to the tests deleted by 8175120 in JDK-8190690.
> 
> Without this fix, the test fails with a name lookup error. With it, it
> passes.

This looks good to me. I'm not an 8u Reviewer, though.

Thanks,
Severin


> [0] https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-March/008846.html



Re: Refresh cacert File?

2019-04-18 Thread Severin Gehwolf
Hi,

On Wed, 2019-04-17 at 22:43 +, Bernd Eckenfels wrote:
>  hello,
> 
> I think it was discussed on security-dev before but did not result in
> some action as far as I understand it. Currently the „cacert“ file
> shipped with 8u upstream builds is a bit outdated. It contains
> multiple expired certificates and misses latest additions.

Are you referring to these builds?
https://adoptopenjdk.net/upstream.html

The reason for this is that for OpenJDK 8u upstream builds the cacerts
file will be empty unless the --with-cacerts-file configure option is
being used. That's the case for the above 8u builds[1].

> Also I noted there are multiple vendors struggling with this file. 

There is bound to be divergence as no cacerts file is included upstream
in OpenJDK 8u.

> Since the later Java releases have a canonical source for that file
> with vetted licensing it totally would make sense to refresh I.e.
> backport the changes. Is there anything planned in that direction?

There has been a proposal and IMO it would make sense to backport
JEP319 to JDK 8u:
http://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-March/008975.html

Thanks,
Severin

[1] 
https://github.com/AdoptOpenJDK/openjdk8-upstream-binaries/blob/master/build-openjdk8.sh#L36



[8u] RFR: 8203190: SessionId.hashCode generates too many collisions

2019-05-16 Thread Severin Gehwolf
Hi,

Could I please get a review of this OpenJDK 8u only fix? JDKs 11+ don't
seems to have this issue as with the TLS 1.3 feature (JDK-8196584)
SessionId.hashCode() got changed to use Arrays.hashCode() already.

webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/01/webrev/
Bug: https://bugs.openjdk.java.net/browse/JDK-8203190

The rationale for the fix are these assumptions:

a) elements in trees on hash collision of LinkedHashMap used internally
by the MemoryCache class become prohibitively large for many SessionId
entries in the cache, b) moderate speed of the new hashCode() impl will
not have a detrimental effect on performance overall.

Comparison of performance of hashCode impls[1]:

BenchmarkMode  Cnt Score Error  Units
SessionIDBench.newHashCode  thrpt  100  43649538.284 ±  678702.696  ops/s
SessionIDBench.oldHashCode  thrpt  100  94068843.923 ± 1379930.266  ops/s

Collision testing[2] showed that indeed, the current hashCode()
implementation of SessionId produces more collissions and, thus,
produce more elements in trees for collision resolution in the
underlying LinkedHashMap. The default cache expiry is 24 hours per
entry and this can result in millions of entries in the cache in some
circumstances[3].

Before:
##
Collision test for 100 sessions:

Total number of collisions: 4
Max length of collision list over all buckets: 2

Collision test for 20480 sessions:

Total number of collisions: 18311
Max length of collision list over all buckets: 30

Collision test for 1000 sessions:

Total number of collisions: 9996395
Max length of collision list over all buckets: 9709
##

After:
##
Collision test for 100 sessions:

Total number of collisions: 0

Collision test for 20480 sessions:

Total number of collisions: 0

Collision test for 1000 sessions:

Total number of collisions: 11530
Max length of collision list over all buckets: 2
##


Testing: Above testing, and make test. No new failures.

Thoughts?

Thanks,
Severin

[1] http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/SessionIDBench.java
[2] 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/SessionIdCollissionTest.java
[3] https://bugs.openjdk.java.net/browse/JDK-8210985



Re: [8u] RFR: 8203190: SessionId.hashCode generates too many collisions

2019-05-17 Thread Severin Gehwolf
On Thu, 2019-05-16 at 19:10 +0100, Andrew John Hughes wrote:
> 
> Change looks good.

Thanks for the review.

> Is there a reason the tests aren't included in the webrev? I think it
> would be better to have them checked in, even if they can't be run
> automatically.

The reason was that it's not a good test to be run automatically. It
would have to have some heuristic which it uses as "passed" and "fail".
Checking in the code anyway has a tendency for it to bitrot. If you
really feel strongly about it, I can add it. FWIW, the reference to the
test isn't going away so it'll be available either way.

> They will need copyright headers and I'd correct the spelling of
> 'collision' too :-)

:) Af for the typo: Well spotted.

Thanks,
Severin



Re: [8u] RFR: 8203190: SessionId.hashCode generates too many collisions

2019-05-17 Thread Severin Gehwolf
On Fri, 2019-05-17 at 12:07 +0200, Aleksey Shipilev wrote:
> On 5/16/19 7:51 PM, Severin Gehwolf wrote:
> > Could I please get a review of this OpenJDK 8u only fix? JDKs 11+ don't
> > seems to have this issue as with the TLS 1.3 feature (JDK-8196584)
> > SessionId.hashCode() got changed to use Arrays.hashCode() already.
> > 
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/01/webrev/
> 
> This looks good to me.

Thanks for the review, Aleksey!

Cheers,
Severin



Re: [8u] RFR: 8203190: SessionId.hashCode generates too many collisions

2019-05-17 Thread Severin Gehwolf
On Fri, 2019-05-17 at 16:28 +0100, Andrew John Hughes wrote:
> 
> On 17/05/2019 12:37, Severin Gehwolf wrote:
> 
> snip...
> 
> > The reason was that it's not a good test to be run automatically. It
> > would have to have some heuristic which it uses as "passed" and "fail".
> > Checking in the code anyway has a tendency for it to bitrot. If you
> > really feel strongly about it, I can add it. FWIW, the reference to the
> > test isn't going away so it'll be available either way.
> > 
> 
> I get that, but there are other manual tests in the repositories. I saw
> one yesterday that required downloading a font before running it. I
> think it better to have everything in one place rather than relying on
> someone to find this e-mail thread.
> 
> The bitrot argument seems a little odd, given it would be more open to
> updates in the repositories rather than on a web server where only you
> can update it :/

Sure. Here the webrev with the test:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/02/webrev/

OK to push?

Thanks,
Severin



Re: [8u] RFR: 8203190: SessionId.hashCode generates too many collisions

2019-05-17 Thread Severin Gehwolf
Hi Daniel,

On Fri, 2019-05-17 at 17:15 +0100, Daniel Fuchs wrote:
> Hi Severin,
> 
> Here is an example of a manual test checked in in the jdk repo:
> 
> http://hg.openjdk.java.net/jdk/jdk/file/tip/test/jdk/sun/security/provider/PolicyParser/ExtDirs.java
> 
> - it has an @test annotation
> - it has an @bug annotation
> - it has an @run main/manual line
> - it has a comment explaining how to run the test
>(if necessary) and should have also explain
>in which case it should be declared successful
>or failed...

Thanks! How about this?
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/03/webrev/

Cheers,
Severin

> best regards,
> 
> -- daniel
> 
> On 17/05/2019 17:00, Severin Gehwolf wrote:
> > On Fri, 2019-05-17 at 16:28 +0100, Andrew John Hughes wrote:
> > > On 17/05/2019 12:37, Severin Gehwolf wrote:
> > > 
> > > snip...
> > > 
> > > > The reason was that it's not a good test to be run
> > > > automatically. It
> > > > would have to have some heuristic which it uses as "passed" and
> > > > "fail".
> > > > Checking in the code anyway has a tendency for it to bitrot. If
> > > > you
> > > > really feel strongly about it, I can add it. FWIW, the
> > > > reference to the
> > > > test isn't going away so it'll be available either way.
> > > > 
> > > 
> > > I get that, but there are other manual tests in the repositories.
> > > I saw
> > > one yesterday that required downloading a font before running it.
> > > I
> > > think it better to have everything in one place rather than
> > > relying on
> > > someone to find this e-mail thread.
> > > 
> > > The bitrot argument seems a little odd, given it would be more
> > > open to
> > > updates in the repositories rather than on a web server where
> > > only you
> > > can update it :/
> > 
> > Sure. Here the webrev with the test:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203190/02/webrev/
> > 
> > OK to push?
> > 
> > Thanks,
> > Severin
> > 



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

2019-08-09 Thread Severin Gehwolf
Hi Christoph,

On Fri, 2019-08-09 at 15:04 +, Langer, Christoph wrote:
> 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/

Context differs in JDK 11u. OK. This looks fine to me.

Thanks,
Severin



[8u] RFR: 8218780: Update MUSCLE PCSC-Lite header files

2019-08-26 Thread Severin Gehwolf
Hi,

Could I please get a review of this MUSCLE header files update in
OpenJDK 8u? I'd like to backport this bug as it's also going to be in
Oracle JDK 8u231 (equiv to OpenJDK 8u232) as well. The OpenJDK 11 patch
applies almost cleanly post path-unshuffelling. Changes which didn't
apply were a copyright header update in pcsc.c. I've omitted these.

Bug: https://bugs.openjdk.java.net/browse/JDK-8218780
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218780/jdk8/01/webrev/
JDK 11u changeset: 
https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/0bcc59a50c88

There is going to be a follow-up fix adding back COPYING via 
JDK-8226607 which I propose for backport to OpenJDK 8u as well.

Testing: smartcardio sanity and build on Linux x86_64 (Fedora 30 and RHEL 6)

Thanks to Aleksey Shipilev who helped test this patch.

Thoughts?

Thanks,
Severin



[8u] RFR: 8226607: Inconsistent info between pcsclite.md and MUSCLE headers

2019-08-26 Thread Severin Gehwolf
Hi,

Could I get a review of this follow-up fix for an 8u backport (JDK-
8218780)? This follow-up re-adds a COPYING file to the MUSCLE pcsc
library header files removed by the JDK-8218780 backport. The patch
differs from the version in JDK 11 since there is no pcsclite.md file
in OpenJDK 8u.

Bug: https://bugs.openjdk.java.net/browse/JDK-8226607
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226607/jdk8/01/webrev/
JDK 11u changeset: 
https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/9e304e99cbb2

I intend to push this fix together with JDK-8218780 once it passed
review and got approved.

Thoughts?

Thanks,
Severin





Re: [8u] RFR: 8218780: Update MUSCLE PCSC-Lite header files

2019-09-02 Thread Severin Gehwolf
Hi Andrew,

Thanks for the review!

On Wed, 2019-08-28 at 18:15 +0100, Andrew John Hughes wrote:
> On 26/08/2019 14:23, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I please get a review of this MUSCLE header files update in
> > OpenJDK 8u? I'd like to backport this bug as it's also going to be in
> > Oracle JDK 8u231 (equiv to OpenJDK 8u232) as well. The OpenJDK 11 patch
> > applies almost cleanly post path-unshuffelling. Changes which didn't
> > apply were a copyright header update in pcsc.c. I've omitted these.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8218780
> > webrev: 
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218780/jdk8/01/webrev/
> > JDK 11u changeset: 
> > https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/0bcc59a50c88
> > 
> > There is going to be a follow-up fix adding back COPYING via 
> > JDK-8226607 which I propose for backport to OpenJDK 8u as well.
> > 
> > Testing: smartcardio sanity and build on Linux x86_64 (Fedora 30 and RHEL 6)
> > 
> > Thanks to Aleksey Shipilev who helped test this patch.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 
> 
> Most of this looks good. I was a little confused at first because the
> patch in your webrev looks quite different to the 11u changeset.
> However, once applied locally to the 8u repo, the diff between the two
> was as suggested and the PCSC library files (those in MUSCLE) were
> identical. I don't know what webrev did in creating that patch.
> 
> The bit I don't understand is why you've decided to drop the copyright
> header change on the floor. Just because the original in 8u has 2014,
> while the original in 11u had 2015, does not make the change inapplicable.

OK. I see. I wasn't sure how to deal with copyright year updates. I've
added the copyright update back. The patch is now identical to JDK 11u
(modulo differing copyright year base: 2014 - jdk 8 - vs. 2018 - jdk
11):
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8218780/jdk8/02/webrev


> A better alternative may actually be to backport JDK-8207233 [0] first
> which is a nice little cleanup patch. I suspect this patch would then
> apply cleanly as these PCSC files are rarely touched.
> 
> [0] https://bugs.openjdk.java.net/browse/JDK-8207233

Hmm, this seems overkill just to get the copyright hunk to apply. I'd
prefer to keep this dependency out of scope for this patch. While a
nice clean-up it's not without risk backporting that too. Thoughts?

Thanks,
Severin



Re: [8u] RFR: 8226607: Inconsistent info between pcsclite.md and MUSCLE headers

2019-09-02 Thread Severin Gehwolf
On Mon, 2019-09-02 at 15:38 +0100, Andrew John Hughes wrote:
> 
> On 26/08/2019 14:24, Severin Gehwolf wrote:
> > Hi,
> > 
> > Could I get a review of this follow-up fix for an 8u backport (JDK-
> > 8218780)? This follow-up re-adds a COPYING file to the MUSCLE pcsc
> > library header files removed by the JDK-8218780 backport. The patch
> > differs from the version in JDK 11 since there is no pcsclite.md file
> > in OpenJDK 8u.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8226607
> > webrev: 
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226607/jdk8/01/webrev/
> > JDK 11u changeset: 
> > https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/9e304e99cbb2
> > 
> > I intend to push this fix together with JDK-8218780 once it passed
> > review and got approved.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> > 
> > 
> > 
> 
> The *.md files in OpenJDK 9+ are the modular equivalent of the
> THIRD_PARTY_README file found in each OpenJDK 8u repository. See my
> review of JDK-8217676 [0] for Zhengyu for more details.
> 
> For reference, the conversion took place in JDK-8169925.

Thanks for this. Updated webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226607/jdk8/02/webrev/

I intend to push the same updat to 
THIRD_PARTY_README files on all other repos. Example here is jdk repo.
Do you want to see webrevs of this THIRD_PARTY_README update for all 7
other repos?

Thanks,
Severin

> [0]
> https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-August/010116.html
> [1] https://bugs.openjdk.java.net/browse/JDK-8169925



Re: [8u] RFR: 8226607: Inconsistent info between pcsclite.md and MUSCLE headers

2019-09-25 Thread Severin Gehwolf
On Wed, 2019-09-25 at 15:59 +0100, Andrew John Hughes wrote:
> On 02/09/2019 16:05, Severin Gehwolf wrote:
> > On Mon, 2019-09-02 at 15:38 +0100, Andrew John Hughes wrote:
> > > On 26/08/2019 14:24, Severin Gehwolf wrote:
> > > > Hi,
> > > > 
> > > > Could I get a review of this follow-up fix for an 8u backport (JDK-
> > > > 8218780)? This follow-up re-adds a COPYING file to the MUSCLE pcsc
> > > > library header files removed by the JDK-8218780 backport. The patch
> > > > differs from the version in JDK 11 since there is no pcsclite.md file
> > > > in OpenJDK 8u.
> > > > 
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8226607
> > > > webrev: 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226607/jdk8/01/webrev/
> > > > JDK 11u changeset: 
> > > > https://hg.openjdk.java.net/jdk-updates/jdk11u-dev/rev/9e304e99cbb2
> > > > 
> > > > I intend to push this fix together with JDK-8218780 once it passed
> > > > review and got approved.
> > > > 
> > > > Thoughts?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > 
> > > > 
> > > 
> > > The *.md files in OpenJDK 9+ are the modular equivalent of the
> > > THIRD_PARTY_README file found in each OpenJDK 8u repository. See my
> > > review of JDK-8217676 [0] for Zhengyu for more details.
> > > 
> > > For reference, the conversion took place in JDK-8169925.
> > 
> > Thanks for this. Updated webrev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226607/jdk8/02/webrev/
> > 
> > I intend to push the same updat to 
> > THIRD_PARTY_README files on all other repos. Example here is jdk repo.
> > Do you want to see webrevs of this THIRD_PARTY_README update for all 7
> > other repos?
> > 
> > Thanks,
> > Severin
> > 
> > > [0]
> > > https://mail.openjdk.java.net/pipermail/jdk8u-dev/2019-August/010116.html
> > > [1] https://bugs.openjdk.java.net/browse/JDK-8169925
> 
> I'm happy assuming the same THIRD_PARTY_README change for all repos.
> 
> This looks fine to me.

Thanks for the review!

> Can you flag it jdk8u-critical-request so we can
> get this into 8u232 with JDK-8218780?

Done.

Thanks,
Severin



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

2019-12-17 Thread Severin Gehwolf
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



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

2019-12-17 Thread Severin Gehwolf
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: 8233223: Add Amazon Root CA certificates

2019-12-19 Thread Severin Gehwolf
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

2019-12-19 Thread Severin Gehwolf
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: 8232019: Add LuxTrust certificate updates to the existing root program

2019-12-20 Thread Severin Gehwolf
On Fri, 2019-12-20 at 07:42 +, Andrew John Hughes wrote:
> 
> On 19/12/2019 20:13, Severin Gehwolf wrote:
> 
> snip...
> 
> > > 
> > > 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
> > 
> 
> There's an option #4:
> 
> 4. Propose these backports for 8u242 and do the correct backports, with
> JDK-8193255 and friends, in 8u252.

OK.

> 8193255 is sensitive to the status of cacerts at the time it is applied.
> It needs to be backported with cacerts containing the same certificates
> as it did when applied in later versions, so that the textual
> replacements match. By applying these backports in 8u252, we'd
> complicate the 8193255 backport further by having to effectively include
> backports of the Amazon & LuxTrust updates in it.
> 
> So what I'd suggest is:
> 
> 1. Do the full backport series in 8u252 from 8193255 on.
> 2. Create a separate bug for 8u242 to add the new certificates and apply
> for jdk8u-critical-request for that.

Can you clarify why a separate bug for 8u242 would be needed? Why can't
we just use 8232019 and 8233223? I'd include mentioning 8232019 and
8233223 for the backport of 8193255 for it to be clear that it has been
taken care of those additional cert updates. The 8193255 backport would
only start once 8u242 backports got merged back into jdk8u-dev. At no
point we'd miss inclusion of those. Saves creation of extra bugs.

Thanks,
Severin

> When the two are merged together, the webrev changes in 8u242 should be
> the same as those in 8u252, and the changed cacerts binary can just be
> deleted.
> 
> Thanks,



Re: RFR 8191137: keytool fails to format resource strings for keys for some languages after JDK-8171319

2017-11-14 Thread Severin Gehwolf
Hi,

On Tue, 2017-11-14 at 12:20 +0800, Weijun Wang wrote:
> keytool contains a printf("%d-bit %s key", 1024, "RSA") call but when it's 
> translated into French the call becomes printf("Clave %s de %d bits", 1024, 
> "RSA") and %s does not match 1024.
> 
> The fix adds position parameters to printf resources strings when there are 
> multiple arguments. The translations should follow.
> 
> diff --git 
> a/src/java.base/share/classes/sun/security/tools/keytool/Resources.java 
> b/src/java.base/share/classes/sun/security/tools/keytool/Resources.java
> --- a/src/java.base/share/classes/sun/security/tools/keytool/Resources.java
> +++ b/src/java.base/share/classes/sun/security/tools/keytool/Resources.java
> @@ -456,21 +456,21 @@
>          {"the.tsa.certificate", "The TSA certificate"},
>          {"the.input", "The input"},
>          {"reply", "Reply"},
> -        {"one.in.many", "%s #%d of %d"},
> +        {"one.in.many", "%1$s #%2$d of %3$d"},
>          {"alias.in.cacerts", "Issuer <%s> in cacerts"},
>          {"alias.in.keystore", "Issuer <%s>"},
>          {"with.weak", "%s (weak)"},
> -        {"key.bit", "%d-bit %s key"},
> -        {"key.bit.weak", "%d-bit %s key (weak)"},
> +        {"key.bit", "%1$d-bit %2$s key"},
> +        {"key.bit.weak", "%1$d-bit %2$s key (weak)"},
>          {"unknown.size.1", "unknown size %s key"},
>          {".PATTERN.printX509Cert.with.weak",
>                  "Owner: {0}\nIssuer: {1}\nSerial number: {2}\nValid from: 
> {3} until: {4}\nCertificate fingerprints:\n\t SHA1: {5}\n\t SHA256: 
> {6}\nSignature algorithm name: {7}\nSubject Public Key Algorithm: 
> {8}\nVersion: {9}"},
>          {"PKCS.10.with.weak",
>                  "PKCS #10 Certificate Request (Version 1.0)\n" +
> -                        "Subject: %s\nFormat: %s\nPublic Key: %s\nSignature 
> algorithm: %s\n"},
> -        {"verified.by.s.in.s.weak", "Verified by %s in %s with a %s"},
> -        {"whose.sigalg.risk", "%s uses the %s signature algorithm which is 
> considered a security risk."},
> -        {"whose.key.risk", "%s uses a %s which is considered a security 
> risk."},
> +                        "Subject: %1$s\nFormat: %2$s\nPublic Key: 
> %3$s\nSignature algorithm: %4$s\n"},
> +        {"verified.by.s.in.s.weak", "Verified by %1$s in %2$s with a %3$s"},
> +        {"whose.sigalg.risk", "%1$s uses the %2$s signature algorithm which 
> is considered a security risk."},
> +        {"whose.key.risk", "%1$s uses a %2$s which is considered a security 
> risk."},
>          {"jks.storetype.warning", "The %1$s keystore uses a proprietary 
> format. It is recommended to migrate to PKCS12 which is an industry standard 
> format using \"keytool -importkeystore -srckeystore %2$s -destkeystore %2$s 
> -deststoretype pkcs12\"."},
>          {"migrate.keystore.warning", "Migrated \"%1$s\" to %4$s. The %2$s 
> keystore is backed up as \"%3$s\"."},
>          {"backup.keystore.warning", "The original keystore \"%1$s\" is 
> backed up as \"%3$s\"..."},
> diff --git 
> a/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java 
> b/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java
> --- 
> a/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java
> +++ 
> b/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java
> @@ -270,7 +270,7 @@
>          
> {"The.1.algorithm.specified.for.the.2.option.is.considered.a.security.risk.",
>                  "The %1$s algorithm specified for the %2$s option is 
> considered a security risk."},
>          
> {"The.1.signing.key.has.a.keysize.of.2.which.is.considered.a.security.risk.",
> -                "The %s signing key has a keysize of %d which is considered 
> a security risk."},
> +                "The %1$s signing key has a keysize of %2$d which is 
> considered a security risk."},
>          
> {"This.jar.contains.entries.whose.certificate.chain.is.invalid.reason.1",
>                   "This jar contains entries whose certificate chain is 
> invalid. Reason: %s"},
>          
> {"This.jar.contains.entries.whose.tsa.certificate.chain.is.invalid.reason.1",
> 
> After this there is neither /%.*%[^0-9]/ nor /%[^0-9].*%/ patterns.

This looks fine, but I wonder if a regression test would be in order.
E.g. test/sun/security/tools/keytool/WeakAlg.java with
-Duser.language=fr or some such.

Thanks,
Severin


Re: RFR 8191137: keytool fails to format resource strings for keys for some languages after JDK-8171319

2017-11-14 Thread Severin Gehwolf
On Tue, 2017-11-14 at 18:47 +0800, Wang Weijun wrote:
> > 在 2017年11月14日,18:02,Severin Gehwolf  写道:
> > 
> > This looks fine, but I wonder if a regression test would be in
> > order.
> > E.g. test/sun/security/tools/keytool/WeakAlg.java with
> > -Duser.language=fr or some such.
> 
> But my change has not really fixed anything. It’s just a hint on how
> to correctly translate the strings. The test you suggested would
> still fail after this change. It might be useful when the French
> translation is updated. 

Why can't this change update Resources_fr.java to include the correct
position and then the test updated to also work in French? It's not
really a localization change, but introduction of the correct position
of the parameter.

A follow-up bug could then move all other resources to a model similar
to Resources_fr.java?

Thanks,
Severin


Re: On 8202598: [linux] keytool -certreq inconsistent with platform line.separator

2018-06-27 Thread Severin Gehwolf
Hi Max,

On Wed, 2018-06-27 at 09:15 +0800, Weijun Wang wrote:
> Hi Severin and/or Andrew
> 
> I'm going through all security bugs with JDK 11 in affected versions and 
> noticed this one:
> 
>8202598: [linux] keytool -certreq inconsistent with platform line.separator
>https://bugs.openjdk.java.net/browse/JDK-8202598
> 
> What kind of interop issue have you observed? IMHO, \r\n is legal in a PEM 
> file.

All we know is that this breaks interop with tools on Linux/Unix which
don't expect \r\n in PEM files.

> Also, you mentioned a patch in the comment. Can I take a look?

I've posted a link to the JDK 8 patch in the bug report.

Thanks,
Severin


Re: Bug in HttpClient

2018-07-20 Thread Severin Gehwolf
Adding net-dev

On Fri, 2018-07-20 at 08:52 +0200, Thomas Lußnig wrote:
> Hi,
> i found an bug in JDK 10 with the new HttpClient. It does not handle
> responses wihtout contentlength correctly.
> Normally i would expect that the content is returned even without
> content length. Since i can not open an JDK bug
> i hope some person from the list can do it. Below is an example that
> show the problem.
> 
> Gruß Thomas Lußnig
> import java.io.InputStream;
> import java.io.OutputStream;
> import java.net.InetSocketAddress;
> import java.net.ServerSocket;
> import java.net.Socket;
> import java.net.URI;
> import java.time.Duration; 
> import javax.net.ServerSocketFactory;
> import jdk.incubator.http.HttpClient;
> import jdk.incubator.http.HttpRequest;
> import jdk.incubator.http.HttpResponse; 
> public class Client1 {
>static void server(final boolean withContentLength) {
>  try(ServerSocket ss =
> ServerSocketFactory.getDefault().createServerSocket()) {
> ss.setReuseAddress(true);
> ss.bind(new InetSocketAddress("127.0.0.1",80));
> final byte[] buf = new byte[120400];
> try(Socket s = ss.accept()) {
>   System.out.println("Accepted:
> "+s.getRemoteSocketAddress());
>   try(  OutputStream os =
> s.getOutputStream(); InputStream is = s.getInputStream()) {
>  is.read(buf);
>  is.read(buf);
>  os.write("HTTP/1.0 200
> OK\r\nConnection: close\r\nContent-Type: text/xml; charset=UTF-
> 8\r\n".getBytes());
>  if(withContentLength)
> os.write("Content-Length: 4\r\n".getBytes());
>  os.write("\r\n".getBytes());
>  os.write("".getBytes());
>  os.flush();
>   }
> }
>  } catch(final Throwable t) { t.printStackTrace(); }
>   }
>static void client() {
>  try {
> final HttpClient client =
> HttpClient.newBuilder().version(HttpClient.Version.HTTP_2).build();
> final HttpResponse response = client
> .send(HttpRequest.newBuilder(new URI("htt
> p://127.0.0.1/test")).timeout(Duration.ofMillis(120_000))
> 
> .POST(HttpRequest.BodyPublisher.fromString("body")).build(),
> HttpResponse.BodyHandler.asString());
> System.out.println("Received reply: " +
> response.statusCode());
> System.out.println("Received body: " +
> response.body());
>  } catch(final Throwable t) { t.printStackTrace(); }
>   }
> public static void main(final String[] args) throws Exception
> {
>  new Thread(()->server(true)).start();
>  client();
>  new Thread(()->server(false)).start();
>  client();
>}
> }


Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary

2022-03-03 Thread Severin Gehwolf
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers  wrote:

> When testing compatibility of jdk TLS implementation with gnutls, I have 
> found a problem. The problem is, that gnutls does not like use of 
> user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() 
> (used by socket.close() unless shutdownOutput was called explicitly) and 
> considers it error. (For more details see: [1])
> 
> As I understand it, usage of user_canceled alert before close is workaround 
> for an issue of not being able to cleanly initialize full (duplex) close of 
> TLS-1.3 connection (other side is not required to immediately close the after 
> receiving close_notify, unlike in earlier TLS versions). Some legacy programs 
> could probably hang or something, expecting socket.close to perform immediate 
> duplex close. Problem is this is not what user_canceled alert is intended for 
> [2] and it is therefore undefined how the other side handles this. (JDK 
> itself replies to close_notify preceded by user_canceled alert by immediately 
> closing its output [3].)
> 
> This fix disables this workaround when it is not necessary (connection is 
> already half-closed by the other side). This way it fixes my case (gnutls 
> client connected to jdk server initiates close) and it should be safe. (As 
> removing workaround altogether could probably reintroduce issues for legacy 
> apps... )
> 
> I also ran jdk_security tests locally, which passed for me.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473
> [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1
> [3] 
> https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243

@zzambers I've create https://bugs.openjdk.java.net/browse/JDK-8282600 for this 
issue. Please reference it in the PR title.

-

PR: https://git.openjdk.java.net/jdk/pull/7664


Re: [11u] RFR: 8243559: Remove root certificates with 1024-bit keys

2021-03-16 Thread Severin Gehwolf
Hi Martin,

On Mon, 2021-03-15 at 17:10 +, Doerr, Martin wrote:
> 11u backport:
> http://cr.openjdk.java.net/~mdoerr/8261209_xml_11u/webrev.00/

This doesn't look like the right webrev to me. Could you please double-
check?

Thanks,
Severin



Re: [11u] RFR: 8243559: Remove root certificates with 1024-bit keys

2021-03-16 Thread Severin Gehwolf
On Tue, 2021-03-16 at 10:39 +, Doerr, Martin wrote:
> http://cr.openjdk.java.net/~mdoerr/8243559_root_ca_11u/webrev.00/

This looks good to me.

Thanks,
Severin



[8u] RFR: 8206925: Support the certificate_authorities extension

2021-04-20 Thread Severin Gehwolf
Hi,

Please review this OpenJDK 8u backport of the certificate_authorities
extensionj. The OpenJDK 11u patch didn't apply cleanly after path
unshuffeling, but was fairly trivial to resolve. Conflicts caused by:

1. X509Authentication.java copyright line conflict only. Resolved
   manually.
2. SSLContextTemplate.java private interface methods not allowed in
   JDK 8. It's a JDK 9+ feature via JEP 213. Changed modifier to
   default. Note: this is code used in tests only.
3. TooManyCAs.java. Added -Djdk.tls.client.protocols=TLSv1.3 to the
   test invocations since JDK 8u doesn't enable TLSv1.3 on the
   client side by default. See JDK-8248721, CSR of the TLSv1.3 8u
   backport.

Other than that, the patch is identical to the OpenJDK 11.0.12 version
of this patch.

This introduces a new system property,
jdk.tls.client.enableCAExtension, for compatibilty reasons. CSR has
been reused from the Oracle JDK backport. See below.

Bug: https://bugs.openjdk.java.net/browse/JDK-8206925
CSR: https://bugs.openjdk.java.net/browse/JDK-8248709
webrev: 
https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8206925/jdk8/02/webrev/

Testing: sun/security/ssl tests and tier1. No new regressions.
 New tests pass.

Thoughts?

Thanks,
Severin



Re: Ping? [8u] RFR: 8206925: Support the certificate_authorities extension

2021-04-29 Thread Severin Gehwolf
Anyone?

On Tue, 2021-04-20 at 12:23 +0200, Severin Gehwolf wrote:
> Hi,
> 
> Please review this OpenJDK 8u backport of the certificate_authorities
> extensionj. The OpenJDK 11u patch didn't apply cleanly after path
> unshuffeling, but was fairly trivial to resolve. Conflicts caused by:
> 
> 1. X509Authentication.java copyright line conflict only. Resolved
>    manually.
> 2. SSLContextTemplate.java private interface methods not allowed in
>    JDK 8. It's a JDK 9+ feature via JEP 213. Changed modifier to
>    default. Note: this is code used in tests only.
> 3. TooManyCAs.java. Added -Djdk.tls.client.protocols=TLSv1.3 to the
>    test invocations since JDK 8u doesn't enable TLSv1.3 on the
>    client side by default. See JDK-8248721, CSR of the TLSv1.3 8u
>    backport.
> 
> Other than that, the patch is identical to the OpenJDK 11.0.12
> version
> of this patch.
> 
> This introduces a new system property,
> jdk.tls.client.enableCAExtension, for compatibilty reasons. CSR has
> been reused from the Oracle JDK backport. See below.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8206925
> CSR: https://bugs.openjdk.java.net/browse/JDK-8248709
> webrev:
> https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8206925/jdk8/02/webrev/
> 
> Testing: sun/security/ssl tests and tier1. No new regressions.
>  New tests pass.
> 
> Thoughts?
> 
> Thanks,
> Severin




Re: Ping? [8u] RFR: 8206925: Support the certificate_authorities extension

2021-05-10 Thread Severin Gehwolf
Hi!

Would anyone be willing to review this?

Many thanks in advance!

Cheers,
Severin

On Thu, 2021-04-29 at 17:24 +0200, Severin Gehwolf wrote:
> Anyone?
> 
> On Tue, 2021-04-20 at 12:23 +0200, Severin Gehwolf wrote:
> > Hi,
> > 
> > Please review this OpenJDK 8u backport of the
> > certificate_authorities
> > extensionj. The OpenJDK 11u patch didn't apply cleanly after path
> > unshuffeling, but was fairly trivial to resolve. Conflicts caused
> > by:
> > 
> > 1. X509Authentication.java copyright line conflict only. Resolved
> >    manually.
> > 2. SSLContextTemplate.java private interface methods not allowed in
> >    JDK 8. It's a JDK 9+ feature via JEP 213. Changed modifier to
> >    default. Note: this is code used in tests only.
> > 3. TooManyCAs.java. Added -Djdk.tls.client.protocols=TLSv1.3 to the
> >    test invocations since JDK 8u doesn't enable TLSv1.3 on the
> >    client side by default. See JDK-8248721, CSR of the TLSv1.3 8u
> >    backport.
> > 
> > Other than that, the patch is identical to the OpenJDK 11.0.12
> > version
> > of this patch.
> > 
> > This introduces a new system property,
> > jdk.tls.client.enableCAExtension, for compatibilty reasons. CSR has
> > been reused from the Oracle JDK backport. See below.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8206925
> > CSR: https://bugs.openjdk.java.net/browse/JDK-8248709
> > webrev:
> > https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8206925/jdk8/02/webrev/
> > 
> > Testing: sun/security/ssl tests and tier1. No new regressions.
> >  New tests pass.
> > 
> > Thoughts?
> > 
> > Thanks,
> > Severin
> 




Re: TLS v1.3 extensions in TLS v1.2 handshake

2021-05-25 Thread Severin Gehwolf
CC'ing jdk8u-dev list.

Fridrich, is this an 8u-only problem you are observing? Would you have
some details about the problem so that I can file a bug for you?

Thanks,
Severin

On Tue, 2021-05-25 at 07:12 +0200, Fridrich Strba wrote:
> Hello, good people,
> 
> The java 11 implementation of TLS v1.3 was backported into java 8
> since some CPUs and it results sometimes in new handshake failures
> with hard-to-updage-firmware devices whose shell life might be still
> long.
> 
> We somehow debugged those failures and some devices bomb because of 
> TLSv1.2 handshake containing the signature_algorihms_cert and 
> supported_versions extensions.
> 
> TLSv1.3 handshake still contains both extensions as it should. This 
> could solve the differences of Java 8 behaviour between different
> update 
> numbers.
> 
> Please, have a look and comment
> 
> Cheers
> 
> Fridrich
>