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

[Lindenmaier, Goetz]

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