> On May 19, 2020, at 1:41 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
>
> Hi Max,
>
> I saw in the bug description that handling and parsing of the public key will
> be resolved in a separate enhancement which is fine with me.
>
> In addition to relaxing the PKCS8 version check to accept 1, the current
> webrev does not check for the trailing bytes and its validity. Perhaps we
> should consider adding necessary checks to ensure spec conformance? Perhaps
> some utility method like: (This includes basic checks plus checks for
> multiple attributes and version 1 w/ public key. )
>
> private static void checkTrailingBytes(DerInputStream derIn, int version)
> throws IOException {
> boolean hasAttributes = false;
> boolean hasPublicKey = false;
> while (derIn.available () != 0) {
> // check for OPTIONAL attributes and/or publicKey
> DerValue tmp = derIn.getDerValue();
> if (tmp.isContextSpecific((byte)0) && !hasAttributes) {
> // OPTIONAL attributes not supported yet
> // discard for now and move on
> hasAttributes = true;
> } else if (version == V2 && tmp.isContextSpecific((byte)1) &&
> !hasPublicKey) {
> // OPTIONAL v2 public key not supported yet
> // discard for now and move on
> hasPublicKey = true;
> } else {
> throw new IOException ("illegal encoding in private key");
> }
> }
> }
I wonder if this is necessary. The extra bytes are quite harmless, and we
didn't check it in decode().
>
> Besides the encoding parsing check above, maybe V1, V2 can be made private
> static?
OK.
> I noticed that the PKCS8Key class has a block of code under the comments "Try
> again using JDK1.1-style...", however the provider property
> "PrivateKey.PKCS#8.<alg>" seems long gone? Without these property, this block
> of code seems useless and probably should be cleaned up as well.
Yes.
>
> As for the test, the existing comments for the EXPECTED bytes are off and
> plain misleading. Could you please fix them or at least remove them. For
> example, the comment of "integer 3" should be associated with "0x02, 0x01,
> 0x03," bytes instead of "0x01, 0x02, 0x02,". In the updated test,
> NEW_ENCODED_KEY_INTS is PKCS8 v1 key and NEW_ENCODED_KEY_INTS_2 is PKCS8 v2
> key w/ public key. Since the version value is always at index 4, we can
> either clone the existing bytes or directly override the version value in
> NEW_ENCODED_KEY_INTS or NEW_ENCODED_KEY_INTS_2 to add additional negative
> tests, e.g. encoding w/ the version value 2 (anything besides the allowed 0
> and 1), version value 0 w/ trailing public key. It'd be nice to test version
> 1 w/ trailing bytes w/ invalid tag value as well.
If you still want checkTrailingBytes, then yes.
Thanks,
Max
>
> Thanks,
>
> Valerie
>
>
> On 5/13/2020 5:16 PM, Valerie Peng wrote:
>> I will take a look.
>>
>> Valerie
>>
>> On 5/8/2020 3:39 AM, Weijun Wang wrote:
>>> Found an existing test I can update. Webrev updated at
>>>
>>> http://cr.openjdk.java.net/~weijun/8244565/webrev.01/
>>>
>>> Thanks,
>>> Max
>>>
>>>> On May 8, 2020, at 5:41 PM, Weijun Wang <weijun.w...@oracle.com> wrote:
>>>>
>>>> Please take a review at
>>>>
>>>> http://cr.openjdk.java.net/~weijun/8244565/webrev.00/
>>>>
>>>> Now we accepts a PKCS8 file with version being 0 or 1. The publicKey and
>>>> attributes are still not parsed.
>>>>
>>>> I also take this chance to make some format change.
>>>>
>>>> There is no regression test and I'll add one. I can generate a PKCS8 key
>>>> and then modify the version from 0 to 1 and try to read it. Or I can find
>>>> a PKCS8 generated by some other tool and embed it the test to read it.
>>>> Please advise which is better.
>>>>
>>>> Thanks,
>>>> Max
>>>>
