Updated webrev at https://cr.openjdk.java.net/~weijun/8244565/webrev.03.
I've inlined more methods that is only called once and inside the same method. Thanks, Max > On May 28, 2020, at 9:16 AM, Weijun Wang <weijun.w...@oracle.com> wrote: > > > >> 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 >>>>>>>>>>> >
