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

[Martin Balao]

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.