RFR: [Update 2] 8074064 : OCSPResponse.SingleResponse objects do not parse singleExtensions

[Sean Mullan]

On 03/11/2015 06:10 PM, Jamil Nimeh wrote:
> Okay, an updated webrev has been posted that addresses Sean's comments
> (thanks, BTW).
>
> http://cr.openjdk.java.net/~jnimeh/reviews/8074064/webrev.04/

Code changes look good, but I had not reviewed the test so here are a 
few more comments on that part.

- for the path, can you remove "Tests" from the directory name? 
Everything is a test so that seems redundant to me. So the new path 
would be test/sun/security/provider/certpath/OCSP

- Why do you need the DEBUG boolean property? When would you ever want 
to turn this off? Most of the debug info is valuable (like "Single 
Extension count mismatch ...") so when something fails you would always 
want to see it in the logs.

- readFile should create the FileReader in a try-with-resources block so 
it is closed.

- line 165 needs a 4-space indent

--Sean

>
> --Jamil
>
> On 03/11/2015 07:38 AM, Sean Mullan wrote:
>> Hi Jamil,
>>
>> Just a few comments, mostly minor.
>>
>> * OCSPResponse
>>
>> 768: there is an extra space in the indentation
>>
>> 811: use Collections.emptyMap instead of new HashMap<>() as that does
>> not allocate a new Map object.
>>
>> 813-15,861-63: Use Map.values instead of Map.keySet, ex:
>>
>>     for (Extension ext : singleExtensions.values()) {
>>         sb.append("singleExtension: " + ext + "\n");
>>     }
>>
>> 784-807: I think this code could be compressed a bit to avoid calling
>> parseExtensions twice. You can eliminate lines 786-92, if you do
>> something like this (I think), starting at line 784:
>>
>>     if (tmp.available() > 0) {
>>         derVal = tmp.getDerValue();
>>     } else {
>>         derVal == null;
>>     }
>>
>> then what is now line 794 becomes:
>>
>>     if (derVal != null && derVal.isContextSpecific((byte)1)) {
>>
>> --Sean
>>
>> On 03/04/2015 05:50 PM, Jamil Nimeh wrote:
>>> One more round of updates.  Only the test code and the test inputs have
>>> changed.  Test input is now Base64 format, with comment headers that
>>> display the OCSP response pretty-print (or an asn1parse of the
>>> BasicOCSPResponse for malformed response test cases).
>>>
>>> http://cr.openjdk.java.net/~jnimeh/reviews/8074064/webrev.03/index.html
>>>
>>> Thanks, Vinnie for the feedback and suggestions!
>>> --Jamil
>>>
>>> On 03/03/2015 04:10 PM, Jamil Nimeh wrote:
>>>> Okay, I've got an updated webrev for this issue:
>>>> http://cr.openjdk.java.net/~jnimeh/reviews/8074064/webrev.02/index.html
>>>>
>>>> Thanks,
>>>> --Jamil
>>>>
>>>> On 03/03/2015 02:18 PM, Jamil Nimeh wrote:
>>>>> Hello all, I've come across a couple edge cases that this fix doesn't
>>>>> cover properly.  I'll put out another webrev in a bit that should
>>>>> tighten up the singleResponse parsing, particularly with the optional
>>>>> fields. It will also include a couple other negative test input
>>>>> samples.
>>>>>
>>>>> Thanks,
>>>>> --Jamil
>>>>>
>>>>> On 03/02/2015 04:05 PM, Jamil Nimeh wrote:
>>>>>> Hello all, this review fixes an issue in OCSPResponse where it does
>>>>>> not parse singleExtensions in the SingleResponse structure.  I also
>>>>>> fixed two minor deviations from the OCSP.RevocationStatus interface
>>>>>> when getRevocationTime and getRevocationReason are called on good
>>>>>> responses.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8074064
>>>>>> Review:
>>>>>> http://cr.openjdk.java.net/~jnimeh/reviews/8074064/webrev.01/index.html
>>>>>>
>>>>>>
>>>>>> Thank you,
>>>>>> --Jamil
>>>>>
>>>>
>>>
>