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

[Weijun Wang]

On Wed, 20 Oct 2021 14:47:31 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 decomposed SHA 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:
> 
>   - Skip digest alg decomposing check for algorithms that don't contain "SHA".
>   - Remove hasLoop method and fold code into decomposeName method.

src/java.base/share/classes/sun/security/provider/certpath/AlgorithmChecker.java line 80:

> 78:     private final Date date;
> 79:     private PublicKey prevPubKey;
> 80:     private final String variant;

You can group fields to final and non-final ones.

src/java.base/share/classes/sun/security/provider/certpath/AlgorithmChecker.java line 174:

> 172:     }
> 173: 
> 174:     @Override

In the `init()` method below, just write `prevPubKey = trustedPubKey` no matter if `trustedPubKey` is null or not.

src/java.base/share/classes/sun/security/provider/certpath/AlgorithmChecker.java line 318:

> 316:             prevPubKey = currPubKey;
> 317:             return;
> 318:         }

I find it more natural to enclose the block below in a `if (prevPubKey != null)` block, and you only need to call one `prevPubKey = currPubKey` at the end.

src/java.base/share/classes/sun/security/provider/certpath/AlgorithmChecker.java line 362:

> 360:         // Don't bother if the check has started or trust anchor has already
> 361:         // been specified.
> 362:         if (this.prevPubKey == null) {

Maybe it's cleaner to write `if (this.trustedPubKey == null)` above. Anyway, after `init()` they are the same.

src/java.base/share/classes/sun/security/provider/certpath/AlgorithmChecker.java line 363:

> 361:         // been specified.
> 362:         if (this.prevPubKey == null) {
> 363:             if (anchor == null) {

This won't happen. Or, you can ignore it.

This makes it possible to call this method in the constructor.

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

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