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

[Lindenmaier, Goetz]

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/

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
> > >
> > >