On Thu, 29 May 2025 00:54:37 GMT, Bradford Wetmore <wetmore at openjdk.org> wrote:
>> Adds the RFC 5705/8446 TLS Key Exporters API/implementation to JSSE/SunJSSE respectively.
>>
>> CSR is complete/approved.
>>
>> Tests include new unit tests for TLSv1-1.3. Have run tier1-2, plus the JCK API (jck:api/java_security jck:api/javax_crypto jck:api/javax_net jck:api/javax_security jck:api/org_ietf jck:api/javax_xml/crypto)
>
> Bradford Wetmore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 37 commits:
>
> - Merge branch 'master' into JDK-8341346
> - Remove TlsExporterKeyingMaterial for now. Can add later if needed.
> - Merge branch 'master' into JDK-8341346
> - Merge branch 'master' into JDK-8341346
> - Added PKCS11 testing
> - Minor bug
> - Missed one change
> - Merge branch 'master' into JDK-8341346
> - Private Codereview comment: Don't allow use of null keyAlgs, plus some minor cleanups
> - get*() no longer needed, backout error (oops!)
> - ... and 27 more: https://git.openjdk.org/jdk/compare/2ec6ab34...858362c8
src/java.base/share/classes/javax/net/ssl/ExtendedSSLSession.java line 176:
> 174: * RFC 8446 (TLSv1.3) treats a null context as non-null/empty.
> 175: * <P>
> 176: * The {@code label} {@code String} will be converted to bytes using
Two consecutive `{@code}` look a little strange to me, visually. How about just `{@code label} will be...`? You already use that in `@param label`. Same as in the other method.
src/java.base/share/classes/javax/net/ssl/ExtendedSSLSession.java line 198:
> 196: * {@code label} will be converted to a {@code byte[]}
> 197: * before the operation begins.
> 198: * @param context the context bytes used in the EKM calculation, or null
`null` needs to be in `{@code}`. Same as in `@throws NullPointerException`. Same as in the other method.
src/java.base/share/classes/sun/security/internal/spec/TlsPrfParameterSpec.java line 85:
> 83: * @param secret the secret to use in the calculation (or null)
> 84: * @param keyAlg the algorithm name that the generated SecretKey should
> 85: * have, or null if the default should be used
You no longer allow it to be `null`.
src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 1494:
> 1492: if (exporterMasterSecret == null) {
> 1493: throw new RuntimeException(
> 1494: "Exporter master secret not captured");
Do you want to choose another exception type? Like `ProviderException`. Actually, how unlikely this is? If you believe this would never happen (unless there is a programming error), you can even throw an `AssertionError`.
Same question in `useTLS10PlusSpec()` for the two randoms.
test/jdk/javax/net/ssl/ExtendedSSLSession/ExportKeyingMaterialTests.java line 1:
> 1: /*
Do you want to test about null/empty context difference in TLS 1.2/1.3?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24976#discussion_r2114057734
PR Review Comment: https://git.openjdk.org/jdk/pull/24976#discussion_r2114060429
PR Review Comment: https://git.openjdk.org/jdk/pull/24976#discussion_r2114065976
PR Review Comment: https://git.openjdk.org/jdk/pull/24976#discussion_r2114120216
PR Review Comment: https://git.openjdk.org/jdk/pull/24976#discussion_r2114083657