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

[Anthony Scarpino]

On Tue, 19 Oct 2021 20:32:21 GMT, Sean Mullan <mullan at openjdk.org> 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