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

[Jamil Nimeh]

On 03/13/2015 08:43 AM, Sean Mullan wrote:
> 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
Yeah, I guess it is redundant, isn't it?  I can change that.  I probably 
should do the same with a bunch of my OCSP stapling unit tests too.
>
> - 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.
>
Force of habit.  Some of my other tests, particularly in the SSL arena, 
have debug output that can get very chatty, above and beyond what would 
be useful for normal log output.  For a test like this though, what you 
say makes sense.  I'll rip that out and just go with normal 
System.out.println output.
> - readFile should create the FileReader in a try-with-resources block 
> so it is closed.
Oh wow.  I should've caught that.
>
> - line 165 needs a 4-space indent
>
All these will be fixed up in a jiffy.

> --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
>>>>>>
>>>>>
>>>>
>>