[8u] TLSv1.3 RFR: 8245470: Fix JDK8 compatibility issues

Tue Jun 2 09:13:14 UTC 2020

Hello Martin,

Thank you for review.
Updated webrev : http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245470/webrev.v2/

Please see comments inline

Regards
Alexey

> On 1 Jun 2020, at 22:59, Martin Balao <mbalao at redhat.com> wrote:
> 
> Hi Alexey,
> 
> Thanks for having a look at this.
> 
> I've seen that v1 removes methods from
> src/share/classes/sun/security/ssl/BaseSSLSocketImpl.java, which were
> not removed by v0. Was this by mistake? Otherwise, please let me know of
> additional change between revisions because I may overlook something.
> 
This class were changed as part of JDK-8245469 [1]
Later, after discussion in [2] I’ve moved modification to JDK-8245470 [3]
This is a reason BaseSSLSocketImpl was not changed in the v0 and changed in v1

> My other comments inline.
> 
> Regards,
> Martin.-
> 
> On 5/30/20 4:40 PM, Alexey Bakhtin wrote:
>>> A few questions and comments below:
>>> 
>>> * I found several compilation errors. Isn't Step 3 supposed to be the
>>> one that makes the code compile in JDK-8? In case it's not, I wish you
>>> could clarify what are the expectations for Step 3 and where are will
>>> these errors be addressed.
>> 
>> It was not supposed to make all classes compilable on this step
>> A lot of classes will be changed in subsequent steps and I did not want to make double work:
>> - Fix compilation errors in methods that will be changed in the next steps
>> - Double task for reviewer : review changes that will be excluded later
>> The name of the step could be misleading but at this step I just fixed general compilation errors.
>> The known issues that fixed in later steps:
>> - Finished.java
>>  Fixed in JDK-8245471: http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245471/webrev.v0/
>> - KerberosClientKeyExchange.java krb5/KerberosClientKeyExchangeImpl.java and krb5/KerberosPreMasterSecret.java
>>  Fixed in JDK-8245474 : http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245474/webrev.v0/
>> - X509TrustManagerImpl.java
>>  Fixed in JDK-8245473 : http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245473/webrev.v0/
>>         and JDK-8245472 : http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245472/webrev.v0/
>> 
> 
> Thanks for your clarification. Yes, makes sense to me. I'll stick to the
> actual change more than to what be missing. Compilation will not be an
> expectation for this intermediate step then (as it was not for the
> previous steps).
> 
>>> 
>>> * HandshakeContext.java
>>> 
>>> Why is casting needed in JDK-8 and not needed in JDK-11? (I was going to
>>> check this myself compiling but found several compilation errors, and I
>>> do not realize of the difference looking at the ByteBuffer API only). I
>>> guess this question applies to SSLCipher.java, SSLSocketInputRecord.java
>>> and SSLEngineInputRecord.java too.
>> 
>> Actually, I do not know Why JDK11 allows such downcasting without explicit cast
>> 
> 
> Aha, found the mystery!
> 
> JDK-11:
>      22: invokevirtual #4                  // Method
> java/nio/ByteBuffer.rewind:()Ljava/nio/ByteBuffer;
> 
> JDK-8:
>      22: invokevirtual #4                  // Method
> java/nio/ByteBuffer.rewind:()Ljava/nio/Buffer;
>      25: checkcast     #5                  // class java/nio/ByteBuffer
> 
> In JDK-11, ByteBuffer::rewind overrides rewind with a compatible return
> type: ByteBuffer. JDK-8 declares Buffer::rewind to be final and the
> return type is Buffer, so there is not an override.
> 
> I'm okay with your cast because we know that 'fragment' is of type
> ByteBuffer and rewind won't change that -in fact, it returns 'this'-. I
> do not expect a runtime failure here and there shouldn't be a noticeable
> performance impact either.
> 
O, Thank you.
It is interesting. I’ve missed changes in final declaration between JDK8 and JDK11

>>> 
>>> * HandshakeHash.java
>> Thank you
>> You are absolutely right. I have to use JDK8 implementation of digestKey
>> Fixed
> 
> Can we apply JDK-11's approach with MessageDigestSpi2?

OK. I did not apply JDK11 approach because of changes outside of sun.security.ssl
Also, this code is for SSLv3.0 only and marked as subject to remove in the future.
But OK. I’ve backported JDK-8165275 [4], so it is similar to JDK11 now. It’ll make easy
to backport TLS and PKCS11 functionality
This backport adds changes in classes:
java/security/MessageDigest.java - changed
sun/security/pkcs11/P11Digest.java - changed
sun/security/util/MessageDigestSpi2.java - added
sun/security/ssl/HandshakeHash.java - no changes, identical to JDK11

> 
>>> * JsseJce.java
>>> 
>>> Have you considered backporting SecurityConstants.java part of 8130181
>>> [5] and use PROVIDER_VER in JsseJce.java
>>> 
>> Thank you. Fixed.
>> PROVIDER_VER is added to SecurityConstants.
>> SunJSSE.jave is also updated
> 
> Can't we use JDK-11's approach with
> "GetPropertyAction.privilegedGetProperty("java.specification.version");"? I
> guess the string is not exactly the same and you want to keep JDK-8
> compatibility here. But please confirm the reason so we leave a record.
There are several reasons I do not like JDK11 approach here:
1. JDK11 and later have the same code base, so PROVIDER_VER from system property
 has clear advantage - correct value for every JDK11+ version.
It is not a case for JDK8.
Provider class in JD8 does not allow version as String, so we have to convert it to double.
 It means we still have different code in JDK8 and JDK11+
2. JDK8 code base will not be used for later JDK versions, so no reasons to create
 PROVIDER_VER from system property
3. Additional unnecessary code to read system property, and convert it to double

> 
>> 
>>> * SSLLogger.java
>>> 
>>> Can't we leverage on PlatformLogger values directly? For each value,
>>> there should be a corresponding one. In case there is a good reason why
>>> not, we might want to think of moving the code to a more reusable place.
>> 
>> I have updated it to use java.util.logging.Logger
>> Internal Level enum is changed to java.util.logging.Level
>> Also, I’ve made possible to register app Logger. It was not possible in the first version.
> 
> Looks good to me. Well done! Remind me of asking for log examples at the
> end of this series :-)
> 
>>> 
>>> * RandomCookie.java / RenegoInfoExtension.java
>>> 
>>> Have you analyzed the performance implication of not having vectorized /
>>> intrinsics for the comparison?
>> 
>> No, I did not analyzed it. I understand possible performance impact but it could be
>> optimized later.
>> 
> 
> Right. Given the effort and risk involved in the 'vectorized' backport,
> I won't suggest it now. However, we should keep an eye on this and if
> any performance downgrade is noticed, here we have something to look at.
> The RandomCookie use is for a few bytes only, so it shouldn't be a big
> deal. The Renegotiation Extension does not look like a big deal either,
> at first glance at least.
> 
>>> 
>>> * SSLSessionContextImpl.java
>>> 
>>> Can't we use GetPropertyAction::privilegedGetProperty?
>> 
>> I have updated it as recommended in the JDK8 GetIntegerAction class
>> 
> 
> Even though the documentation gives an example with an Integer cast and
> an intValue call, that shouldn't be necessary.

Thank you. Updated.

> 
>>> 
>>> * X509TrustManagerImpl.java
>>> 
>>> Why is this change needed?
>>> 
>> ExtendedSSLSession does not contain getStatusResponses() public API method.
>> This method is part of JEP-249 : OCSP Stapling for TLS [1]
>> It is possible to implement and use OCSP Stapling without public API.
>> However, these changes should be moved to the step responsible for
>> OCSP stapling support - JDK-8245473
>> 
>> Fixed
> 
> Good.
> 
> 

[1] - http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245469/webrev.v0/src/share/classes/sun/security/ssl/BaseSSLSocketImpl.java.udiff.html
[2] - https://mail.openjdk.java.net/pipermail/jdk8u-dev/2020-May/011799.html
[3] - http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245470/webrev.v1/src/share/classes/sun/security/ssl/BaseSSLSocketImpl.java.udiff.html
[4] - https://bugs.openjdk.java.net/browse/JDK-8165275