On Wed, 20 Oct 2021 14:47:31 GMT, Sean Mullan <mul...@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
