On Thu, 27 Apr 2023 13:22:38 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> I checked for our implementations, and until now all I found is IKE. Even >> the PBEWithMD5AndDES algorithm mentioned in >> https://bugs.openjdk.org/browse/JDK-4953551 also throws an IKE. >> >> I'd rather follow the existing style. If it’s not clear enough, I can update >> the line to >> >> @throws InvalidKeyException if {@code publicKey} is {@code null} or invalid >> >> >> If we are worried that a security provider might throw an NPE, how about we >> add a check on the user API side: >> >> public Encapsulator newEncapsulator(PublicKey publicKey, >> AlgorithmParameterSpec spec, SecureRandom secureRandom) >> throws InvalidAlgorithmParameterException, InvalidKeyException { >> if (publicKey == null) { >> throw new InvalidKeyException("Null key provided"); >> } >> return delayed != null >> ? delayed.newEncapsulator(publicKey, spec, secureRandom) >> : new Encapsulator(spi.engineNewEncapsulator(publicKey, >> spec, secureRandom), provider); >> } > >> I checked for our implementations, and until now all I found is IKE. Even >> the PBEWithMD5AndDES algorithm mentioned in >> https://bugs.openjdk.org/browse/JDK-4953551 also throws an IKE. >> >> I'd rather follow the existing style. If it’s not clear enough, I can update >> the line to >> >> ``` >> @throws InvalidKeyException if {@code publicKey} is {@code null} or invalid >> ``` > > I'll leave it up to you, or consensus from others. This one particular issue > is probably not going to overly affect the API much. > > The APIs you referenced were created a long time ago before more accepted API > best practices evolved. Also, this API already throws NPE in other methods > when a required parameter is `null`, so these methods seem inconsistent with > the way it treats `null` parameters. The new `KEM` class still follows the API/SPI design principal. I gave up designing `DecapsulteException` as an uncheck exception and I still use class instead of record for `Encapsulated`. So let's consistently traditional. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1179528648
