Code Review Request for 7146728 and 7130959

Wed Mar 7 03:37:30 UTC 2012

BTW, have you looked at the fixes of 7130959?
> * 7130959: Tweak 7058133 fix for JDK 8 (javah makefile changes)
> o http://cr.openjdk.java.net/~valeriep/7130959/webrev.00/
>
> Instead of using the -Xbootclasspath, switching over to use
> -boothclasspath for consistency with the backported changes in
> the update releases for earlier JDK, e.g. 7u. 
Thanks,
Valerie

On 03/06/12 19:19, Valerie (Yu-Ching) Peng wrote:
> Brad,
>
> Thanks for the review.
> Please find the update webrev at:
> http://cr.openjdk.java.net/~valeriep/7146728/webrev.01/
>
> On 03/02/12 17:31, Brad Wetmore wrote:
>> DHKeyAGreement.java
>> ===================
>> Mainly just a request for some additional commenting so the reader 
>> doesn't have to have a good understanding of the BigInteger logic to 
>> understand the assumptions being made here:
>>
>>     For example, if the expected bits of the modulus is
>>     1024 bits (128 bytes exactly)
>>
>>     secret-magnitude     secret.length           action
>>     ================     =============           ======
>>     1024                 1025 bits (129 bytes)   trim front byte
>>     1023                 1024 bits (128 bytes)   return
>>                                                  (sign bit (always 0)
>>                                                   becomes leading 0)
>>     ...
>>     1015                 1016 bits (127 bytes)   shift to end of array
>>                                                  (implied leading 0's)
>>     ...
>>
> I added more comments to both code blocks to explain what's going on.
>
>> In the second bit of code, you have the new ProviderException that 
>> you don't have in the first.  I'm not sure how you could get into 
>> this case, if you're % by modulus, but I guess it's good to be 
>> careful.  That said, you should either make consistent with the above 
>> or remove.  You might also combine the actual copy logic into a 
>> single function called from both.
> Good point, I modified the engineGenerateSecret() to leverage 
> engineGenerateSecret(byte[], int) method.
>
>> P11KeyAgreement.java
>> ====================
>>
>>     // Solaris
>>
>> Shouldn't this be "// Solaris/NSS", for the case in which there isn't 
>> a leading zero?
> Well, I think putting NSS in both places seems somewhat confusing. 
> What I tried to point out is that NSS (at least in the older versions, 
> I have not tried the newer version to see if they switched behavior) 
> trims off the leading 0s whenever possible. I've made some 
> modification hoping to make it clearer.
>
>> For performance, it's a shame that engineGenerateSecret(byte[] 
>> sharedSecret, int offset) is doing a second copy here.  Not a request 
>> to do anything, just an observation.
> Well, that's just how it is written from the very beginning. The 
> current impl is more optimized for engineGenerateSecret().
> Probably not worthwhile to re-write the code to get rid of the extra 
> copying.
>
>>
>> DHKeyAgreement2.java
>> ====================
>> >    Security.addProvider(jce)
>>
>> Good grief!  ;)  Still finding these!
>>
>> I think you should include the new bugid.
>>
>>      @bug 7146728
> Done.
>
>> TestShort.java
>> ==============
>> New bugid
>>
>>     @bug 4942494 7146728
> Done.
>
>> Indention problem around the try.
> Fixed.
>
>> Was the change at 69 because the leading zeros were being trimmed?
> Yes, SunPKCS11 provider was explicitly trimming off the leading 0s 
> before this 7146728 fix.
> Now that we decide to preserve the leading 0s, this test vector has to 
> be updated.
>
> Thanks,
> Valerie
>
>> Brad
>>
>>
>>
>>
>>
>>
>> On 3/1/2012 2:03 PM, Valerie (Yu-Ching) Peng wrote:
>>>
>>> Yes, that's the section that mentioned the generated secret having the
>>> same length as p.
>>> I guess it depends on how it's interpreted. The way I look at it is 
>>> that
>>> if the generated secrets aren't of the same length, then whoever using
>>> this variant for deriving the keying material will need to do the extra
>>> work of checking the size of generated secret assuming that they know
>>> the size of p to add back the leading 0s if needed.
>>> I can't find any specification dictating trimming off the leading 0s 
>>> for
>>> the generated secret. Considering both models, preserving vs 
>>> trimming, I
>>> feel the former makes more sense since it is simpler to use and 
>>> provides
>>> more flexibility than the later. RFC 2631 has not be crystal clear on
>>> this I am afraid, that's why there has been some inconsistency in
>>> different vendors implementation since people interpret what they read
>>> differently.
>>> Valerie
>>>
>>> On 02/29/12 23:00, Brad Wetmore wrote:
>>>>
>>>>
>>>> On 2/21/2012 5:33 PM, Valerie (Yu-Ching) Peng wrote:
>>>>> Brad,
>>>>>
>>>>> Can you please review the fixes for the following 2 bugs:
>>>>>
>>>>> * 7146728: Inconsistent length for the generated secret using DH key
>>>>> agreement impl from SunJCE and PKCS11
>>>>> o http://cr.openjdk.java.net/~valeriep/7146728/webrev.00/
>>>>>
>>>>> This impacts both SunJCE provider and SunPKCS11 provider. The
>>>>> implementations are inconsistent within SunJCE provider itself
>>>>> between the engineGenerateSecret() and
>>>>> engineGenerateSecret(byte[], int). Given that RFC 2631 specifies
>>>>> the leading 0s must be preserved so the generated secret has as
>>>>> many octets as the prime P,
>>>>
>>>> Just to be clear here, you're referring to Section 2.1.2 of 2631,
>>>> which is just one of the DH Key agreement variants (based on X9.42)
>>>> for generating Keying Material from secret keys obtained from a "raw"
>>>> DH calculations, and is then subject later SHA1 manipulations, right?
>>>> This method provides motivation/incentive to output our secret keys
>>>> with the same lengths, but I don't think this RFC makes any claims
>>>> that the general output of "raw" DH key agreement operation must be
>>>> the same length.
>>>>
>>>> I'll take another look over the code tomorrow.
>>>>
>>>> Thanks,
>>>>
>>>> Brad
>>>>
>>>>
>>>> I have changed both SunJCE and
>>>>> SunPKCS11 provider to do so. When testing against Solaris and
>>>>> NSS libraries, Solaris preserves the leading 0s while NSS trims
>>>>> it off, thus similar handling is also needed in SunPKCS11 provider.
>>>>>
>>>>> * 7130959: Tweak 7058133 fix for JDK 8 (javah makefile changes)
>>>>> o http://cr.openjdk.java.net/~valeriep/7130959/webrev.00/
>>>>>
>>>>> Instead of using the -Xbootclasspath, switching over to use
>>>>> -boothclasspath for consistency with the backported changes in
>>>>> the update releases for earlier JDK, e.g. 7u.
>>>>>
>>>>> Thanks,
>>>>> Valerie
>>>>>
>>>>>
>>>
>