On Tue, 8 Oct 2024 19:11:45 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> null check as asserts, and better exception messages
>
> src/java.base/share/classes/sun/security/provider/NamedSignature.java line
> 121:
>
>> 119: return implSign(name, secKey, sk2, msg, appRandom);
>> 120: } else {
>> 121: throw new IllegalStateException("No private key");
>
> `Signature.engineSign` is not specified to throw `IllegalStateException`,
> even though it seems like the more correct exception to throw. So this should
> probably be a `SignatureException`.
>
> Same comment for `engineVerify`.
In fact, this should never happen since `engineVerify` should only be called
when there is a public key (`Signature::verify` checks if `state == VERIFY`).
But I'll use `SignatureException`.
> src/java.base/share/classes/sun/security/provider/NamedSignature.java line
> 196:
>
>> 194: /// This object will be passed into the [#implVerify] method along
>> with the raw key.
>> 195: ///
>> 196: /// The default implementation returns `null`.
>
> If the majority of implementations are supposed to implement this method,
> then it may be better to make it abstract (so that it won't be accidentally
> missed) and add a note that subclasses should return null if they don't have
> any additional checks.
I'll discuss my customers.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1792455162
PR Review Comment: https://git.openjdk.org/jdk/pull/21167#discussion_r1792452057
