Code Review Requests for 7196382 and 8010134

[Valerie (Yu-Ching) Peng]

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