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

[Martin Balao]

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.

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.

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

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

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

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