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

[Wang Weijun]

CRLExtensions.java
==================

 116             Class<?> extClass = ExtensionsMap.getClass(ext.getExtensionId());
 117             if (extClass == null) {   // Unsupported extension
 118                 if (ext.isCritical())
 119                     unsupportedCritExt = true;
 120                 if (map.put(ext.getId(), ext) != null)
 121                     throw new CRLException("Duplicate extensions not allowed");
 122                 return;
 123             }
 124             Constructor<?> cons = extClass.getConstructor(PARAMS);
 125             Object[] passed = new Object[] {Boolean.valueOf(ext.isCritical()),
 126                                             ext.getExtensionValue()};
 127             CertAttrSet<?> crlExt = (CertAttrSet<?>)cons.newInstance(passed);
 128             if (map.put(ext.getId(), (Extension)crlExt) != null) {
 129                 throw new CRLException("Duplicate extensions not allowed");
 130             }

I see no reason to convert cons.newInstance(passed) first to one type then another. Also, the 2 throw statements can be combined to one.

 195     public void set(String oid, Object obj) {
 196         map.put(oid, (Extension) obj);
 197     }

How about simply set(Object obj) or set(Extension ext)? You can always find oid from ext.getId(). This prevents unnoticed codes still using the name as keys.

I'd like to add a comment to the map field, something like

   For known extensions listed in ExtensionMap, the value must be of a child type of Extension (defined in ExtensionMap).

This is quite critical for the getExtension() methods in X509CRLImpl to work correctly.


CertificateExtensions.java
==========================

Same as above, but in the comment of map, there is an extra case

   Known extensions which is unparseable and not critical will be stored in an unparseableExtensions map.

Since we don't use names as keys, CertificateExtensions is not a typical CertAttrSet now. Remove the implements clause.


OtherName.java
==============

ExtensionMap will not support OtherName anymore. getGNI() should always return null. File a new RFE on how to implement OtherName in post-jdk9 era.


certAttributes.html
===================

 229 Extensions can be added by implementing the
 230 <code>sun.security.x509.CertAttrSet</code> interface and
 231 subclassing <code>sun.security.x509.Extension</code> class.
 232 Register the new extension using the ExtensionsMap class.

After jdk9, these interface/class will not be available outside jdk. File an enhancement on how to do this later.


ExtensionsMap.java
==================

 195     public static void addAttribute(String name, String oid, Class<?> clazz)
 196             throws CertificateException {

After jdk9, this method will not be available outside jdk. Mention this in the enhancement filed above.


Thanks
Max


> On Mar 28, 2015, at 22:45, Wang Weijun <weijun.wang at oracle.com> wrote:
> 
>> 
>> On Mar 28, 2015, at 05:19, Jason Uh <jason.uh at oracle.com> wrote:
>> 
>> 
>> 
>> On 03/27/2015 03:53 AM, Wang Weijun wrote:
>>> 
>>>> On Mar 27, 2015, at 06:37, Jason Uh <jason.uh at oracle.com> wrote:
>>>> 
>>>> Please review this revision:
>>>> http://cr.openjdk.java.net/~juh/7145757/01/
>>>> 
>>>> * a global nameCache is maintained in OIDMap as suggested
>>> 
>>> Can you just use the existing OIDMap.getId() method? It looks like your getCachedOid(name) is the same as getId("x509.info.extensions." + name).
>>> 
>>> In fact, since the OIDMap only contains mapping of extensions, I'd suggest renaming it to ExtensionMap and change the name inside to short names (without the "x509.info.extensions." prefix).
>> 
>> OK, thanks for that suggestion. I thought there was some significance to using the "full" name in OIDMap,
> 
> I think it was designed to be more powerful (see how OtherName uses it) but unfortunately it hasn't been so. Now that with jdk9/module it is no more accessible from outside the JDK, we can simplify it.
> 
>> but if that's not necessary, it makes things more flexible. Here is the latest revision that uses only the existing OIDMap (now called ExtensionsMap).
>> 
>> http://cr.openjdk.java.net/~juh/7145757/02/
> 
> Will read it.
> 
> Thanks
> Max
> 
>> 
>> Thanks,
>> Jason
>> 
>>> --Max