On Sun, 12 May 2024 14:39:40 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional >> commit since the last revision: >> >> commenting out until better understood -- causing failures > > src/java.base/share/classes/javax/crypto/KDF.java line 398: > >> 396: * <p> >> 397: * Delayed provider selection is also supported such that the >> provider >> 398: * performing the derive is not selected until the method is called. > > Delayed provider selection is an important enough topic that it probably > should be in the class summary. However it is complicated to word correctly > as there is also the case if someone calls `getProviderName` beforehand which > locks the provider to the first one supporting the algorithm. I would > probably also avoid "delayed provider" as that is not a term currently used > in the javadocs. Suggest something like: > > If a provider is not specified in the getInstance method when instantiating a > KDF object, the provider is selected the first time the deriveKey or > deriveData method is called and a provider is chosen that supports the > parameters passed to the deriveKey or deriveData method, for example the > initial key material. However, if getProviderName is called before calling > the deriveKey or deriveData methods, the first provider supporting the KDF > algorithm is chosen which may not be the desired one; therefore it is > recommended to not call getProviderName until after a key derivation > operation. This is because the selection occurs just once. Should we explicitly mention this? > src/java.base/share/classes/javax/crypto/KDF.java line 414: > >> 412: * >> 413: */ >> 414: public SecretKey deriveKey(String alg, KDFParameterSpec >> kdfParameterSpec) > > Are there cases where the parameters are correct, but the derive operation > can still fail for other reasons? If so, I'm not sure we should be wrapping > those in InvalidParameterSpecException. That exception should be thrown by > the method initially when it inspects the parameters and before the derive > operation begins. > > This is why I mentioned previously if it makes sense to add a > DerivationException class. First, very wrong parameters (say, null info, negative length) should not be create-able at all. Then, in some cases, "correct" parameters could still be "invalid". For example, HKDF expand key length cannot exceed HashLen * 255, but HashLen is determined by the KDF algorithm. In this case, maybe an `InvalidAlgorithmParameterException` should be thrown because it does not conform to the spec. Sometimes the implementation just does not support some parameters. For example, in PKCS #11 you cannot provide an arbitrary string as the algorithm name. Also, only if HKDF expand "info" is a well-known value that's recognized by the underlying implementation, `deriveData` is able to return a byte array. See 7a in https://docs.oasis-open.org/pkcs11/pkcs11-profiles/v3.1/os/pkcs11-profiles-v3.1-os.html#_Toc142307348. In these cases, maybe an `UnsupportedOperationException` should be thrown because the implementation does not support them. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597686355 PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1597685778
