[11u] Second opinion? RFR: 8171279: Support X25519 and X448 in TLS
Doerr, Martin
martin.doerr at sap.com
Mon Oct 12 09:54:08 UTC 2020
Hi Götz,
thanks for restoring the functionality. Your incremental change looks correct.
> The larger comment a bit down states that FIPS is unusable in 11.
I guess you would need a Java debugger / JVMTI agent to enable it with SunJSSE which is nothing we have to support ��
Note that FIPS 140 mode can be used by another provider which doesn't need your new code: https://github.com/elastic/elasticsearch/pull/48378
On the other hand, your new version reflects the status of JDK-8217835 correctly, which is not backported.
So I'm fine with either version.
Best regards,
Martin
> -----Original Message-----
> From: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
> Sent: Montag, 12. Oktober 2020 08:39
> 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
> Subject: RE: [11u] Second opinion? RFR: 8171279: Support X25519 and X448 in
> TLS
>
> 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
> > > >
> > > >
More information about the jdk-updates-dev
mailing list