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.mul...@oracle.com>
To: security-dev@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-552cd90f8...@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
