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

[Alexey Bakhtin]

Hi Martin,

Thank you very much for review and comments.
Please see my comments below.

Updated webrev : http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245470/webrev.v1/

Regards
Alexey

> On 29 May 2020, at 21:41, Martin Balao <mbalao at redhat.com> wrote:
> 
> On 5/21/20 10:33 AM, Alexey Bakhtin wrote:
>> Please review changes required to backport TLSv1.3 protocol from JDK11.0.7 to JDK8u
>> 
> 
> Hi,
> 
> 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/

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

> 
> * HandshakeHash.java
> 
> This does not look good to me because we are loosing functionality that
> was available in JDK-8 and is available in JDK-11. In particular, for
> P11 sensitive keys we cannot extract the key bytes in plain (keyBytes
> will be null indeed), but we can still use PKCS#11 primitives to get the
> key digest [1]. In JDK-8, this was available here [2] [3]. Have you
> considered using the JDK-11 MessageDigestSpi2 + Delegate approach?
> Shouldn't involve any new public API [4].

Thank you
You are absolutely right. I have to use JDK8 implementation of digestKey
Fixed

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

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

> 
> * SSLSessionContextImpl.java
> 
> Can't we use GetPropertyAction::privilegedGetProperty?

I have updated it as recommended in the JDK8 GetIntegerAction class

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

> Thanks,
> Martin.-
> 
> --
> [1] -
> http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/c0dd958bb895/src/share/classes/sun/security/pkcs11/P11Digest.java#l236
> [2] -
> http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/c0dd958bb895/src/share/classes/sun/security/ssl/HandshakeMessage.java#l1738
> [3] -
> http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/c0dd958bb895/src/share/classes/sun/security/ssl/HandshakeMessage.java#l1784
> [4] - https://bugs.openjdk.java.net/browse/JDK-8165275
> [5] - http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/6c96af8a34b2#l12.1
> 

[1] - https://bugs.openjdk.java.net/browse/JDK-8046321