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

Valerie Peng valerie.peng at
Wed Apr 13 00:26:00 UTC 2016

Hi Xuelei,

Mostly look good, just some comments in line...

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 
      this.params = ParameterCache.getCachedDHParameterSpec(keysize);
     // Due to performance issue, only support DH parameters generation
     // up to 1024 bits
     if ((this.params == null) && (keysize > 1024)) {

- line 212, redundant?
- 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.

- 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)
- 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).

I will look at the tests next and send u comments separately.
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:
> 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

More information about the security-dev mailing list