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

[Alexey Bakhtin]

Hello Martin,

Please find the updated version of the patch:
http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245473/webrev.v1/


In this version :
- Added instanceof verification in the X509TrustManagerImpl.java
- JDK-8154015 changes are excluded from the Validator.java
- RevocationChecker.java is reverted. It will be fixed as separate backport of JDK-8161973
  JDK-8247276 subtask is submitted
- commit message changed to “8046321: OCSP Stapling for TLS"

Regards
Alexey


> On 9 Jun 2020, at 19:07, Martin Balao <mbalao at redhat.com> wrote:
> 
> Hi Alexey,
> 
> My reply inline.
> 
> Thanks,
> Martin.-
> 
> 
> On 6/6/20 6:14 PM, Alexey Bakhtin wrote:
>> 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
>> 
> 
> Good, I'll analyze them later then.
> 
>>> 
>>> * 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
> 
> Hmm.. okay. I see your point but OCSP stapling on the server side is
> still important for performance reasons. We need to turn this 'on' at
> some point. Please keep that in your backlog.
> 
>> 
>>> * 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
> 
> Let me know if you have a new Webrev.
> 
>> 
>>> * OCSP.java
>>> 
>>> Why are these changes not included?
>> 
>> OCSP.java is identical in JDK8 and JDK11.
>> No changes required
>> 
> 
> Ah, ok. Seems like 8176536 introduced many of the changes needed for the
> files to be identical.
> 
>>> 
>>> * OCSPRequest.java
>>> 
>> OCSPRequest.java is identical in JDK8 and JDK11.
>> No changes required
> 
> Good.
> 
>> 
>>> Why are these changes not included?
>>> 
>>> * OCSPResponse.java
>>> 
>> OCSPResponse.java is identical in JDK8 and JDK11.
>> No changes required
> 
> Good.
> 
>> 
>>> Why are these changes not included?
>>> 
>>> * ResponderId.java
>> 
>> ResponderId.java  is identical in JDK8 and JDK11.
>> No changes required
> 
> Yes, only a typo fixed by 8190323. Not a big deal and it's not worth to
> backport it separately. If you want to fix it here, I'm okay too.
> 
>>> 
>>> 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.
>> 
> 
> All the differences visible when comparing 8245473 with 8046321 in
> Validator.java, unless there is a good reason to have them. Looks to me
> that there shouldn't be traces from 8154015, but let me know if I'm wrong.
> 
>>> I suggest not to bump the copyright end date.
>>> 
>>> * PKIXExtensions.java
>> PKIXExtensions.java is identical in JDK8 and JDK11.
>> No changes required
>> 
> 
> Good.
> 
>>> 
>>> * 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
> 
> Yes, I wish we could have a separate step for this fix. 8245473 should
> be referred in the commit message as the backport of 8046321, while
> there should be a separate commit for the backport of 8161973. Sorry
> about the overhead but this will make it easier to understand the
> changes in the future: instead of a single backport with both patches
> mixed we will have 2 separate -and almost straightforward- backports.
> 
> Note: it's very important to have a reference to 8046321 in the commit
> message so we can find it in the repo. That reference must be: "8046321:
> OCSP Stapling for TLS", as we do with all the backports.
>