Code review request, 7106773: 512 bits RSA key cannot work with SHA384 and SHA512
Xuelei Fan
xuelei.fan at oracle.com
Mon Nov 21 07:30:43 UTC 2011
new webrev: http://cr.openjdk.java.net/~xuelei/7106773/webrev.02/
I run a few performance tests about the reflection in KeyLength.java.
The result shows that the Method.invoke() does not hurt too much
performance comparing to directly calling Object.method().
Most of the cycle is spent on searching for the method and
implementation class.
In the updated KeyLength.java, I use cached implement classes and
methods. If the current key is a sub-class of one of the cached classes,
the cached method will be used. As will improve the performance
significantly. And there is not much performance hurt overall.
Only KeyLength.java is updated in this webrev.
Thanks,
Xuelei
On 11/19/2011 10:26 AM, Xuelei Fan wrote:
>>> Performance is a big issue here. What do you think if
>>> we change the get key length method of pkcs11.P11Key
>>> and mscapi.Key for better performance?
>> How do you want to change?
>
> I thought to expose the package private class and method. The update
> looks like:
>
> package sun.security.pkcs11;
> ...
> - abstract class P11Key implements Key {
> + abstract public class P11Key implements Key {
> ...
> - int keyLength() {
> + public int keyLength() {
> ...
> }
>
>
> It is similar for MSCAPI. Then we may be able to call the keyLength()
> method directly.
>
> But as MSCPI is only available for Windows, we may run into compiling
> problem on non-windows platforms.
>
>
> Another solution is to define a new interface:
> package sun.security.util;
> public interface Measurable {
> public int getLength();
> }
>
> And updated P11Key and MSCPI Key accordingly to implement the
> Measureable interface:
> package sun.security.pkcs11;
> ...
> - abstract class P11Key implements Key, Measurable {
> + abstract class P11Key implements Key {
> ...
> - int keyLength() {
> + public int getLength() {
> ...
> }
>
> It is similar for MSCAPI Key.
>
> Xuelei
>
> On 11/18/2011 10:43 PM, Weijun Wang wrote:
>>
>>
>> On 11/18/2011 10:38 PM, Xuelei Fan wrote:
>>> On 11/18/2011 10:16 PM, Weijun Wang wrote:
>>>>
>>>> On 11/18/2011 08:39 PM, Xuelei Fan wrote:
>>>>> The VPN is awesomely bad today.
>>>>>
>>>>> The webrev in cr.openjdk is
>>>>> http://cr.openjdk.java.net/~xuelei/7106773/webrev.01/
>>>>>
>>>>> On 11/18/2011 7:39 PM, Weijun Wang wrote:
>>>>>> Oh, there seems to be much more non-trivial code changes then the
>>>>>> previous version of webrev.
>>>>>>
>>>>>> I now focus on the utility class KeyLength.java:
>>>>>>
>>>>>> In ReflectiveProvider, how about adding a "." at the end of package
>>>>>> name?
>>>>>>
>>>>> It's safer.
>>>>>
>>>>>> Also, is the while loop really necessary in getKeyLengthMethod? Why
>>>>>> not
>>>>>> directly call getDeclaredMethod on the the baseClassName? If you
>>>>>> want to
>>>>>> make sure the current class is a subclass of the baseClassName, try
>>>>>> isAssignableFrom.
>>>>>>
>>>>> It's a little strange of the implement of getKeyLengthMethod() because
>>>>> of the following causes:
>>>>> 1. Class.getDeclaredMethod() cannot return inherited methods.
>>>>>
>>>>> 2. Class.forName("sun.security.pkcs11.P11Key") is not reliable.
>>>>> The current class loader is not always able to find the P11Key
>>>>> class.
>>>>
>>>> Oh, In which case?
>>>>
>>> In the previous webrev, I found the case while running
>>> test/sun/security/pkcs11/KeyStore/ClientAuth.java while call
>>> Class.forName() with the current class loader.
>>>
>>>>> As you saw in the previous webrev, I specified the class loader
>>>>> while
>>>>> searching for the class:
>>>>> Class.forName("sun.security.pkcs11.P11Key", true,
>>>>> key.getClass().getClassLoader());
>>>>>
>>>>> There is one assumption that every key will come from the same
>>>>> class
>>>>> loader during the JVM life. So we can cache the loaded P11Key class.
>>>>> It's a little risky.
>>>>>
>>>>> And more, it's OK when there is only one PKCS11 provider.
>>>>> However,
>>>>> while we adding MSCAPI in, we will have to look through both PKCS11
>>>>> provider and MSCAPI provider in order to select the proper cached key
>>>>> class. If we get a key from none of PKCS11 and MSCAPI, we will continue
>>>>> calling the Class.forName() repeatedly until we are able to encounter
>>>>> the PKCS11 key or MSCAPI key. It's very bad in performance.
>>>>>
>>>>> The calling in the getKeyLengthMethod() is lightweight, and we
>>>>> don't
>>>>> have to worry about the class loader any more.
>>>>
>>>> Calling getDeclaredMethod each time is a waste. How about just
>>>>
>>>> while (clazz != null) {
>>>> if (clazz.getName().equals(baseClassName)) {
>>>> // do sth
>>>> break;
>>>> }
>>>> clazz = clazz.getSuperclass();
>>>> }
>>>>
>>>> Also, you're using methods in reflection. Is it faster to use fields?
>>>>
>>> I also concerns about the override of the target method, although there
>>> is no such override at present. From some reports, it seems that fields
>>> reflection cost more cycles than method reflection.
>>
>> Oh, I thought get a field is simpler.
>>
>>>
>>> Performance is a big issue here. What do you think if we change the get
>>> key length method of pkcs11.P11Key and mscapi.Key for better performance?
>>
>> How do you want to change?
>>
>> -Max
>>
>>>
>>> Thanks,
>>> Xuelei
>>>
>>>> Thanks
>>>> Max
>>>>
>>>>>
>>>>>> As for the test, I was thinking that you can add a unit test on the
>>>>>> KeyLength class itself.
>>>>>>
>>>>> I may add some new and simple tests on KeyLength only.
>>>>>
>>>>> Thanks,
>>>>> Xuelei
>>>>>
>>>>>> Thanks
>>>>>> Max
>>>>>>
>>>>>>
>>>>>> On 11/17/2011 11:14 PM, Xuelei Fan wrote:
>>>>>>> Sean, could you also review the fix? I plan to backport the fix to
>>>>>>> JDK
>>>>>>> 7, so that the key size constraints for both JSSE and CertPath can
>>>>>>> work
>>>>>>> with PKCS11 and MSCPI.
>>>>>>>
>>>>>>> updated webrev:
>>>>>>> http://javaweb.us.oracle.com/~xf138604/bugbios/7106773/webrev.01/
>>>>>>>
>>>>>>> In the new webrev, I add a new KeyLength class under
>>>>>>> sun.security.util.
>>>>>>> I think it may be useful for other models.
>>>>>>>
>>>>>>> On 11/11/2011 2:33 PM, Weijun Wang wrote:
>>>>>>>> SSLContextVersion.java:
>>>>>>>>
>>>>>>>> typo: suported -> supported
>>>>>>>>
>>>>>>> Updated.
>>>>>>>
>>>>>>>> DisabledAlgorithmConstraints.java:
>>>>>>>>
>>>>>>>> When can size == 0? Why it's not allowed?
>>>>>>>>
>>>>>>> It's a line of dummy code in case the get key size method return 0.
>>>>>>>
>>>>>>>> Yes, I think the reflection calls should be wrapped in doPriv
>>>>>>>>
>>>>>>>> Please also consider keys from MSCAPI
>>>>>>>>
>>>>>>> Support MSCAPI now.
>>>>>>>
>>>>>>>> Try looking at sun.misc.SharedSecrets and see if it's better
>>>>>>>> for you. Probably not. That class is used to share non-public
>>>>>>>> methods in public classes. Not the same case here.
>>>>>>>>
>>>>>>>> SignatureAndHashAlgorithm.java:
>>>>>>>>
>>>>>>>> Is it possible to combine expected and signingKey into a
>>>>>>>> single
>>>>>>>> signingKey? Of course that means it might need to be never
>>>>>>>> null
>>>>>>>> and all key alg call it in a consistent way. I wonder if this
>>>>>>>> is OK for you.
>>>>>>>>
>>>>>>> It not always work. For example, the key algorithm may be "EC",
>>>>>>> but the
>>>>>>> expected algorithm may be "ECDSA".
>>>>>>>
>>>>>>>> The RSAKey interface seems not used anywhere.
>>>>>>>>
>>>>>>> Yes, removed.
>>>>>>>
>>>>>>>> How about changing
>>>>>>>>
>>>>>>>> 288 } // Otherwise, cannot determine the key size,
>>>>>>>> prefer the
>>>>>>>> most
>>>>>>>> 289 // perferable hash algorithm.
>>>>>>>>
>>>>>>>> to
>>>>>>>>
>>>>>>>> maxDigestLength = Integer.MAX_VALUE;
>>>>>>>>
>>>>>>>> then you don't need to assign -1 and check for< 0 later.
>>>>>>>>
>>>>>>> Good point.
>>>>>>>
>>>>>>>> I would be glad if lines 302-306 can be further indented 4
>>>>>>>> spaces
>>>>>>>>
>>>>>>>> The comment on lines 268-273 is confusing, because you did
>>>>>>>> calculate
>>>>>>>> maxDigestAlgorithm for 512 bits key later.
>>>>>>>>
>>>>>>>> Personally, I prefer comments on 274-281 and codes on 284-288
>>>>>>>> to be from a smaller keySize to bigger ones, but everything is
>>>>>>>> OK.
>>>>>>>>
>>>>>>> Updated.
>>>>>>>
>>>>>>>> Test:
>>>>>>>>
>>>>>>>> I would like a unit test on
>>>>>>>> DisabledAlgorithmConstraints::getKeySize.
>>>>>>>> Hopefully the test can cover all SecretKey and PrivateKey from
>>>>>>>> all
>>>>>>>> providers with all keysizes.
>>>>>>>>
>>>>>>> Added some new tests for PKCS11 and MSCAPI.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Xuelei
>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Max
>>>>>>>>
>>>>>>>> On 11/11/2011 01:09 PM, Xuelei Fan wrote:
>>>>>>>>> Hi Weijun,
>>>>>>>>>
>>>>>>>>> Are you available to review my fix for CR 7106773? The fix in JSSE
>>>>>>>>> part
>>>>>>>>> is straightforward in that the call to
>>>>>>>>> SignatureAndHashAlgorithm.getPreferableAlgorithm() needs an
>>>>>>>>> additional
>>>>>>>>> Key parameter for RSA key exchanges. The significant change is at
>>>>>>>>> sun/security/util/DisabledAlgorithmConstraints.java. I modified the
>>>>>>>>> code
>>>>>>>>> in order to get the key size of the unextractable key in PKCS#11
>>>>>>>>> device.
>>>>>>>>>
>>>>>>>>> webrev: http://cr.openjdk.java.net/~xuelei/7106773/webrev.00/
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Xuelei
>>>>>>>
>>>>>
>>>
>
More information about the security-dev
mailing list