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
