Code review request, 7106773: 512 bits RSA key cannot work with SHA384 and SHA512
Xuelei Fan
xuelei.fan at oracle.com
Sat Nov 19 02:26:32 UTC 2011
>> 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