I find the duplication of parsing code annoyed. I'll see if I can consolidate
them into one.
--Max
> On May 20, 2020, at 1:37 PM, Valerie Peng <valerie.p...@oracle.com> wrote:
>
>
> True, the current handling of the extra bytes in parseKey() and decode() are
> kind of opposite, one does not allow any extra bytes but the other ignores
> all. The trailing bytes may look harmless but simply ignores it may open up
> something unexpected.
>
> Given that this is key related class, I think it's important to do reasonable
> validation even though we currently do not use these trailing bytes. It'd
> also be good if the handling can be consistent regardless of the call path.
>
> Thanks,
> Valerie
> On 5/18/2020 9:36 PM, Weijun Wang wrote:
>>> 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
>>>>>>
