[9] RFR: 7145757: CertificateExtensions uses non-consistent key names

[Jason Uh]

Please review this revision:
http://cr.openjdk.java.net/~juh/7145757/01/

* all get()/set() calls to CertificateExtensions and CRLExtensions
   is by OID
* get()/set() from X509CertImpl/Info continue to work by name.
   ex: X509CertImpl.get("x509.info.extensions.<name>")
* a global nameCache is maintained in OIDMap as suggested
* no IOException from CRLExtensions
* fixed a bug on line 753 of X509CertImpl to allow extension deletion

Thanks,
Jason

On 03/18/2015 06:06 PM, Wang Weijun wrote:
> Yes, it will be extensive, but since we already make all keys OID, we should always call those set/get() with OIDs for performance. In case we might miss some, inside the implementation of get/set(), we can still accept names.
>
> Also, it seems there is no need for a nameCache everywhere, you can probably maintain a global one inside OIDMap.
>
> As for the IOException, I think it's because CRLExtensions does not have getExtension() that returns null. Now that you create another inconsistence that X509CRLImpl.getExtension() is starting to throw IOE while X509CertImpl.getExtension() does not.
>
> --Max
>
>> On Mar 19, 2015, at 07:40, Jason Uh <jason.uh at oracle.com> wrote:
>>
>> Hi Max,
>>
>> On 03/18/2015 04:09 PM, Wang Weijun wrote:
>>> Hi Jason
>>>
>>> I was thinking about changing all set/get calls to using OID so inside CertificateExtensions and CRLExtensions you won't need to care about name conversions. Is that possible?
>>
>> It might be, but it'd call for more extensive changes. Currently, when certificates are parsed, the extensions are added using the name, and when they are retrieved, they are also specified by name -- for example, X509CertImpl.getBasicConstraints() calls a get() method with the BasicConstraints alias as the lookup parameter.
>>
>> It's been a while, but the last we spoke about this, I think you wanted to keep the changes for this contained within CertificateExtensions and CRLExtensions. If that's still the case, we'll have to do this name conversion. If not, I could try changing things outside of these classes.
>>
>>> Also I see you adding some IOException throwing and catching. Is there any case you actually see them? I would think if that really happen there might be some programming error.
>>
>> I added the IOExceptions mainly to keep CRLExtensions consistent with CertificateExtensions. I thought it was a little odd that they weren't being thrown, but they may not need to be there for CRLExtensions. I can take them out.
>>
>>> --Max
>>>
>>>> On Mar 18, 2015, at 09:34, Jason Uh <jason.uh at oracle.com> wrote:
>>>>
>>>> Please review this fix, which changes the internal map in sun.security.x509.CertificateExtensions and CRLExtensions to always use the OID as the key.
>>>>
>>>> As stated in the JBS issue:
>>>>> The sun.security.x509.CertificateExtensions class maintains a Map<String,Extension> map field to store all the extensions it manages. The key of map uses the name (say, "BasicConstraints") of the extension, or the OID when the type of the extension is unknown.
>>>>
>>>> With this change, the map will consistently use the OID as the key.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~juh/7145757/00/
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-7145757
>>>>
>>>> Thanks,
>>>> Jason
>>>
>