RFR JDK-8134364: Add defensive copies to get/set methods for OCSPNonceExtension

[Jamil Nimeh]

Hi Sean, et al.,

I've updated the review to incorporate Sean's comments.  Sean, I was 
able to remove encodeInternal() and make it into a one-liner inside each 
of the ctors, so that shrunk things down a bit more which is nice.  Let 
me know what you think.

Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8134364/webrev.02/

Thanks,
--Jamil

On 09/01/2015 11:29 AM, Sean Mullan wrote:
> On 08/28/2015 09:25 PM, Jamil Nimeh wrote:
>> Hello all,
>>
>> I've removed the CertAttrSet interface from OCSPNonceExtension and
>> trimmed away a few unneeded methods.  As a result the class is immutable
>> now.
>
> Looks a lot cleaner. Strictly speaking, the cloning is not necessary 
> since this is an internal class -- unless the array can leak through 
> some code path to untrusted code, but I think in this case it is good 
> practice and probably won't affect performance much as nonces are a 
> rarely used feature (I think). A few comments (some stylistic based on 
> [1]) on OCSPNonceExtension:
>
> 48: make class final
>
> 49: constants are generally named upper case and with "_" as a word 
> separator, so EXTENSION_NAME is better.
>
> 58: move this private method after the constructors (or better yet, 
> embed it inside the ctor since it is only called from there)
>
> 84: not necessarily the SUN provider, depends on the configured 
> provider order
>
> 174: toString should probably use StringBuilder.append instead of "+".
>
> --Sean
>
> [1] review ongoing at 
> http://cr.openjdk.java.net/~alundblad/styleguide/feedback.html
>
>>
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8134364
>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8134364/webrev.01
>>
>> Thanks,
>> --Jamil
>>
>> On 08/25/2015 08:11 AM, Jamil Nimeh wrote:
>>> Hi Sean,
>>>
>>> Yes, I was just following Extension convention in terms of
>>> implementing CertAttrSet.  Within sun.security.x509, X509CertInfo uses
>>> the set methods from other objects implementing CertAttrSet. But
>>> OCSPNonceExtension really isn't a certificate attribute so it probably
>>> doesn't need to bring this interface in.
>>>
>>> I'll refactor this and alter the tests to match.
>>>
>>> --Jamil
>>>
>>> On 08/25/2015 06:40 AM, Sean Mullan wrote:
>>>> Is it necessary for the class to implement CertAttrSet? I realize all
>>>> of the internal X.509 extensions implement that, but that was done a
>>>> long time ago and not something we really want to carry forward if
>>>> not needed. I think it would be cleaner not to do that as it would
>>>> more easily allow you to make the class immutable, and would allow
>>>> you to remove the set method.
>>>>
>>>> Thanks,
>>>> Sean
>>>>
>>>> On 08/25/2015 04:14 AM, Xuelei Fan wrote:
>>>>> OCSPNonceExtension.java
>>>>> =======================
>>>>> -      nonceData = (byte[])obj;
>>>>> +      nonceData = ((byte[])obj).clone();
>>>>>
>>>>> Do you want to check null obj?
>>>>>
>>>>> -      return nonceData;
>>>>> +      return (nonceData != null ? nonceData.clone() : null);
>>>>>
>>>>> I think you may want to enclose the "!=" operator as:
>>>>>
>>>>> +      return (nonceData != null) ? nonceData.clone() : null;
>>>>>
>>>>>
>>>>> Xuelei
>>>>>
>>>>> On 8/25/2015 12:55 PM, Jamil Nimeh wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> This is a quick fix to the OCSPNonceExtension class to add a couple
>>>>>> defensive copies to public get/set methods.
>>>>>>
>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8134364
>>>>>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8134364/webrev.00
>>>>>>
>>>>>> Thanks,
>>>>>> --Jamil
>>>>>
>>>
>>