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

[Xuelei Fan]

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