Code review request, 7106773: 512 bits RSA key cannot work withSHA384 and SHA512

[Weijun Wang]

On 01/11/2012 07:42 PM, Xuelei Fan wrote:
> On 1/11/2012 7:12 PM, Weijun Wang wrote:
>>
>>
>> On 01/11/2012 06:55 PM, Xuelei Fan wrote:
>>> On 1/11/2012 6:42 PM, Weijun Wang wrote:
>>>>
>>>>
>>>> On 01/11/2012 06:02 PM, Xuelei Fan wrote:
>>>>> On 1/11/2012 5:50 PM, Weijun Wang wrote:
>>>>>> Hi Andrew
>>>>>>
>>>>>> Take a brief look at the webrev. Looks like this Lengthable thing is the
>>>>>> only change after your previous webrev. Please confirm.
>>>>>>
>>>>> Yes.
>>>>>
>>>>>> But I want something bigger. I would like to know if it is possible to
>>>>>> add this keysize() method deep down into the very basic Key interface.
>>>>>> If Key can have a method called getEncoded() I think this means it
>>>>>> normally has a concrete form and surely has a publicly acceptable
>>>>>> keysize() attribute. In JDK 8 we have default implementation for new
>>>>>> interface methods. Is this issue a good candidate?
>>>>>>
>>>>> As Key is an java interface, we may not be able to add one more method
>>>>> for compatibility reason. We may export the "Lengthable"/"Measurable"
>>>>> interface in JDK 8. It's possible to implement Lengthable in all
>>>>> sub-classes of Key in Oracle provider, but as would involve too many
>>>>> changes. As we need to backport this fix into JDK 7, I think we'd better
>>>>> consider the big picture in the future.
>>>>
>>>> Then I think the previous webrev is enough for JDK 7, and for JDK 8, we
>>>> simply add a new keysize() method to Key.
>>>>
>>> If we add one new method to Key interfaces. The providers based on JDK 7
>>> and previous releases would have to update their codes so as to
>>> implement this new method. As will result in serious compatibility issues.
>>
>> I am talking about the new default method language feature in JDK 8 ([1]
>> Section 11, 12). Then the default impl of Key::keySize() returns -1,
>> default impl of SecretKey::keySize() returns getEncoded().length()*8, etc.
>>
> A great feature!
> 
>>>
>>> It is possible that we export the "Lengthable" interface, and have
>>> Oracle providers support this interface, and suggest other providers to
>>> use this interfaces.
>>>
>>> The previous webrev hurt the performance a little because of reflections.
>>
>> Thanks for reminding me this. Yes, those P11 and MSCAPI keys. This
>> webrev is still necessary, and the code changes are fine except for
>>
>> 1. SignatureAndHashAlgorithm.java:283, you left a System.out.println
>>
>> 2. KeyLength.java:58, more System.out.printlns
>>
> Should remove them.
> 
>> 3. KeyLength.java:88, UnsupportedOperationException, necessary?
>>
> I want to reserve the flexibility that we may not be able to get key
> size from a hardware device. Yes, the exception are not thrown in our
> implementation.
> 
> BTW, I will change the interface name from "Lengthable" to "Measurable".

Well, this is not much better. There are many things that can be
measured, length, weight, duration. But at least it's a real word.

> 
> Do you want to review more? If not, I will update the code locally and
> push the changes.

No more.

Thanks
Max

> 
> Thanks,
> Xuelei
> 
>> Thanks
>> Max
>>
>> [1] http://cr.openjdk.java.net/~briangoetz/lambda/lambda-state-4.html
>>
>>>
>>> Xuelei
>>>
>>>> Max
>>>>
>>>>>
>>>>>> At least, in KeyLength::getKeySize(), I would like to see "if (key
>>>>>> instanceof Lengthable)" to be the first check, and, if possible, the
>>>>>> only one needed, at least for keys from providers built in JDK.
>>>>>>
>>>>> It's OK to check it at first. But as we also need to support other
>>>>> providers, I think we'd better also check other types of instance.
>>>>>
>>>>> Thanks,
>>>>> Xuelei
>>>>>
>>>>>> Thanks
>>>>>> Max
>>>>>>
>>>>>>
>>>>>> On 01/11/2012 08:57 AM, Xuelei Fan wrote:
>>>>>>> "Measurable" looks like a better name. I will update the name in the
>>>>>>> next webrev after this round of code review:
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~xuelei/7106773/webrev.04/
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Xuelei
>>>>>>>
>>>>>>> On 1/10/2012 11:47 PM, Vincent Ryan wrote:
>>>>>>>> On 01/10/12 03:19 PM, Xuelei Fan wrote:
>>>>>>>>> On 1/10/2012 11:09 PM, Weijun Wang wrote:
>>>>>>>>>> It's late night and I'll read it tomorrow. But can you choose another
>>>>>>>>>> word instead of Lengthable? Length is not a verb.
>>>>>>>>>>
>>>>>>>>> ;-) The name took me a lot of time, searching by google, dictionary, and
>>>>>>>>> any possible English translation. I have to agree that I failed to find
>>>>>>>>> a suitable name. I tried hardly to persuade myself that "lengthable" is
>>>>>>>>> also used by someother application code, so it might not too bad to use
>>>>>>>>> it here.
>>>>>>>>>
>>>>>>>>> With the word "lengthable", I want to express that the length is
>>>>>>>>> measurable. Any suggestion for the better one?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Measurable ;-)
>>>>>>>>
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Xuelei
>>>>>>>>>
>>>>>>>>>> Max
>>>>>>>>>> ------------------------------------------------------------------------
>>>>>>>>>> 发件人: Xuelei Fan
>>>>>>>>>> 发送时间: 2012/1/10 22:51
>>>>>>>>>> 收件人: Weijun Wang
>>>>>>>>>> 抄送: OpenJDK
>>>>>>>>>> 主题: Re: Code review request, 7106773: 512 bits RSA key cannot work
>>>>>>>>>> withSHA384 and SHA512
>>>>>>>>>>
>>>>>>>>>> It has been around 50 days passed since the last day we talked about the
>>>>>>>>>> issue. Hope you can recall it from the deep memory. ;-)
>>>>>>>>>>
>>>>>>>>>> webrev: http://javaweb.us.oracle.com/~xufan/bugbios/7106773/webrev.04/
>>>>>>>>>>
>>>>>>>>>> In this update, as we agreed, a new Oracle private interface was
>>>>>>>>>> introduced: sun.security.util.Lengthable, and Lengthable.length() is
>>>>>>>>>> defined to get the length an object. sun.security.pkcs11.P11Key and
>>>>>>>>>> sun.security.mscapi.Key will implements the interface. As will easy and
>>>>>>>>>> speedup (comparing with reflection approach) the getting of key length
>>>>>>>>>> of those unextractable keys in hardware device.
>>>>>>>>>>
>>>>>>>>>> In the webrev, I should also include another two signed jars,
>>>>>>>>>> sunpkcs11.jar and sunmscapi.jar. I will include them when I get the
>>>>>>>>>> official signed jars.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Xuelei
>>>>>>>>>>
>>>>>>>>>> On 11/22/2011 8:41 AM, Weijun Wang wrote:
>>>>>>>>>>> I really like this one.
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> Max
>>>>>>>>>>>
>>>>>>>>>>> On 11/21/2011 08:05 PM, Xuelei Fan wrote:
>>>>>>>>>>>>>>      How about this approach? This looks very safe.
>>>>>>>>>>>>>>
>>>>>>>>>>>> I also prefer this approach, although it need more updates in PKCS11 and
>>>>>>>>>>>> MSCPI source code. If you vote for this approach, I will try to
>>>>>>>>>>>> implement it.
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>