[8u] TLSv1.3 RFR: 8245473: OCSP stapling support

[Alexey Bakhtin]

Hello Martin,

Thank you for review.
Please see my comments below

Regards
Alexey

> On 5 Jun 2020, at 23:44, Martin Balao <mbalao at redhat.com> wrote:
> 
> On 5/21/20 11:02 AM, Alexey Bakhtin wrote:
>> Please review changes required to backport TLSv1.3 protocol from JDK11.0.7 to JDK8u
> 
> Hi Alexey,
> 
> A few questions and comments.
> 
> * test/*
> 
> Will tests be handled in later steps? I guess so but please confirm.
> This is probably not ideal but okay -we have managed them separately
> anyways in previous steps-.

Yes, OCSP stapling related test are added in JDK-8245681:
- test/java/security/testlibrary/CertificateBuilder.java
- test/java/security/testlibrary/SimpleOCSPServer.java
- test/javax/net/ssl/Stapling/*
- sun/security/provider/certpath/Extensions/OCSPNonceExtensionTests.java
- sun/security/provider/certpath/ResponderId/ResponderIdTests.java
- sun/security/ssl/Stapling/StatusResponseManagerTests.java

> 
> * SSLContextImpl.java
> 
> Why did you turn 'jdk.tls.client.enableStatusRequestExtension' to
> 'false' by default?
> 
> A CSR will be needed if we are introducing new properties anyways.
> 
OCSP Stapling should be disabled by default to minimise possible difference for
existing 8u applications.
You are right. It should be documented in CSR

> * X509TrustManagerImpl.java
> 
> You probably want to make some changes here after I asked you to remove
> changes introduced for Step 3 (8245470). However, before casting to
> SSLSessionImpl I'd suggest to add a 'instanceof' check. If, for any
> reason, it's a JDK-8 ExtendedSSLSession implementation class different
> than SSLSessionImpl, we shouldn't fail because having the
> getStatusResponses method was not part of the contract at that time.
> 
Agree. You are right. Will be fixed

> * OCSP.java
> 
> Why are these changes not included?

OCSP.java is identical in JDK8 and JDK11.
No changes required

> 
> * OCSPRequest.java
> 
OCSPRequest.java is identical in JDK8 and JDK11.
No changes required

> Why are these changes not included?
> 
> * OCSPResponse.java
> 
OCSPResponse.java is identical in JDK8 and JDK11.
No changes required

> Why are these changes not included?
> 
> * ResponderId.java

ResponderId.java  is identical in JDK8 and JDK11.
No changes required
> 
> Why are these changes not included?
> 
> * Validator.java
> 
> Seems to be including changes from 8154015. Why? Looks to me that
> 8154015 was introduced to JDK-8 and then backed out.
> 
Do you mean changes in comments ? Yes, I can revert them. Will be fixed
The rest changes should be fine and identical to JDK-8046321 changes.

> I suggest not to bump the copyright end date.
> 
> * PKIXExtensions.java
PKIXExtensions.java is identical in JDK8 and JDK11.
No changes required

> 
> Why are these changes not included?
> 
> * RevocationChecker.java
> 
> Why are we including changes from 8161973 here? Please propose 8161973
> as an independent backport.

It is important fix. Without this fix OCSP functionality works incorrectly and several test fails.
I’d like this fix is ported as part of this OCSP backport.
Otherwise we’ll ship TLS functionality with known bug
Possible solution - additional step for this fix in terms of JDK-8245466

> 
> Step 6 (8245473) is the JDK-8 backport of 8046321. Don't forget to add a
> reference to 8046321 in the commit message.
> 
> Thanks,
> Martin.-
>