Code Review Request for 7196805: DH Key interoperability testing between SunJCE and JsafeJCE not successful

[Valerie (Yu-Ching) Peng]

Max,

Please find comments in line...

On 06/12/13 18:27, Weijun Wang wrote:
> Hi Valerie
>
> Not sure if we really need to fix the equals() method here. Is there a 
> spec saying the equality is based on internal fields instead of 
> encoded bytes?
There is no spec saying the equality check should be based on encoded 
bytes neither.
DH keys are different from other keys in that its parameters contains an 
optional field. Thus, even for 2 DH public keys whose internal fields 
are identical (including the optional one), they could still have 
different ASN.1 encodings, i.e. one includes the optional value vs one 
does not. I think the reasonable expectation would be that if all 
non-optional fields match, then the 2 keys are equal. Otherwise, it's 
hard to predict the behavior since DH keys can be constructed w/ or w/o 
the optional field and there are mixed usages in our JDK code base.

> I am saying this because it's not easy to get equals() correct here. 
> In DHPrivateKey.java, the new equals() requires the other object to be 
> instanceof javax.crypto.interfaces.DHPrivateKey. I see that another 
> class P11DHPrivateKey also implements this interface but has not 
> overridden equals, so it looks like a 
> com/sun/crypto/provider/DHPrivateKey could equals() to a 
> P11DHPrivateKey, but a P11DHPrivateKey will not equals() to a 
> com/sun/crypto/provider/DHPrivateKey.
>
> Same with DHPublicKey.java.
I will also apply the same change to P11DHPrivateKey/P11DHPublicKey 
then. Equality check using ASN.1 encoding is fine for non-DH algorithms 
but not for DH.

>
> BTW, the hashCode() methods in these 2 classes use <<2 and <<4 which 
> looks not as cool as the normal "*31+". I would simply use 
> Objects.hash(....) method.
Ok, I am fine with just using Objects.hash(Object... values) method.

>
> For DHKeyPairGenerator.java, it looks like you don't want the first 
> octet being zero. Is this related to this bug? Is that required in the 
> "Handbook of Applied Cryptography" book? I understand it could be 
> necessary for interop.
The change is for conforming to the description under section 7.1 
"Private-value generation" of PKCS#3 DH Key Agreement Standard 
ftp://ftp.rsasecurity.com/pub/pkcs/ascii/pkcs-3.asc , i.e.

An integer x, the private value, shall be generated
privately and randomly. This integer shall satisfy 0<  x<
p-1, unless the central authority specifies a private-value
length l, in which case the integer shall satisfy 2^(l-1)<=
x<  2^l.

I will re-test everything and let you know once I have the webrev updated.
Thanks,
Valerie
>
> Thanks
> Max
>
> On 5/29/13 9:25 AM, Valerie (Yu-Ching) Peng wrote:
>> Vinnie,
>>
>> Can you help reviewing my fix for 7196805 "DH Key interoperability
>> testing between SunJCE and JsafeJCE not successful"?
>>
>> In SunJCE provider, the equality check for DH private/public keys is
>> based on DER encoding which may not be correct all the time due to the
>> optional L value defined in the DER syntax. In addition, JsafeJCE
>> provider sometimes encode the optional L value incorrectly which leads
>> to unexpected IOException when parsing the DER bytes.
>> I have changed the comparison to based on component values rather than
>> DER encodings which may vary due to the presence or missing of optional
>> values. In addition, I made the changes to DHKeyPairGenerator to ensure
>> that generated private value has the requested length/size.
>>
>> Webrev: http://cr.openjdk.java.net/~valeriep/7196805/webrev.00/
>>
>> Thanks,
>> Valerie