RFR: 8281236: (D)TLS key exchange named groups [v4]

Xue-Lei Andrew Fan xuelei at openjdk.org
Wed Dec 7 06:56:38 UTC 2022


On Tue, 6 Dec 2022 18:45:57 GMT, Sean Mullan <mullan at 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



More information about the security-dev mailing list