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

[Sean Mullan]

On Thu, 21 Oct 2021 02:22:18 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> 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.

Ok.

> 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.

Yes.

> 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.

Ok.

> 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.

Agree.

> 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.

Ok, will remove. But I will keep this method separate since, unlike the ctor it needs to check if `trustedPubKey` is `null` before setting the `prevPubKey`.

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

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