[15] RFR JDK-8242151 Improve OID mapping and reuse among JDK security providers for aliases registration

[Valerie Peng]

Hi Max,

Thanks for the comments, they are very helpful. I should have webrev.02 
ready in a bit. Please find comments below in line.

On 5/1/2020 2:45 AM, Weijun Wang wrote:
> ObjectIdentifier.java
> ---------------------
>
> Have you thought about storing the ObjectIdentifier object somewhere? ObjectIdentifier.of() creates a new object each time and the conversion of string to byte[] might be a performance issue. We used to have a lot of ObjectIdentifier objects in AlgorithmId but now we only have KnownOIDs.

The ObjectIdentifier objects are stored inside the oidTable map in 
AlgorithmId class. I removed the static oid constants which aren't 
referenced. The oids are retrieved from the oidTable when needed. With 
the webrev, we are exchange oid construction with a map look up, at 
least this is the intention.

> I had a talk with Stuart and he has a suggestion that we can stuff all pre-calculated OID DER encodings in a long byte array in a resource file, and in KnownOIDs each element has an offset/length pair that point to its DER encoding. Also, whatever cache mechanism we use in the future, I suggest making "new ObjectIdentifier(String)" private and keep "ObjectIdentifier of(String)".
Done in webrev.02.
>
> SecurityProviderConstants.java
> ------------------------------
>
>      public static List<String> alias(String ... aliases) {
>          return Arrays.asList(aliases);
>      }
>
> Probably not necessary, you can simply call List.of(...) everywhere.
Done in webrev.02.
>
> SunMSCAPI.java
> --------------
>
> Why not call getAliases() inside "new ProviderService" like in the other providers? Same in UCrypto.
Ok, done in webrev.02.
>
> AlgorithmId.java
> ----------------
>
> algOID(String). You don't check "if (name.indexOf('.') != -1)" at the beginning anymore. Is there an algorithm name containing "."?
Nope, it's more because the KnownOIDs enum can handle oid string lookup, 
so I just need to remove the "OID." prefix if present.
> It's a pity we have to collect OIDs from other providers. Maybe it should only be necessary when we use that provider, for example, when encoding a signature, we should ask the provider about the OID. I wish there were a Signature::getAlgorithmId, but if not, maybe we can rename Algorithm::alfOID(name) to Algorithm::alfOID(name, provider).

The existing OID collection code in AlgorithmId class serves as a safety 
net when new OID alias mapping is added to provider but AlgorithmId 
class is not also updated. After this KnownOIDs change, this safety net 
may not be that necessary and for performance reason, the code would 
skip JDK providers. However, for max backward compatibility, we may have 
to keep it. I agree with your name->oid mapping can be provider-specific 
and handling that would be somewhat sticky with current code. As for 
including an extra provider argument, I am not sure if that'd work 
across the board as the caller of AlgorithmId class may not have the 
provider info. OID usage can be somewhat complicated as demonstrated by 
this change.

> Do you know a bad case if we don't collect those OIDs? It must be some algorithm that is not in the Standard Names.
With existing regression tests, nothing breaks if we don't collect the 
OID aliases.
> Overall I think the change looks great, and we have a single place to store all OIDs. The mapping among the OID string, KnownOIDs, and ObjectIdentifier could be enhanced. Do you have a benchmark?

I have not done one. I can try something out once I finish addressing 
your other comments.

Thanks,
Valerie
>
> Thanks,
> Max
>
>
>> On Apr 30, 2020, at 6:59 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
>>
>> Here is the updated webrev http://cr.openjdk.java.net/~valeriep/8242151/webrev.01/