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

Wed Oct 7 08:59:16 UTC 2020

Hi Goetz,

Not a reviewer, but as you call for opinion, I'll pitch in.

Isn't that FIPS option still accessible in jdk11 by passing something
like:

javac --add-exports java.base/com.sun.net.ssl.internal.ssl=ALL-UNNAMED
FipsProviderTest.java

I haven't tested the full FIPS functionality, 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. 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?

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