[11u] Second opinion? RFR: 8171279: Support X25519 and X448 in TLS
Severin Gehwolf
sgehwolf at redhat.com
Mon Oct 12 10:24:00 UTC 2020
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
More information about the jdk-updates-dev
mailing list