> On May 28, 2020, at 8:46 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
>
> Hi Max,
>
> I like this new structure better. Much easier to understand. Most of the
> changes are technical debt that's accumulated inside PKCS8Key class.
>
> A few notable differences which I am not so sure about are
>
> - the encodedKey is returned by getEncoded() directly w/o cloning, and
Too bad. I'll fix.
>
> - the protected parseKeyBits() method being made private. I wonder if the
> parseKeyBits() method should be made abstract so it's more obvious that the
> subclasses needs to parse the key bytes into algorithm-specific components.
Or how about I just inline parseKeyBits in those child classes into their
constructors? I don't think the child class will forget it. Otherwise, why
write a child class?
Thanks,
Max
>
> Thanks,
> Valerie
> On 5/26/2020 5:54 PM, Weijun Wang wrote:
>> Can you please take a look (not a review) at an updated webrev at
>> http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the code
>> to inspect the extra fields. I haven't deal with the "..." here yet.
>>
>> However, when I tried to consolidate the DER parsing into one place, I've
>> made more and more changes everywhere. The major change now is a base
>> constructor PKCS8Key(byte[]) and subclass constructors that call it and no
>> more protected parseKeyBits(). Although I don't think there is any technical
>> error here but at the end of the day I'm asking myself if this is too much
>> code change for such a simple bug.
>>
>> Do you like the overall structure? If yes, I'll continue this way.
>> Otherwise, I can restrict the change only inside PKCS8Key.
>>
>> Thanks,
>> Max
>>
>>
>>> On May 27, 2020, at 7:14 AM, Valerie Peng <valerie.p...@oracle.com> wrote:
>>>
>>> I suppose we can allow trailing data to conform to "..." then.
>>>
>>> But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the
>>> publicKey should not be present for V1? This should be checked?
>>>
>>> Valerie
>>>
>>> On 5/25/2020 9:02 PM, Weijun Wang wrote:
>>>> The new definition at https://tools.ietf.org/html/rfc5958 shows,
>>>>
>>>> OneAsymmetricKey ::= SEQUENCE {
>>>> version Version,
>>>> privateKeyAlgorithm PrivateKeyAlgorithmIdentifier,
>>>> privateKey PrivateKey,
>>>> attributes [0] Attributes OPTIONAL,
>>>> ...,
>>>> [[2: publicKey [1] PublicKey OPTIONAL ]],
>>>> ...
>>>> }
>>>>
>>>> According to
>>>> https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:
>>>>
>>>> The string "..." in ASN.1 is called an extension marker. The extension
>>>> marker,
>>>> ellipsis, is an indication that extension additions are expected. It
>>>> makes no
>>>> statement as to how such additions should be handled. However they
>>>> shall not be
>>>> treated as an error during the decoding process.
>>>>
>>>> So there might be more fields in the future. Do you still insist we need
>>>> more validation?
>>>>
>>>> Thanks,
>>>> 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
>>>>>>>>>>
