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

[Valerie Peng]

These two BASE ones are simply used to get rid of the hardcoded oid 
string code in keytool/Main.java.

I can remove them (in webrev.02) and maybe you can update 
keytool/Main.java later to use the right KnownOIDs enum for oid 
construction? There are a few places in keytool/Main.java which can be 
updated to use the enum KnownOIDs, but that's a bit far from the main 
purpose of this RFE.

Thanks,
Valerie
On 5/1/2020 6:16 AM, Weijun Wang wrote:
One more thing:

In KnownOIDs.java, I found these 2 lines:

     PKIX_KP_BASE("1.3.6.1.5.5.7.3."),
     PKIX_OCSP_BASE("1.3.6.1.5.5.7.48."),

IMHO, they should not belong here, at least, we should remove the dot at the 
end and make them real OIDs.

I was testing the ObjectIdentifier generation and notice these two.

Thanks,
Max


On May 1, 2020, at 5:45 PM, Weijun Wang <weijun.w...@oracle.com> 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.

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)".

SecurityProviderConstants.java
------------------------------

    public static List<String> alias(String ... aliases) {
        return Arrays.asList(aliases);
    }

Probably not necessary, you can simply call List.of(...) everywhere.

SunMSCAPI.java
--------------

Why not call getAliases() inside "new ProviderService" like in the other 
providers? Same in UCrypto.

AlgorithmId.java
----------------

algOID(String). You don't check "if (name.indexOf('.') != -1)" at the beginning anymore. 
Is there an algorithm name containing "."?

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).

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.

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?

Thanks,
Max


On Apr 30, 2020, at 6:59 AM, Valerie Peng <valerie.p...@oracle.com> wrote:

Here is the updated webrev 
http://cr.openjdk.java.net/~valeriep/8242151/webrev.01/