[jdk17u-dev] RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v2]

[Francisco Ferrari Bihurriet]

On Wed, 13 Nov 2024 18:18:31 GMT, Alexey Bakhtin <abakhtin at openjdk.org> wrote:

>> src/java.base/share/classes/sun/security/provider/certpath/OCSP.java line 72:
>> 
>>> 70: 
>>> 71:     private static final int DEFAULT_CONNECT_TIMEOUT = 15000;
>>> 72:     private static final int DEFAULT_READ_TIMEOUT = 15000;
>> 
>> Hi @alexeybakhtin and @GoeLin,
>> 
>> After e73f8c1a0301574a3248081a80142d7fbc9c8b74, `DEFAULT_READ_TIMEOUT` is now unused and can be removed.
>> 
>> Apparently, this backward-compatibility problem went undetected in the original change. Please also note that the behavior is no longer aligned with [the CSR](https://bugs.openjdk.org/browse/JDK-8337407 "JDK-8337407: Enhance OCSP, CRL and Certificate Fetch Timeouts") (emphasis by me):
>>> For __*all properties*__, existing and new, the proposed expanded syntax will conform to the following:
>>>
>>> - […]
>>> - As with the current behavior, non-numeric, non-decimal (e.g. hexadecimal values prepended by "0x", etc) values will be interpreted as illegal and __*will default to the 15 second timeout*__. The same is true for negative values.
>> 
>> `com.sun.security.ocsp.readtimeout` is now an exceptional case, it won't default to the 15 seconds timeout, but to `com.sun.security.ocsp.timeout` (which defaults to 15 seconds).
>> 
>> Should the CSR be adjusted? Otherwise, how do we ensure Oracle's backport behaves the same way? I don't have enough backporting experience, is it usually accepted to diverge from newer releases (≥ 21u) in a backport CSR?
>
> Hi @franferrax 
> Thank you for the finding. I'll remove  `DEFAULT_READ_TIMEOUT` 
> You are right, the CSR for update releases should be updated for `com.sun.security.ocsp.readtimeout` default value. I'll do it and ask you to review it from an engineering point of view. 
> I hope in this way, without backward compatibility, the patch will be accepted.

I will take a look, but please note I'm not a Reviewer.

One more comment, I see that the following issues are not part of the backport. Although all of them are test-only changes (mostly tuning the timeout), we might also want to include these to avoid some instability.

* [JDK-8309740](https://bugs.openjdk.org/browse/JDK-8309740 "Expand timeout windows for tests in JDK-8179502"), openjdk/jdk at 5ca4cdd2caceba9dad8025e5a8851740a3961921
* [JDK-8310629](https://bugs.openjdk.org/browse/JDK-8310629 "java/security/cert/CertPathValidator/OCSP/OCSPTimeout.java fails with RuntimeException  Server not ready"), openjdk/jdk at b20dc1e9cda1ea3a76b3f14c778c6816e5cc1c0c
* [JDK-8325024](https://bugs.openjdk.org/browse/JDK-8325024 "java/security/cert/CertPathValidator/OCSP/OCSPTimeout.java incorrect comment information"), openjdk/jdk at 432756b6e51c903e2bff8b9c3028a4f2ea8973f4
* [JDK-8337826](https://bugs.openjdk.org/browse/JDK-8337826 "Improve logging in OCSPTimeout and SimpleOCSPResponder to help diagnose JDK-8309754"), openjdk/jdk at 9b11bd7f4a511ddadf9f02e82aab6ba78beb6763

The last one is an ongoing effort to try to fix [JDK-8309754](https://bugs.openjdk.org/browse/JDK-8309754 "java/security/cert/CertPathValidator/OCSP/OCSPTimeout.java failed with "Expected to pass, found 1 soft fail exceptions""), so we could alternatively wait for that fix and then make all the 5 backports together.

-------------

PR Review Comment: https://git.openjdk.org/jdk17u-dev/pull/2747#discussion_r1841396469