Re: [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...@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...@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