Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
On Thu, 16 May 2024 06:26:12 GMT, Alan Bateman wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> javadoc formatting
>
> src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 270:
>
>> 268:
>> 269: /**
>> 270: * Returns a builder for building {@code Extract}-Only and {@code
>> ExtractThenExpand} objects.
>
> The class description speaks of Extract-Only operations. Here we are talking
> about the Extract object so I think confusing to have "-Only" appended to the
> class name. I see the same in the expandOnly method.
Yes, I agree with (and prefer) the original `Extract` naming convention. This
change was suggested during code reviews, but I can revert it to be as you
suggest.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1603628488
Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
On Thu, 16 May 2024 06:38:29 GMT, Alan Bateman wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> javadoc formatting
>
> src/java.base/share/classes/javax/crypto/KDF.java line 202:
>
>> 200: * @param provider
>> 201: * the provider to use for this key derivation; if null, this
>> method is
>> 202: * equivalent to {@code getInstance(String)}
>
> It might be better to disallow null here. If the code doesn't have a provider
> name then it would be clearer to use the 1-arg getInstance method. That would
> also help catch bugs where the provider name is null due to some bug.
I agree that is probably better. Our JCE APIs are somewhat inconsistent on this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1603587186
Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
On Wed, 15 May 2024 19:59:59 GMT, Kevin Driver wrote: >> Introduce an API for Key Derivation Functions (KDFs), which are >> cryptographic algorithms for deriving additional keys from a secret key and >> other data. See [JEP 478](https://openjdk.org/jeps/478). > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > javadoc formatting We cannot say the KDF instance is immutable because it does have an internal state and it could change. On the other hand, because 1) the change is done only once, 2) it's synchronized, 3) it's not observable before the change, we can at least say the instance is "stable", which means its `getProviderName` method will always return the same value. It's a little unfortunate that even the `getProviderName` performs a provider selection, but this is all for the "stable" effort. If it throws an ISE, then the object's mutability is publicly observable. We've done this in all existing crypto engines, like `Signature`, `KeyAgreement`, and `Cipher`. I don't think it's worth introducing a brand new behavior. As for the benefit of this design in a real world environment, I think it's safe to assume that an application usually has input keying materials of the same type, either software based or from an HSM, and in this case the correct provider will be selected and all `deriveXyz` calls should succeed. On the other hand, if multiple threads call the method with different types of input keying materials, then yes some will succeed and some will fail, and it depends on who is the 1st one to acquire the lock to decide the provider. This could also happen in a single thread. I don't think this can be called intermittent, either this half or the other half will fail anyway. - PR Comment: https://git.openjdk.org/jdk/pull/18924#issuecomment-2115083687
Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
On Wed, 15 May 2024 19:59:59 GMT, Kevin Driver wrote: >> Introduce an API for Key Derivation Functions (KDFs), which are >> cryptographic algorithms for deriving additional keys from a secret key and >> other data. See [JEP 478](https://openjdk.org/jeps/478). > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > javadoc formatting Back to the question about delayed provider selection and concurrent threads using a shared KDF instance to derive a key or raw data. Is it possible that InvalidParameterSpecException may be thrown intermittently in that scenario? I'm trying to assess if the issues with delayed provider selection is a major design issue or not. - PR Comment: https://git.openjdk.org/jdk/pull/18924#issuecomment-2114248643
Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
On Wed, 15 May 2024 19:59:59 GMT, Kevin Driver wrote:
>> Introduce an API for Key Derivation Functions (KDFs), which are
>> cryptographic algorithms for deriving additional keys from a secret key and
>> other data. See [JEP 478](https://openjdk.org/jeps/478).
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> javadoc formatting
src/java.base/share/classes/javax/crypto/KDF.java line 202:
> 200: * @param provider
> 201: * the provider to use for this key derivation; if null, this
> method is
> 202: * equivalent to {@code getInstance(String)}
It might be better to disallow null here. If the code doesn't have a provider
name then it would be clearer to use the 1-arg getInstance method. That would
also help catch bugs where the provider name is null due to some bug.
src/java.base/share/classes/javax/crypto/KDF.java line 228:
> 226:
> 227: /**
> 228: * Returns a {code KDF} object that implements the specified
> algorithm from
Missing the `@` here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1602698687
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1602696489
Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
On Wed, 15 May 2024 19:59:59 GMT, Kevin Driver wrote:
>> Introduce an API for Key Derivation Functions (KDFs), which are
>> cryptographic algorithms for deriving additional keys from a secret key and
>> other data. See [JEP 478](https://openjdk.org/jeps/478).
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> javadoc formatting
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 92:
> 90: * {@code thenExpand} for {@code Extract} and {@code
> ExtractThenExpand}
> 91: * use-cases respectively.
> 92: */
I think it would be useful to say that Builder is not thread safe.
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 115:
> 113: * @return an immutable {@code Extract}
> 114: */
> 115: public Extract extractOnly() {
I think you can expand the method description to say that it builds the Extract
method from the current state of the builder.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1602692495
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1602690521
Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
On Wed, 15 May 2024 19:59:59 GMT, Kevin Driver wrote:
>> Introduce an API for Key Derivation Functions (KDFs), which are
>> cryptographic algorithms for deriving additional keys from a secret key and
>> other data. See [JEP 478](https://openjdk.org/jeps/478).
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> javadoc formatting
src/java.base/share/classes/javax/crypto/spec/HKDFParameterSpec.java line 270:
> 268:
> 269: /**
> 270: * Returns a builder for building {@code Extract}-Only and {@code
> ExtractThenExpand} objects.
The class description speaks of Extract-Only operations. Here we are talking
about the Extract object so I think confusing to have "-Only" appended to the
class name. I see the same in the expandOnly method.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1602686623
Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
On Wed, 15 May 2024 19:59:59 GMT, Kevin Driver wrote:
>> Introduce an API for Key Derivation Functions (KDFs), which are
>> cryptographic algorithms for deriving additional keys from a secret key and
>> other data. See [JEP 478](https://openjdk.org/jeps/478).
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> javadoc formatting
src/java.base/share/classes/javax/crypto/KDFSpi.java line 85:
> 83: * @param alg
> 84: * the algorithm of the resultant {@code SecretKey} object (may
> not be
> 85: * {@code null})
Now that the method specifying NPE when either param is null then I assume the
"may not be null" is no longer needed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1602681710
Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
On Wed, 15 May 2024 19:59:59 GMT, Kevin Driver wrote: >> Introduce an API for Key Derivation Functions (KDFs), which are >> cryptographic algorithms for deriving additional keys from a secret key and >> other data. See [JEP 478](https://openjdk.org/jeps/478). > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > javadoc formatting > > * The KDF.deriveXXX methods mention "Delayed provider selection". Is this > > idempotent? If I create a KDF and several threads race to derive keys or > > data, is it guaranteed that the same provider will be selection for any > > ordering of these threads? What does KDF::getProviderName if no provider > > has been selected? > > I believe it would _not_ be guaranteed that the same provider would be > selected for any ordering of the threads (depending on their possibly unique > KDFParameterSpec values). Is this a documentation call-out? Or did you have a > concern about this? It means that a KDF is stateful and mutable. It may be thread safe but it would be a hazard to attempt to use a shared instance. So I think there is a bit of a design smell here, something to look at for the next preview. The behaviour of KDF::getProviderName with delayed provider selection is very surprising. It may be that this method has to throw IllegalStateException when not bound to a provider. - PR Comment: https://git.openjdk.org/jdk/pull/18924#issuecomment-2114096567
Re: RFR: 8331008: Implement JEP 478: Key Derivation Function API (Preview) [v77]
> Introduce an API for Key Derivation Functions (KDFs), which are cryptographic > algorithms for deriving additional keys from a secret key and other data. See > [JEP 478](https://openjdk.org/jeps/478). Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: javadoc formatting - Changes: - all: https://git.openjdk.org/jdk/pull/18924/files - new: https://git.openjdk.org/jdk/pull/18924/files/726244be..a61c59d7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18924=76 - incr: https://webrevs.openjdk.org/?repo=jdk=18924=75-76 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18924.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18924/head:pull/18924 PR: https://git.openjdk.org/jdk/pull/18924
