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

[Weijun Wang]

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?

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.

As for the test, I was thinking that you can add a unit test on the 
KeyLength class itself.

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
>