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

[Xuelei Fan]

Updated webrev:
    http://cr.openjdk.java.net/~xuelei/8072452/webrev.01

On 4/2/2016 5:36 AM, Bernd Eckenfels wrote:
> Hello Xuelei,
> 
> glad to see this. :)
> 
>  
> --java.base/share/classes/sun/security/ssl/DHCrypt.java
> 
> Does the comment "// FEDHE" stand for FFDHE (JDK-8140436)?
Good catch! Updated to "FFDHE".

> maybe name the variables ffdheXXXX instead of pXXXX? (they might be
> directly used clients to verify well known parameters)
> 
'p' in "pXXX" means the prime modulus p of DH parameters.  If using
ffdheXXX, there are more than 80 characters in the line.  I would prefer
to use pXXX instead.

> 
> Unrelated to the change: What about reomving p512 and p768 from the
> cache. In normal scenarios they are disabled, but if somebody enables
> the weak sizes he would benefit from dynamic generation (and not much
> time is needed).
> 
In general, the dynamic generation is still slow for a server. 512 and
768 should be used for corner cases only at present.

> 
> Line 154: just an observation, the other places in this file request
> 'JsseJce.get*("DiffieHellmann"' but the KeyFactory is requested by name
> "DH".
> 
"DH" is an alias of "DiffieHellmann".  Nice to use the formal name.

> 
> --java.base/share/classes/com/sun/crypto/provider/DHKeyPairGenerator.java
> 
> For my taste it would be good to document why generation of
> modulus primes >1024 bit is not supported. Is this a performance issue
> or a problem with the security of the generator?)
> 
Performance issue, very slow.  Updated the comment for the reason.

> 
> --java.base/share/classes/com/sun/crypto/provider/DHParameterGenerator.java
> 
> Line 64+70: Does not support 8k?
> 
Not yet because the performance issue.

> 
> --java.base/share/classes/sun/security/provider/ParameterCache.java
> 
> Observation unrelated to the patch: Would it make sense in DHCrypt to
> reference the parametes from ParamterCache and not have 2 distinguished
> places where same (if any) constants are defined? (the differente
> caches can still exist)
> 
Better to use different groups for different protocols.  The groups used
in DHCrypt are for SSL/TLS connections only, which would better be
different from the groups defined in ParameterCache.  For the same
constants (defined by 2409/3526), it would be nice to update to use
different safe primes later.

> Line  68+94+168 - the 3072bit case is provided, should also be supporte?
> 
Good catch!

> Line 291+333 - in case you need to have another revision there is a
> nit: blank lines are used in all other places before the xxxCache.put
> 
Yes.

> Another observation not related to your change: 
> DHParameterGenerator:141+157+DSAParameterGenerator:152 is it
> intentional that two use provider SUN, SunJCE and one does not specify
> a provider?
> 
I think so.  Algorithm parameters normally can be provider independent.

Thanks a lot for the review.

Regards,
Xuelei

> Greetings
> Bernd
> 
> 
> Am Fri, 1 Apr 2016 08:53:05 -0700
> schrieb Xuelei Fan <Xuelei.Fan at Oracle.COM>:
> 
>> 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
>