RFR 8244565: Accept PKCS #8 with version number 1

[Valerie Peng]

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");
             }
         }
     }

Besides the encoding parsing check above, maybe V1, V2 can be made 
private static? 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.

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.

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.wang at 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
>>>