Code Review Request for 7146728 and 7130959

[Michael StJohns]

Sorry for the late comment here -

It may make sense to make a utility function with the calling sequence similar to 

static byte[] getBytesFixedLength (BigInteger x, int numBits)
        throws IllegalArgumentException

some where in the security utility code.   

The length of the returned byte array is ceiling(numBits/8).  

Throws IAE if x is negative or zero or numBits is < x.bitLength().

Put the various comments in the utility function instead of spreading them throughout the code.

This function should be used for both ECDH and DH.

Rethrow the IAE as a ProviderException  if it occurs (it shouldn't for DH or ECDH generated secrets).

Mike


At 07:01 PM 3/14/2012, Brad Wetmore wrote:
>Here's the comment I was trying to suggest in your office yesterday:
>
>        /*
>         * NOTE: BigInteger.toByteArray() returns a byte array containing
>         * the two's-complement representation of this BigInteger with
>         * the most significant byte in the zero-th element. This
>         * contains the minimum number of bytes required to represent
>         * this BigInteger, including at least one sign bit whose value
>         * is always 0.
>         *
>         * Keys are always positive, and the above sign bit isn't
>         * actually used when representing keys.  (i.e. key = new
>         * BigInteger(1, byteArray))  To obtain an array containing
>         * exactly expectedLen bytes of magnitude, we strip any extra
>         * leading 0's, or pad with 0's in case of a "short" secret.
>         */
>
>Hope this helps,
>
>Brad
>
>
>
>On 3/9/2012 12:57 PM, Valerie (Yu-Ching) Peng wrote:
>>
>>Ok, more comments/changes added as suggested.
>>Webrev updated at:
>>http://cr.openjdk.java.net/~valeriep/7146728/webrev.02/
>>
>>Thanks,
>>Valerie
>>
>>On 03/08/12 17:15, Brad Wetmore wrote:
>>>
>>>>> DHKeyAgreement:
>>>>> ===============
>>>
>>>> I added more comments to both code blocks to explain what's going on.
>>>
>>>A couple more requested comments.
>>>
>>>Around line 299, could please add that the sign bit is always 0.
>>>
>>>Around line 303, could you put in a comment that since there is enough
>>>room here for the sign bit in this array, it just becomes a leading zero.
>>>
>>>>Good point, I modified the engineGenerateSecret() to leverage
>>>>engineGenerateSecret(byte[], int) method.
>>>
>>>Great, thanks!
>>>
>>>>>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.
>>>
>>>Much better, thanks!
>>>
>>>Minor nit (take or leave) you could adjust the logic around 208 to
>>>only create newSecret if secret.length < secretLen.
>>>
>>>if (secret.length > secretLen) {
>>>throw new ProviderException(...
>>>}
>>>byte [] newSecret = new byte[secretLen];
>>>System.arraycopy(...);
>>>return newSecret;
>>>
>>>
>>>>>TestShort.java
>>>>>==============
>>>>>New bugid
>>>>>
>>>>>@bug 4942494 7146728
>>>>Done.
>>>>
>>>>>Indention problem around the try.
>>>>Fixed.
>>>
>>>Thanks for taking out the commented out code, I almost mentioned it
>>>and then didn't.
>>>
>>>Brad