Code Review Requests for 7196382 and 8010134

[Xuelei Fan]

> http://cr.openjdk.java.net/~valeriep/7196382/webrev.02/
This is the latest webrev. ;-)

Looks fine to me.

Xuelei


On 7/11/2013 10:25 AM, Xuelei Fan wrote:
> Ooops, please ignore this mail.  I went into the wrong webrev.
> 
> Xuelei
> 
> On 7/11/2013 10:23 AM, Xuelei Fan wrote:
>> src/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java
>> ==============================================================
>> --------
>> It is instinctive and easy to maintain to code if we have the comment:
>> +        // RSA, DH, and DSA
>>  104     keySize = 1024;
>>
>>
>> --------
>> I'm not sure, just a double check. Do we really allow DSA  keysize is
>> not a multiple of 64 bits when it is bigger than or equal to 1024?
>>
>>  240     } else if (algorithm.equals("DSA")) {
>>  241         if ((ks < 1024) && ((ks & 0x3f) != 0)) {
>>  242             throw new InvalidAlgorithmParameterException
>>  243                 ("DSA key must be a multiple of 64 bits");
>>
>> --------
>> If the mechanism info is not specified in the underlying device
>> (minKeySize/maxKeySize is -1), do we still want to reserve the minimal
>> length checking (112 for EC, 512 for non-EC)? I think we may still want
>> the constraints. For example:
>>
>>  226         } else {
>> +                // The key size is too small.
>> +                if (algorithm.equals("EC")) {
>> +                    // keysize < 112, throws exception
>> +                } else {
>> +                    // keysizse < 512, throws exception
>> +                }
>>
>>
>> Xuelei
>>
>> On 7/10/2013 8:33 AM, Valerie (Yu-Ching) Peng wrote:
>>> Xuelei,
>>>
>>> I got side-tracked w/ other bugs before coming back to this one.
>>>
>>> I have updated the webrev to incorporate your review comments, including
>>> 1) auto-adjust the default keysize when it's out of range
>>> 2) apply our own range checking besides the native limit checking
>>>
>>> I also noticed that the DH parameter generator from SunJCE provider
>>> relies on DSA, so I checked its keysize checking to match the DSA keysize.
>>>
>>> The webrev is updated at:
>>> http://cr.openjdk.java.net/~valeriep/7196382/webrev.02/
>>> Thanks,
>>> Valerie
>>>
>>> On 04/27/13 01:58, Xuelei Fan wrote:
>>>> I like this update.
>>>>
>>>> On 4/27/2013 7:29 AM, Valerie (Yu-Ching) Peng wrote:
>>>>> Xuelei,
>>>>>
>>>>> I have updated the webrev for 7196382 so it uses the key size range info
>>>>> from the underlying PKCS library for key size checking:
>>>>> http://cr.openjdk.java.net/~valeriep/7196382/webrev.01/
>>>>>
>>>> 107  //TBD: auto-adjust default keysize in case it's out-of-range?
>>>>
>>>> I think it's nice to enable this following block. The key size will be
>>>> checked in initialize(), so I think it is a little bit reasonable to
>>>> select a proper default key size instead of throwing an exception later.
>>>>
>>>>
>>>> 209 private void checkKeySize(int ks, RSAKeyGenParameterSpec params)
>>>>
>>>> I think when minKeySize is -1, we need to consider the default key size
>>>> limit (EC 112, RSA/DH/DSA 512).  In this update, it seems that if
>>>> minKeySize is -1, we can generate small keys. I don't think it is the
>>>> intended design.  It is similar when maxKeySize is -1.
>>>>
>>>> I was wondering that if minKeySize is -1 (or less than the default
>>>> hard-coded key size), or maxKeySize is -1 (or greater than the
>>>> hard-coded default key size), we may be able to reset them to the
>>>> default hard-coded sizes.
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>>> Thanks,
>>>>> Valerie
>>>>>
>>>>> On 04/25/13 10:59, Valerie (Yu-Ching) Peng wrote:
>>>>>> Xuelei,
>>>>>>
>>>>>> Thanks for the review and comments.
>>>>>>
>>>>>> Supposedly, we don't have to have default parameters for all valid key
>>>>>> sizes.
>>>>>> The pre-generated default parameters are for the most-commonly used
>>>>>> keysizes.
>>>>>> As for the rest of supported key sizes, the needed parameters will be
>>>>>> generated at runtime upon request.
>>>>>>
>>>>>> Well, I don't quite like the current approach of hardcoding ranges
>>>>>> inside the checkKeySize(...) method.
>>>>>> There is a way to query the supported keysize ranges from the PKCS11
>>>>>> library and I think that should be the values that we base the key
>>>>>> size check on, plus any additional algorithm-specific check (e.g.
>>>>>> multiples of 64 bits) that can't be expressed through the ranges. I am
>>>>>> still testing out the changes. Will post an updated webrev for 7196382
>>>>>> once I am done testing...
>>>>>>
>>>>>> Thanks!
>>>>>> Valerie
>>>>>>
>>>>>> On 04/18/13 21:45, Xuelei Fan wrote:
>>>>>>> On 4/19/2013 10:43 AM, Valerie (Yu-Ching) Peng wrote:
>>>>>>>> Xuelei,
>>>>>>>>
>>>>>>>> Do you have time to review the following two fixes?
>>>>>>>> 7196382: PKCS11 provider should support 2048-bit DH
>>>>>>>> 8010134: A finalizer in sun.security.pkcs11.wrapper.PKCS11 perhaps
>>>>>>>> should be protected
>>>>>>>>
>>>>>>>> The first one removes the hardcoded limit of 1024 for DH and the
>>>>>>>> second
>>>>>>>> one is making the finalize() method protected.
>>>>>>>>
>>>>>>>> Webrevs:
>>>>>>>> http://cr.openjdk.java.net/~valeriep/7196382/webrev.00/
>>>>>>> Looks fine.
>>>>>>>
>>>>>>> Do we plan to support DH keys bwteen 1024 and 2048 with default (null)
>>>>>>> parameters, for example 1536, in PKCS11 provider?  Recently, I run
>>>>>>> into
>>>>>>> a case that uses DH public keys of 1536 bits. I was wondering we may
>>>>>>> also want to support more.
>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~valeriep/8010134/webrev.00/
>>>>>>> Looks fine.
>>>>>>>
>>>>>>> Xuelei
>>>>>>>
>>>>>>>> Thanks!
>>>>>>>> Valerie
>>>
>>
>