Re: RFR[15] 8242060: Add revocation checking to jarsigner

[Weijun Wang]

> 
> * EnableRevocation.java
> 
> - How long does this test take - does it hang for a little while trying to 
> make a connection or timeout right away? If it takes a while, you could 
> experiment with overriding the default timeouts for CRLs and OCSP checks to 
> make this test finish faster. Use the system properties 
> com.sun.security.ocsp.timeout and com.sun.security.crl.readtimeout.

What if we use 0.0.0.0 for both OCSP and CRLDP? I assume it will return 
immediately, just hope it's not an uncaught RuntimeException.

--Max

> 
> Looks good otherwise. Please add a release-note and open a follow-on issue to 
> update the man page with the new option.
> 
> --Sean
> 
> On 5/1/20 12:02 PM, Hai-May Chao wrote:
>> Hi,
>> With small change added to ‘Usages.java' test, here is the updated webrev:
>> https://cr.openjdk.java.net/~hchao/8242060/webrev.01/
>> Thanks,
>> Hai-May
>>> On Apr 30, 2020, at 4:29 PM, Hai-May Chao  wrote:
>>> 
>>> Hi,
>>> 
>>> I’d like to request a review for:
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8242060
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8244046
>>> Webrev: https://cr.openjdk.java.net/~hchao/8242060/webrev.00/
>>> 
>>> The jarsigner command currently does certificate chain validation, but does 
>>> not check revocation. Users won’t be able to know if the certificates are 
>>> revoked. This change is to provide an option in jarsigner to enable the 
>>> revocation check, and to emit progress messages when jarsigner starts 
>>> network connections to get OCSP responses and CRL.
>>> 
>>> Thanks,
>>> Hai-May
>>> 
>>> 
>>> 

Re: [RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)

[Bradford Wetmore]

For keysize in things like KeyPairGenerator, why are we using 255/448 
(externally and internally) instead of 256/456?  From RFC 8032:


 section 3.2:  "An EdDSA private key is a b-bit string k"
+
 section 5.1.5/Ed25519:  "The private key is 32 octets (256 bits,
 corresponding to b) of cryptographically secure random data..."
 section 5.2.5/Ed448:  "The private key is 57 octets (456 bits,
 corresponding to b) of cryptographically secure random data..."

and

 section 3, parameter 2: "EdDSA public keys have exactly b bits"
and
 section 5.1:  "(parameter) b: 256"
 section 5.2:  "(parameter) b: 456"

The Wikipedia entry also lists as 256.  
https://en.wikipedia.org/wiki/EdDSA


I notice BC does actually use both.

https://github.com/bcgit/bc-java/blob/master/prov/src/test/jdk1.4/org/bouncycastle/jce/provider/test/EdECTest.java#L365 

https://github.com/bcgit/bc-java/blob/master/prov/src/test/jdk1.4/org/bouncycastle/jce/provider/test/EdECTest.java#L371 



I have no answer why 255 and 448 are stored, but changing them breaks 
the underlying implementation.  If a problem occurs where we need 256 & 
456, it can be addressed in a different way.


Tony and I have talked over this offline, and we've decided we'll follow 
the sizings of what was done for XDH (x25519/x448 Key Exchange) which is 
255/448.  Further comments captured here for posterity:


---begin---
I wasn't talking changing about the internal representation, my question 
was more about whether the "keysize" we present to things like 
KeyPairGenerator should be the actual keysize in storage bits, or the 
"keysize" based on the p value. i.e.


KeyPairGenerator kpg = KeyPairGenerator.getInstance("EdDSA");
kpg.initialize(XXX);

Right now, if I call kgp.initialize(256), it fails with an j.s.IPE. 
Only 255 and 448 are allowed.


My initial thought was that it should be the keysize representation, 
which would be 32/57 bytes (256/456 bits), but I can see the other way 
since the curve is over the fields 2^255 - 19 and 2^448 - 2^224 - 1, I 
am not sure what should be going here.


KeyPairGenerator

All key pair generators share the concepts of a keysize...The
keysize is interpreted differently for different algorithms (e.g.,
in the case of the DSA algorithm, the keysize corresponds to the
length of the modulus).


void KeypairGenerator.initalize(int keysize);

Initializes the key pair generator for a certain keysize

keysize - the keysize. This is an algorithm-specific metric,
such as modulus length, specified in number of bits.

---end---

Why did you decide to not use the full algorithm names for the 
variants e.g.
Ed25519ctx/Ed25519ph/Ed448ph instead of just Ed25519/Ed448 and let the 
selection be done using the Parameters.  More for my understanding 
than an issue.


The variants are not the normal case it would seem.  Even the spec hints 
at this.  I suspect Adam didn't want to create all these algorithm names 
just for a few variations.  I can understand the logic.  If a parameter 
spec can address them, why not.  The base is Ed25519/Ed448.


I can buy that.  It's made pretty clear in the EdDSAParameterSpec how to 
set.


Also, I thing you and Max might have discussed EdDSA vs EDDSA (case), 
but in a different context.  RFC 8410/Section 8, the full names are 
"EdDSA"/"Ed25519"/"Ed448",

but you're using "EDDSA"/"ED25519"/"ED448".
There is precedence for upper/lower case in our Java Standard Names.
https://docs.oracle.com/en/java/javase/14/docs/specs/security/standard-names.html 
, so just wondering why you're standardizing on the upper case variant?


That is the plan.  The reviews you were looking at hadn't fixed that 
entirely.


Tony/I talked further, he is going to go with the RFC naming style: 
EdDSA/etc.



src/java.base/share/classes/java/security/spec/NamedParameterSpec.java

94: This never seems to be called in our main test suite, so not sure 
why this is here.  Please add a comment as to why you added this new 
no-args constructor.  (was it some Class.newInstance() 
somewhere/prevent unexpected object creation/some kind of reflection?)


Minor nit, you could put this constructor above the other constructor,
it gets kind of lost here visually.


It may have been created when I was trying different ideas for the CSR 
for potentially subclassing the EdDSA algorithm parameter spec.  I 
deleted it


Thanks, I was scratching my head!  ;)


src/java.base/share/classes/sun/security/pkcs/PKCS7.java

832:  Seems like the right thing to do for making things bulletproof.
You're convinced this shouldn't happen in our code somewhere?  (i.e. 
unexpected new failure?)


The method in question reads the algorithm string searching for "with". 
There is no "with" here in the algorithm string. The digest is internal 
part of the algorithm and not interchangeable like the other Signatures. 
  Finall

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

[Valerie Peng]

Hmm, I took a shot at keytool/Main.java and used 
KnownOIDs.findMatch(...) to construct the oid.


They will be included in webrev.02.

Thanks,
Valerie
On 5/1/2020 3:29 PM, Valerie Peng wrote:


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  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 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  
wrote:


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

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  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 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  wrote:

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

Re: RFR[15] 8242060: Add revocation checking to jarsigner

[Sean Mullan]

* Main.java:

2067 Event.setReportListener(new Event.Reporter() {
2068 @Override
2069 public void handle(String t, Object... o) {
2070 
System.out.println(String.format(rb.getString(t), o));

2071 }
2072 });

I think you could use a lambda expression above.

* Event.java:

  35 static Reporter reporter = null;

Make this private? Also, no need to explicitly initialize to null as 
that is the default value.


Can you add some comments at the top of the class describing the purpose 
of this class?


* EnableRevocation.java

- How long does this test take - does it hang for a little while trying 
to make a connection or timeout right away? If it takes a while, you 
could experiment with overriding the default timeouts for CRLs and OCSP 
checks to make this test finish faster. Use the system properties 
com.sun.security.ocsp.timeout and com.sun.security.crl.readtimeout.


Looks good otherwise. Please add a release-note and open a follow-on 
issue to update the man page with the new option.


--Sean

On 5/1/20 12:02 PM, Hai-May Chao wrote:

Hi,

With small change added to ‘Usages.java' test, here is the updated webrev:

https://cr.openjdk.java.net/~hchao/8242060/webrev.01/

Thanks,
Hai-May


On Apr 30, 2020, at 4:29 PM, Hai-May Chao  wrote:

Hi,

I’d like to request a review for:

JBS: https://bugs.openjdk.java.net/browse/JDK-8242060
CSR: https://bugs.openjdk.java.net/browse/JDK-8244046
Webrev: https://cr.openjdk.java.net/~hchao/8242060/webrev.00/

The jarsigner command currently does certificate chain validation, but does not 
check revocation. Users won’t be able to know if the certificates are revoked. 
This change is to provide an option in jarsigner to enable the revocation 
check, and to emit progress messages when jarsigner starts network connections 
to get OCSP responses and CRL.

Thanks,
Hai-May

Re: [RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)

[Anthony Scarpino]

On 4/28/20 5:58 PM, Bradford Wetmore wrote:

Hi Tony,

Apologies for the delay.

 > I updated the webrev with some minor updates that were commented
 > previously.
 >
 > https://cr.openjdk.java.net/~ascarpino/8166597/webrev.01/

I've finished the APIs and a fair chunk of the implementation, but had 
some initial comments.


Overall, the APIs look reasonable and I (or whoever) should have no 
problems adding TLS support for EdDSA once this is in place.


     https://bugs.openjdk.java.net/browse/JDK-8166596

BTW, I reviewed based off the webrev.01 version (above), but I found in 
a later email exchange there was mention of a .04:


 > https://cr.openjdk.java.net/~ascarpino/8166597/webrev.04/

dated April 1.  I'm hoping that was just a proposed change idea, and not 
a new version to review.  Please advise if so.


In my comments below, if I say "you" below, it's the colloquial "you."
Could be you or the previous engineer.

If some of these questions have already been answered, I'll apologize in 
advance. I'm getting up to speed in RFC 8032 specifics so forgive any 
"duh" questions.




Common:  Throughout the API's, I noticed multiple minor javadoc
style issues, such as:

     . periods inconsistencies (missing or extras) (e.g.  EdECPoint)
     . omissions of @code (e.g. EdDSAParameterSpec/EdECPoint/etc.)

Suggest a javadoc sweep before final CSR.  I started calling them out, 
but stopped.


For keysize in things like KeyPairGenerator, why are we using 255/448 
(externally and internally) instead of 256/456?  From RFC 8032:


     section 3.2:  "An EdDSA private key is a b-bit string k"
+
     section 5.1.5/Ed25519:  "The private key is 32 octets (256 bits,
     corresponding to b) of cryptographically secure random data..."
     section 5.2.5/Ed448:  "The private key is 57 octets (456 bits,
     corresponding to b) of cryptographically secure random data..."

and

     section 3, parameter 2: "EdDSA public keys have exactly b bits"
and
     section 5.1:  "(parameter) b: 256"
     section 5.2:  "(parameter) b: 456"

The Wikipedia entry also lists as 256.  https://en.wikipedia.org/wiki/EdDSA

I notice BC does actually use both.

https://github.com/bcgit/bc-java/blob/master/prov/src/test/jdk1.4/org/bouncycastle/jce/provider/test/EdECTest.java#L365 

https://github.com/bcgit/bc-java/blob/master/prov/src/test/jdk1.4/org/bouncycastle/jce/provider/test/EdECTest.java#L371 


I have no answer why 255 and 448 are stored, but changing them breaks 
the underlying implementation.  If a problem occurs where we need 256 & 
456, it can be addressed in a different way.





Why did you decide to not use the full algorithm names for the variants 
e.g.
Ed25519ctx/Ed25519ph/Ed448ph instead of just Ed25519/Ed448 and let the 
selection be done using the Parameters.  More for my understanding than 
an issue.


The variants are not the normal case it would seem.  Even the spec hints 
at this.  I suspect Adam didn't want to create all these algorithm names 
just for a few variations.  I can understand the logic.  If a parameter 
spec can address them, why not.  The base is Ed25519/Ed448.




Also, I thing you and Max might have discussed EdDSA vs EDDSA (case), 
but in a different context.  RFC 8410/Section 8, the full names are 
"EdDSA"/"Ed25519"/"Ed448",

but you're using "EDDSA"/"ED25519"/"ED448".
There is precedence for upper/lower case in our Java Standard Names.
https://docs.oracle.com/en/java/javase/14/docs/specs/security/standard-names.html 
, so just wondering why you're standardizing on the upper case variant?


That is the plan.  The reviews you were looking at hadn't fixed that 
entirely.




Is Ed448ph working?  I've been playing with your test vectors from RFC 
8032 in test/jdk/sun/security/ec/ed/TestEdDSA.java, where you included 
Ed25519/Ed25519ctx/Ed25519ph, but omitted several (6 of 9) of the Ed448 
tests and all of the Ed448ph tests. I tried very quickly to add the two 
Ed448ph tests in in section 7.5, but no luck.  Could be pilot error, but 
I tried a lot of different combos (with/without ph boolean, null array 
vs. empty array, etc), but seems like it should have been 
straightforward given the test framework.


See comments below




Specific file comments:

make/jdk/src/classes/build/tools/intpoly/FieldGen.java

Lines 643-650 are creating a bunch of comment like this which are
the BigInteger values used later:


//(0%nat,16110573)::(26%nat,10012311)::(52%nat,30238081)::(78%nat,-8746018)::(104%nat,1367802)::nil. 



but what is the format of comment expressing?  What ae %nat/nil/::/etc.


I do not know.  That file is used to create the Interpolynomial files. 
It's probably a comment that explains what is created using the constant 
time code.  I have not had to run this file.




src/java.base/share/classes/java/security/spec/NamedParameterSpec.java

94: This never seems to be called in our main test suite, so not sure 

Re: RFR: 8225069: Remove Comodo root certificate that is expiring in May 2020

[Sean Mullan]

Looks good.

--Sean

On 5/1/20 1:53 PM, Rajan Halade wrote:
Please review this fix to remove expiring Comodo root CA “AddTrust Class 
1 CA Root”. Other two, AddTrust Qualified CA Root and AddTrust External 
CA Root, will remain since there are code signing certificates issued 
from those.


Webrev: http://cr.openjdk.java.net/~rhalade/8225069/webrev.00/

Thanks,
Rajan

RFR: 8225069: Remove Comodo root certificate that is expiring in May 2020

[Rajan Halade]

Please review this fix to remove expiring Comodo root CA “AddTrust Class 1 CA 
Root”. Other two, AddTrust Qualified CA Root and AddTrust External CA Root, 
will remain since there are code signing certificates issued from those.

Webrev: http://cr.openjdk.java.net/~rhalade/8225069/webrev.00/ 


Thanks,
Rajan

Re: RFR[15] 8242060: Add revocation checking to jarsigner

[Hai-May Chao]

Hi,

With small change added to ‘Usages.java' test, here is the updated webrev:

https://cr.openjdk.java.net/~hchao/8242060/webrev.01/

Thanks,
Hai-May

> On Apr 30, 2020, at 4:29 PM, Hai-May Chao  wrote:
> 
> Hi,
> 
> I’d like to request a review for:
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8242060
> CSR: https://bugs.openjdk.java.net/browse/JDK-8244046
> Webrev: https://cr.openjdk.java.net/~hchao/8242060/webrev.00/
> 
> The jarsigner command currently does certificate chain validation, but does 
> not check revocation. Users won’t be able to know if the certificates are 
> revoked. This change is to provide an option in jarsigner to enable the 
> revocation check, and to emit progress messages when jarsigner starts network 
> connections to get OCSP responses and CRL.
> 
> Thanks,
> Hai-May
> 
> 
> 

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

[Weijun Wang]

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  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 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  wrote:
>> 
>> Here is the updated webrev 
>> http://cr.openjdk.java.net/~valeriep/8242151/webrev.01/
> 

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

[Weijun Wang]

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 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  wrote:
> 
> Here is the updated webrev 
> http://cr.openjdk.java.net/~valeriep/8242151/webrev.01/