On Tue, 19 Oct 2021 20:32:21 GMT, Sean Mullan <[email protected]> wrote:
>> This fix improves the exception message to better indicate when the key (and
>> not the signature algorithm) is restricted. This change also includes a few
>> other improvements:
>>
>> - The constraints checking in `AlgorithmChecker.check()` has been improved.
>> If the `AlgorithmConstraints` are an instance of
>> `DisabledAlgorithmConstraints`, the internal `permits` methods are always
>> called; otherwise the public `permits` methods are called. This makes the
>> code easier to understand, and fixes at least one case where duplicate
>> checks were being done.
>>
>> - The above change caused some of the exception messages to be slightly
>> different, so some tests that checked the error messages had to be updated
>> to reflect that.
>>
>> - AlgorithmDecomposer now stores the canonical algorithm names in a Map,
>> which fixed a bug where "RSASSA-PSS" was not being restricted properly.
>
> 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
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
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.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5928
