[11u] Second opinion? RFR: 8171279: Support X25519 and X448 in TLS

Thu Nov 5 13:30:53 UTC 2020

Hi,

We agreed internally that this needs no further work, so I'll push it tomorrow.

Best regards,
  Goetz.

> -----Original Message-----
> From: Lindenmaier, Goetz
> Sent: Monday, October 12, 2020 1:24 PM
> To: 'Severin Gehwolf' <sgehwolf at redhat.com>; 'Alvarez, David'
> <alvdavi at amazon.com>; Doerr, Martin <martin.doerr at sap.com>; jdk-
> updates-dev at openjdk.java.net
> Cc: Martin Balao Alonso <mbalao at redhat.com>
> Subject: RE: [11u] Second opinion? RFR: 8171279: Support X25519 and X448
> in TLS
> 
> Hi Severin, Martin,
> 
> That's fine, there is no hurry!  Any input is
> appreciated.
> 
> Best regards,
>   Goetz.
> 
> > -----Original Message-----
> > From: Severin Gehwolf <sgehwolf at redhat.com>
> > Sent: Monday, October 12, 2020 12:24 PM
> > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; 'Alvarez, David'
> > <alvdavi at amazon.com>; Doerr, Martin <martin.doerr at sap.com>; jdk-
> > updates-dev at openjdk.java.net
> > Cc: Martin Balao Alonso <mbalao at redhat.com>
> > Subject: Re: [11u] Second opinion? RFR: 8171279: Support X25519 and
> X448
> > in TLS
> >
> > Hi Götz,
> >
> > On Mon, 2020-10-12 at 06:39 +0000, Lindenmaier, Goetz wrote:
> > > Hi,
> > >
> > > I gave it a try:
> > > http://cr.openjdk.java.net/~goetz/wr20/8171279-X25519_in_TLS-
> > jdk11/02-incremental/
> > > I think this incremental change restores the lost functionality.
> > > It does not break any of the existing tests. But I didn't run
> > > any tests for FIPS.
> > >
> > > On the other side, Matthias found this:
> > > https://github.com/elastic/elasticsearch/issues/37250
> > > The larger comment a bit down states that FIPS is unusable in 11.
> > > If so, I would prefer to skip the incremental change above.
> > >
> > > Full webrev:
> > > http://cr.openjdk.java.net/~goetz/wr20/8171279-X25519_in_TLS-
> > jdk11/02/
> >
> > Adding Martin Balao who has done some FIPS work. He mentioned that
> > he'll be looking at this after the 11.0.9 release is done. So if you
> > would be OK with waiting a bit before getting this integrated we'd
> > appreciate it. It's still early in the 11.0.10 cycle. Thoughts?
> >
> > Thanks,
> > Severin
> >
> > > Best regards,
> > >   Goetz
> > >
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: jdk-updates-dev <jdk-updates-dev-retn at openjdk.java.net> On
> > Behalf
> > > > Of Lindenmaier, Goetz
> > > > Sent: Thursday, October 8, 2020 6:37 PM
> > > > To: 'Alvarez, David' <alvdavi at amazon.com>; Doerr, Martin
> > > > <martin.doerr at sap.com>; jdk-updates-dev at openjdk.java.net
> > > > Subject: [CAUTION] RE: [11u] Second opinion? RFR: 8171279: Support
> > > > X25519 and X448 in TLS
> > > >
> > > > Hi David,
> > > >
> > > > > I haven't tested the full FIPS functionality,
> > > > That is the problem. I don't have appropriate tests, either.
> > > >
> > > > > but code like this:
> > > > > import com.sun.net.ssl.internal.ssl.Provider;
> > > > >
> > > > > public class FipsProviderTest {
> > > > >     public static void main(String[] args) {
> > > > >         com.sun.net.ssl.internal.ssl.Provider p = new Provider(
> > > > >                 "cryptoProvider");
> > > > >     }
> > > > > }
> > > > >
> > > > >
> > > > > That makes use of that constructor will no longer work.
> > > >
> > > > Ok, I see how you can reach the FIPS functionality.
> > > > But the example is obviously too small to touch
> > > > the code I changed.
> > > > It compiles fine, with and without the change.
> > > > And it runs fine if I add SunJSSE as provider, for example.
> > > >
> > > > > I don't know
> > > > > what is our statement regarding the usage of --add-exports, are we
> ok
> > > > > breaking solutions that make use of it? If we are, is it only when we
> > > > > have no other option or will be one situation like this one acceptable?
> > > > Yes, I would prefer to not change the behavior even for
> > > > things not explicitly exposed.
> > > >
> > > > Best regards,
> > > >   Goetz.
> > > >
> > > > > Thanks,
> > > > > David
> > > > >
> > > > >
> > > > > On Wed, 2020-10-07 at 07:50 +0000, Lindenmaier, Goetz wrote:
> > > > > > CAUTION: This email originated from outside of the organization.
> Do
> > > > > > not click links or open attachments unless you can confirm the
> sender
> > > > > > and know the content is safe.
> > > > > >
> > > > > >
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > I would like to get a second opinion on how to deal
> > > > > > with the lost FIPS coding in this downport.
> > > > > > Could someone from outside SAP have a look please?
> > > > > >
> > > > > > Thanks,
> > > > > >   Goetz.
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Doerr, Martin <martin.doerr at sap.com>
> > > > > > > Sent: Tuesday, October 6, 2020 8:12 PM
> > > > > > > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; jdk-
> > updates-
> > > > dev
> > > > > > > <jdk-updates-dev at openjdk.java.net>
> > > > > > > Subject: RE: [11u] RFR: 8171279: Support X25519 and X448 in TLS
> > > > > > >
> > > > > > > Hi Götz,
> > > > > > >
> > > > > > > thanks for backporting this change. I think it's good to put it
> > > > > > > into 11.0.10,
> > > > > > > now, so we have enough testing time.
> > > > > > > Please find comments and questions inline.
> > > > > > >
> > > > > > >
> > > > > > > > file
> > > > > > > > src/java.base/share/classes/sun/security/ssl/CipherSuite.java
> > > > > > > > Non-applying chunk fixes typo in a comment that is not in 11.
> > > > > > > > I copied the comment to 11.
> > > > > > >
> > > > > > > Good.
> > > > > > >
> > > > > > >
> > > > > > > > file
> > > > > > > >
> src/java.base/share/classes/sun/security/ssl/DHKeyExchange.java
> > > > > > > > Deleting class DHEKAKeyDerivation failed because the code in 11
> > > > > > > > uses
> > > > > > > > JsseJce.getKeyAgreement()
> > > > > > > > where the patch uses KeyAgreement.getInstance().
> > > > > > >
> > > > > > > Right.
> > > > > > >
> > > > > > >
> > > > > > > > file
> > > > > > > >
> > src/java.base/share/classes/sun/security/ssl/ECDHClientKeyExchang
> > > > > > > > e.java
> > > > > > > > 11 has different imports.
> > > > > > > > The remaining conflicts arise from different usages of
> > > > > > > > KeyFactory, ECUtil
> > > > > > >
> > > > > > > and
> > > > > > > > JsseJce.
> > > > > > >
> > > > > > > Good.
> > > > > > >
> > > > > > >
> > > > > > > > file
> > > > > > > >
> > src/java.base/share/classes/sun/security/ssl/ECDHKeyExchange.java
> > > > > > > > The conflicts arise from different usages of KeyAgreement and
> > > > > > > > JsseJce.
> > > > > > >
> > > > > > > Ok.
> > > > > > >
> > > > > > >
> > > > > > > > file
> > > > > > > >
> > src/java.base/share/classes/sun/security/ssl/ECDHServerKeyExchang
> > > > > > > > e.java
> > > > > > > > Different imports.
> > > > > > > > The remaining conflicts arise from different usages of
> > > > > > > > KeyFactory, ECUtil
> > > > > > >
> > > > > > > and
> > > > > > > > JsseJce.
> > > > > > >
> > > > > > > Ok.
> > > > > > >
> > > > > > >
> > > > > > > > file
> > > > > > > > src/java.base/share/classes/sun/security/ssl/SSLExtension.java
> > > > > > > > Copyright
> > > > > > > >
> > > > > > > > file
> > > > > > >
> > > > > > >
> > src/java.base/share/classes/sun/security/ssl/SSLSocketInputRecord.j
> > > > > > > ava
> > > > > > > > Copyright
> > > > > > >
> > > > > > > We don't edit Copyright headers for backports. Ok.
> > > > > > >
> > > > > > >
> > > > > > > > file
> > > > > > > >
> > src/java.base/share/classes/sun/security/ssl/SignatureScheme.java
> > > > > > > > imports
> > > > > > >
> > > > > > > Ok.
> > > > > > >
> > > > > > >
> > > > > > > > file
> > > > > > > >
> > > > > > >
> > > > > > >
> > src/java.base/share/classes/sun/security/ssl/SupportedGroupsExtensi
> > > > > > > on.jav
> > > > > > > > a
> > > > > > > > Different imports.
> > > > > > >
> > > > > > > Ok.
> > > > > > >
> > > > > > > > Enums are moved to a file of their own: NamedGroup.java
> > > > > > > > Removing the enums does not apply because in 13 FIPS support
> > was
> > > > > > > > removed.
> > > > > > >
> > > > > > > So you just deleted them manually. Fine.
> > > > > > >
> > > > > > > > Also, a name string was added.
> > > > > > >
> > > > > > > You mean "String name" in the code you removed?
> > > > > > >
> > > > > > > > The supported group types differ, too.
> > > > > > >
> > > > > > > That's expected. Did that cause manual integration?
> > > > > > >
> > > > > > > > In 11, the enums have a row of additional functionality that
> > > > > > > > I now removed, too.  E.g., additional constructors,
> > > > > > > > isSupported(),
> > > > > > > > isECAvailable.
> > > > > > >
> > > > > > > That's in the code which is gone. That's fine.
> > > > > > >
> > > > > > > > Some further differences are again usages of ECUtil, JsseJce etc.
> > > > > > > > The original patch has more NamedGroups: "// Secondary NIST
> > > > > > > > curves"
> > > > > > >
> > > > > > > Ok.
> > > > > > >
> > > > > > > > file test/jdk/javax/net/ssl/templates/SSLSocketTemplate.java
> > > > > > > > configureClientSocket() already in 11. We downported this with
> > > > > > > > a previous change.
> > > > > > >
> > > > > > > Ok. I can see this code is already there.
> > > > > > >
> > > > > > > > The NamedGroups enum is moved to a file of it's own.
> > > > > > > > Before this change, the named Groups knew about FIPS in 11.
> The
> > > > > > > > new
> > > > > > >
> > > > > > > ones
> > > > > > > > don't.
> > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8217835 "Remove
> the
> > > > > > > > experimental SunJSSE FIPS compliant mode"
> > > > > > > > removed FIPS support in 13 before this change was implemented.
> > > > > > > >
> > > > > > > > As I understand 8217835, the experimental FIPS feature cannot
> be
> > > > > > > > used in
> > > > > > > > 11, it just remained in the code. So I skipped adapting
> > > > > > > > NamedGroups to the
> > > > > > > > old behaviour, I can not test it anyways.
> > > > > > > > What do you think, do we need to add this code to
> > > > > > > > NamedGroupd.java
> > > > > > > > again?
> > > > > > >
> > > > > > > Sounds like this is ok according to the CSR
> > > > > > > https://bugs.openjdk.java.net/browse/JDK-8217907
> > > > > > > Since this file is new with this backport, I prefer keeping it like
> > > > > > > the original
> > > > > > > one for the backport.
> > > > > > > We could open a new bug in case anybody wants it back, but I
> guess
> > > > > > > that's
> > > > > > > not the case.
> > > > > > >
> > > > > > > There's still some Fips code there:
> > > > > > > (!requireFips || namedGroup.isFips)*/     ) {  // GL fix isFips
> > > > > > > With a comment!
> > > > > > > What are you planning to do with it?
> > > > > > >
> > > > > > >
> > > > > > > > There are two follow ups,
> > > > > > > > 8224650: Add tests to support X25519 and X448 in TLS
> > > > > > > > 8243549:
> > > > > > > > sun/security/ssl/CipherSuite/NamedGroupsWithCipherSuite.java
> > > > > > > > failed with Unsupported signature algorithm: DSA
> > > > > > > >
> > > > > > > > I have downported these too, and will send RFR once this is
> > > > > > > > stable.
> > > > > > > > Existing and new test are all passing.
> > > > > > > >
> > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8171279
> > > > > > > > http://hg.openjdk.java.net/jdk/jdk/rev/946f7f2d321c
> > > > > > >
> > > > > > > Ok. That's an important point.
> > > > > > > There may be more to come, but that's all we currently have
> AFAICS.
> > > > > > >
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Martin