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

Fri Apr 15 07:19:40 UTC 2016

Looks good!

Thanks,
Sean.

On 15/04/2016 02:41, Xuelei Fan wrote:
> Good points!  I made the update accordingly.
>
>     http://cr.openjdk.java.net/~xuelei/8072452/webrev.04/
>
> Thanks,
> Xuelei
>
> On 4/15/2016 1:33 AM, Seán Coffey wrote:
>> 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