RFR: 8297878: KEM: Implementation [v10]

[Weijun Wang]

On Wed, 26 Apr 2023 20:56:38 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Every other crypto engine class throws IKE when initing with a null key, including `Signature`, `Cipher`, `KeyAgreement`, `Mac`. So I follow the same style.
>
> Hmm, while I understand your thinking, this seems like the wrong pattern to follow though (ex: see Effective Java, Item 70), and it doesn't seem like something we should have to adhere to for API consistency. There are various bugs filed over the years on confusion over this style (search for "Cipher init NullPointerException"), where providers threw NPE or users thought the APIs should. Actually, there is still an open bug: https://bugs.openjdk.org/browse/JDK-4955097

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);
    }

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1178422376