Code Review Request for 7146728 and 7130959

Fri Mar 9 01:22:48 UTC 2012

Looks good, better to be explicit about these values than prepend to an 
unknown list.

Thanks,

Brad

On 3/6/2012 7:37 PM, Valerie (Yu-Ching) Peng wrote:
>
> 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
>>>>>>
>>>>>>
>>>>
>>
>