On Tue, 6 Dec 2022 18:45:57 GMT, Sean Mullan <mul...@openjdk.org> wrote:
>> Xue-Lei Andrew Fan has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains six commits:
>>
>> - check duplicate
>> - Merge
>> - Merge
>> - Merge
>> - add test cases
>> - 8281236: (D)TLS key exchange algorithms
>
> src/java.base/share/classes/sun/security/ssl/NamedGroup.java line 454:
>
>> 452: }
>> 453:
>> 454: static NamedGroup getPreferredGroup(
>
> Add a comment describing what this method does. And the method on line 471.
Updated.
> src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java line 279:
>
>> 277: String[] ngs = params.getNamedGroups();
>> 278: if (ngs != null) {
>> 279: // Note if 'ss' is empty, then no signature schemes should
>> be
>
> The comment needs to be updated for named groups. It looks like it was copied
> from line 272.
Thanks for the catch. Updated.
> test/jdk/javax/net/ssl/SSLParameters/NamedGroups.java line 122:
>
>> 120: null,
>> 121: false);
>> 122: runTest(new String[0],
>
> I think this case will throw IAE when `SSLParameter.setNamedGroups()` is
> called right? If so, I think you can delete this test (and the one one on
> line 123) since it is already covered in the NamedGroupsSpec test. Also, this
> test is coded such that the expected exception is thrown when the TLS
> handshake is being made in `runClientApplication()`, which is not the case
> here.
>
> Same comment for the DTLSNamedGroups test.
It is allowed to set empty named groups with `SSLParameter.setNamedGroups()`,
which means (see spec at `getNamedGroups`) that
* If the returned array is empty (zero-length), then the named group
* negotiation mechanism is turned off for SSL/TLS/DTLS protocols, and
* the connections may not be able to be established if the negotiation
* mechanism is required by a certain SSL/TLS/DTLS protocol.
The TLS connection will fail, as expected, although for this case.
> test/jdk/javax/net/ssl/SSLParameters/NamedGroupsSpec.java line 27:
>
>> 25: * @test
>> 26: * @bug 8281236
>> 27: * @summary (D)TLS key exchange named groups
>
> Can you write a more specific summary of what this test is testing? The
> @summary doesn't have to match the bug title (and often should not IMO).
>
> Same comment for the other tests.
It makes senses to me. Updated accordingly.
> test/jdk/javax/net/ssl/SSLParameters/NamedGroupsSpec.java line 34:
>
>> 32: public class NamedGroupsSpec {
>> 33: public static void main(String[] args) throws Exception {
>> 34: runTest(new String[] {
>
> How about adding a test for a `null` array?
It is a good case that I missed. Added.
> test/jdk/javax/net/ssl/SSLParameters/NamedGroupsSpec.java line 68:
>
>> 66: SSLParameters sslParams = new SSLParameters();
>> 67: try {
>> 68: sslParams.setNamedGroups(namedGroups);
>
> You should also test that `getNamedGroups` returns the same elements.
Good point. Added. Thank you!
-------------
PR: https://git.openjdk.org/jdk/pull/9776
