RFR: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key [v2]

[Sean Mullan]

On Wed, 20 Oct 2021 02:45:07 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> Sean Mullan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   - Changed names of AlgorithmDecomposer.canonicalName and decomposeOneHashName
>>   methods.
>>   - Changed other code in AlgorithmDecomposer to use DECOMPOSED_DIGEST_NAMES
>>   Map instead of hardcoding algorithm names.
>>   - Changed AlgorithmChecker.trySetTrustAnchor to set trustedPubKey field so that
>>   constraints on the key algorithm and size are checked in the check() method if
>>   the constraints are an instanceof DisabledAlgorithmConstraints.
>
> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 106:
> 
>> 104:         // "SHA-256" and "SHA256" to make the right constraint checking.
>> 105: 
>> 106:         for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
> 
> If you're going to change this code, you can save me a PR if you surround this by "if (algorithm.contains("SHA") {  ...  }"
> Its a perf change to eliminate the unnecessary map lookups when SHA isn't in the algorithm string

That's a fine suggestion, although I'll note that your suggested perf improvement also applies to the previous code which did not check the algorithm parameter first to see if it contained `SHA`.
Also, another small perf imp: I realized below that in the loop, if the first `if` block gets executed, then the 2nd `if` block will always be false, so I changed it to an if/else.

> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 158:
> 
>> 156:         Set<String> elements = decomposeImpl(algorithm);
>> 157: 
>> 158:         for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
> 
> Same "if (algorithm.contains("SHA") { ... }" comment as above

Ok. Let's hope a new algorithm with "SHA" in the words never gets defined :)

> src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java line 159:
> 
>> 157: 
>> 158:         for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
>> 159:             hasLoop(elements, e.getKey(), e.getValue());
> 
> I think it's worth merging the contents of hasLoop() into this for loop.  This is the only method calling hasLoop() and the extra method calls are not useful.  Your addition DECOMPOSED_DIGEST_NAMES makes a merger a more reasonable solution now than before.

Ok. I noticed that this method is different than `decompose` in that it only adds the decomposed `SHA` name to `elements`, and removes the standard `SHA` name from `elements` if it exists, whereas `decompose` adds both. Do you think we should change `decompose` to use the same technique?

-------------

PR: https://git.openjdk.java.net/jdk/pull/5928