RFR: 8237962: give better error output for invalid OCSP response intervals in CertPathValidator checks

[Sean Mullan]

Looks good.

Thanks,
Sean

On 1/29/20 4:20 AM, Baesken, Matthias wrote:
> 
> Hi Sean, new webrev :
> 
> 
> http://cr.openjdk.java.net/~mbaesken/webrevs/8237962.1/
> 
> 
> Best Regards, Matthias
> 
> 
>>
>> Message: 2
>> Date: Tue, 28 Jan 2020 10:45:59 -0500
>> From: Sean Mullan <sean.mullan at oracle.com>
>> To: security-dev at openjdk.java.net
>> Subject: Re: RFR: 8237962: give better error output for invalid OCSP
>> 	response intervals in CertPathValidator checks
>> Message-ID: <d0509125-9322-a269-622f-552cd90f8232 at oracle.com>
>> Content-Type: text/plain; charset=windows-1252; format=flowed
>>
>> Hi Matthias,
>>
>> On 1/28/20 8:18 AM, Baesken, Matthias wrote:
>>> Hello, please review this small change .
>>>
>>> While working on
>>> https://bugs.openjdk.java.net/browse/JDK-8237869
>>> and
>>> https://bugs.openjdk.java.net/browse/JDK-8237888
>>> I noticed that the error output in case of invalid OCSP response
>>> intervals? could be improved a bit.
>>>
>>> Bug/webrev :
>>>> https://bugs.openjdk.java.net/browse/JDK-8237962
>>
>> This should be an Enhancement, and not a Bug. You should also add an
>> appropriate noreg label (maybe noreg-trivial).
>>
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8237962.0/
>>
>> Please try to keep the lines within 80 characters to be consistent with
>> the rest of the file.
>>
>> It seems you could combine lines 602 and 604. I would also tweak the
>> wording a bit, ex:
>>
>> debug.println("Checking validity of OCSP response on " +
>>                 new Date(now) + " with allowed interval between " +
>>                 nowMinusSkew + " and " + nowPlusSkew);
>>
>> Lines 615 through 625 should only be executed if debug is enabled. They
>> should be in an "if (debug != null)" block. Also, if we were going to
>> add this, I would probably restructure this entire block of code so you
>> avoid code duplication.
>>
>> But, I actually don't think these extra debugging statements are
>> necessary. There is enough information in lines 600-605 to know exactly
>> why the validity check failed. My general opinion on debugging logging
>> is to give enough information to help diagnose errors, but don't give
>> too much extraneous information.
>>
>> --Sean
>>
>