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

Thu Apr 14 17:33:52 UTC 2016

without sounding like a broken record(!), can I ask that new exceptions 
provide better information (where possible) ?

com/sun/crypto/provider/DHKeyPairGenerator.java

> +            throw new InvalidParameterException(
> +                    "Keysize must be multiple of 64, and can only range " +
> +                    "from 512 to 8192 (inclusive)");
This is a new checkKeySize method - we should print what keysize was 
attempted in the exception message! Granted - the code was borrowed from 
old code but hopefully you can improve while here.

likewise in com/sun/crypto/provider/DHParameterGenerator.java (print the 
keysize in InvalidParameterException)

Can we improve this exception message while here also  ?
> +        if ((exponentSize <= 0) || (exponentSize >= primeSize)) {
> +            throw new InvalidAlgorithmParameterException(
> +                "Exponent size must be positive and less than modulus size");

> +        } catch (Exception ex) {
> +            throw new ProviderException("Unexpected exception: " + ex);

Could we use the two arg constructor for ProviderException above and 
pass ex as Throwable ?

===

sun/security/provider/DSAParameterGenerator.java
> +                ("Prime size should be 512 - 1024, or 2048, 3072");

again, could we print the strength variable in above exception ?

===

sun/security/ssl/ServerHandshaker.java

>                       throw new IllegalArgumentException(
> -                        "Customized DH key size should be positive integer " +
> -                        "between 1024 and 2048 bits, inclusive");
> +                        "Customized DH key size should not be less " +
> +                        "than 1024 bits");

Can we print customizedDHKeySize value ?

===

sun/security/pkcs11/P11KeyPairGenerator.java

Let's get the keySize printed in all InvalidAlgorithmParameterException 
calls that you're editing.

Thanks,
Sean.

On 14/04/2016 12:12, Xuelei Fan wrote:
> Thanks for the comments.  Here is the new webrev:
>      http://cr.openjdk.java.net/~xuelei/8072452/webrev.03/
>
> On 4/14/2016 7:25 AM, Valerie Peng wrote:
>> 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?
>>
> Updated.
>
>> <test/sun/security/provider/DSA/TestAlgParameterGenerator.java>
>> - format changes only? No test for (3072, 256)?
>>
> I removed the update of this file.
>
>> <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?
>>
> Updated.
>
>> <test/sun/security/pkcs11/KeyAgreement/SupportedDHKeys.java>
>> - line 63, why test for KeyAgreement here instead of KeyPairGenerator?
>>
> Better to use KeyPairGenerator.  Updated.
>
>> <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?
>>
> Updated.
>
>> <test/sun/security/provider/DSA/SupportedDSAParaGen.java>
>> - Use "Param" instead of "Para" in the test name "SupportedDSAParaGen"?
> Hm, nice name.
>
>> - Just curious, have u tried the specified timeout value across
>> different platforms?
> Not really, I only tested on JPRT and the solaris-sparc desktop.  This
> test may be intermittent timeout failure in some circumstances.  I added
> the intermittent key tag.
>
>> - The file is new but the copyright year starts from 2012?
>>
> Updated.
>
>> 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.
> I was wondering to remove the limit in case third party's implementation
> support more size.  Not necessary.  Updated and limited to 8192 bits.
>
>> - 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.
>>
> This is mainly for compatibility and interoperability impact.  Some old
> deployed application may not support DH key size bigger than 2048.  I
> added back the comments.
>
> Thanks,
> Xuelei
>
>> 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