Code Review Request 8072452 Support DHE sizes up to 8192-bits

[Valerie Peng]

Here are some comments for the test changes:

<test/sun/security/pkcs11/KeyPairGenerator/TestDH2048.java>
- Not testing key size 768?
- line 82, would be useful to indicate version here, e.g. NSS (as of 
version xxx) has a hard coded ...
- line 27, 4096 should be 8192?

<test/sun/security/provider/DSA/TestAlgParameterGenerator.java>
- format changes only? No test for (3072, 256)?

<test/com/sun/crypto/provider/KeyAgreement/UnsupportedDHKeys.java>
- line 50, dhp8192(8191) looks like a typo?
- line 66, do we really need to generate the key pair here?
- line 71, seem redundant and potentially confusing as it's aligned 
differently comparing to line70?

<test/sun/security/pkcs11/KeyAgreement/SupportedDHKeys.java>
- line 63, why test for KeyAgreement here instead of KeyPairGenerator?

<test/sun/security/pkcs11/KeyAgreement/UnsupportedDHKeys.java>
- line 53, dhp8192(8191) looks like a typo?
- line 64, why test for KeyAgreement here instead of KeyPairGenerator?
- line 75, do we really need to generate the key pair here?
- line 80, seem redundant and potentially confusing as it's aligned 
differently comparing to line79?

<test/sun/security/provider/DSA/SupportedDSAParaGen.java>
- Use "Param" instead of "Para" in the test name "SupportedDSAParaGen"?
- Just curious, have u tried the specified timeout value across 
different platforms?
- The file is new but the copyright year starts from 2012?

As for the updated sources, all changes look fine. But while I was 
looking at 
src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java, I 
have some more questions:
- line 145, do we need to check for upper limit here? It used to have 
2048, should it be 8192 now? If yes, then line 1526 need to be enhanced too.
- line 1551, now that more DH sizes are supported, shouldn't we make 
some changes here? Now it's either 1024 or 2048. The older impl on line 
1550 uses 1024 as the lower limit and 2048 as the upper limit and will 
keep the value as is if it's in-between the limits. If there is a reason 
to keep 2048 as the upper limit, it'd be useful to add some comment here 
for clarification.

Thanks,
Valerie

On 4/13/2016 7:23 AM, Xuelei Fan wrote:
> Hi Valerie,
>
> All comments are good to me and accepted in the new webrev:
>
>    http://cr.openjdk.java.net/~xuelei/8072452/webrev.02/
>
> On 4/13/2016 8:26 AM, Valerie Peng wrote:
>> Hi Xuelei,
>>
>> Mostly look good, just some comments in line...
>>
>> <src/java.base/share/classes/com/sun/crypto/provider/DHKeyPairGenerator.java>
>>
>> line 95-100, so essentially, we will use the built-in parameters as much
>> as possible and if none available, we will try to generate the
>> parameters for key sizes<= 1024, right? The current comment is a little
>> hard to parse. Maybe we can just explain it as below?
>>       // Use the built-in parameters (ranging from 512 to 8192) when
>> available
>>       this.params = ParameterCache.getCachedDHParameterSpec(keysize);
>>      // Due to performance issue, only support DH parameters generation
>>      // up to 1024 bits
>>      if ((this.params == null)&&  (keysize>  1024)) {
>>
> Updated.
>
>> <src/java.base/share/classes/sun/security/provider/DSAParameterGenerator.java>
>>
>> - line 212, redundant?
> Yes.  Updated.
>
>> <src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java>
>>
>> - line 277-303, the "params"argument for this particular method is
>> always null for non-RSA algorithms based on the usage. It is commented
>> on line 232 and several other places as indicated by the "// XXX sanity
>> check params" comments. So, more code changes will be needed here.
>> Either you can add another boolean hasParams and rely on that argument,
>> or change the specified argument type from RSAKeyGenParameterSpec to a
>> generic one and then cast it later when it is needed.
>>
> Good catch!  Updated per the latter proposal.
>
>> <src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java>
>> - line 141 - 146 and line 1548 - 1554 are identical. Seems to me that
>> they can be customized a little as they are for different variable
>> assignment. The "customizedDHKeySize" variable on line 147 is from user,
>> right? Maybe what you really mean is something like the following?
>>      // DH parameter generation can be extremely slow, best to use one of
>> the
>>      // supported pre-computed DH parameters (see DHCrypt class)
>> As for line 1548 - 1554, it can be as simple as:
>>      // DH parameter generation can be extremely slow, make sure to use
>> one of the
>>      // supported pre-computed DH parameters (see DHCrypt class)
> Updated.
>
>> - line 148, can customizedDHKeySize be 1024? The old code accepts it but
>> the new code will error out. Is there a reason for rejecting 1024? The
>> check only rejects "<1024" but yet the exception message says the value
>> should be larger than 1024.
>> Lastly, "should larger" =>  "should be larger" (line 150).
>>
> 1024 bits should not be rejected.  Updated the exception message.
>
> Thanks for the comments!
>
> Xuelei
>
>> I will look at the tests next and send u comments separately.
>> Thanks,
>> Valerie
>> On 4/1/2016 8:53 AM, Xuelei Fan wrote:
>>> Hi,
>>>
>>> Please review this improvement update to support DHE sizes up to
>>> 8192-bits and DSA sizes up to 3072-bits:
>>>
>>>     http://cr.openjdk.java.net/~xuelei/8072452/webrev.00
>>>
>>> This updated extends to support 3072-bits DH and DSA parameters
>>> generation, and pre-computed DH parameters up to 8192 bits and
>>> pre-computed DSA parameters up to 3072-bits.
>>>
>>> Thanks,
>>> Xuelei