RFR 8157404: Unable to read certain PKCS12 keystores from SequenceInputStream

[Weijun Wang]

Yes, that's why I said it's not implemented correctly, but it has worked 
fine until this bug appears.

--Max

On 8/31/2016 9:44, Bernd Eckenfels wrote:
> Hello,
>
> readBytes read all bytes of a specified length. If the length is not
> known it is not usefull to use. Besides it is also not usefull to only
> read available() bytes, as the nmber of bytes read might be up to all
> kinds of factors (including io request and tcp segment boundaries).
>
> It is not a valid abstraction to model records basd on read sizes. I
> think using available() is almost always a bug. (But I am not sure what
> the specific datastructures involved are).
>
> Gruss
> Bernd
>
>
>
> Am Wed, 31 Aug 2016 09:18:23 +0800
> schrieb Weijun Wang <weijun.wang at oracle.com>:
>>
>>
>> On 8/31/2016 0:46, Svetlana Nikandrova wrote:
>>> Hi Max,
>>>
>>> thank you for your replay. Please see inline.
>>>
>>> On 30.08.2016 5:43, Weijun Wang wrote:
>>>> readAllBytes() wants to real *all* data. If this is during a long
>>>> time communication, and the peer sends a BER using indefinite
>>>> length, and then wait for the next round of dialog without closing
>>>> the stream, will readAllBytes() hang?
>>>
>>> readAllBytes() blocks until all remaining data has been read and
>>> end of stream is detected.
>>> So if I understood your scenario correctly, yes, if EOF is not
>>> present it will hang. AFAIK when KeyStore.load completes
>>> successfully it consumes the input stream until EOF, so isn't it ok
>>> to expect it?
>>
>> The KeyStore::load method makes no assumption on the InputStream
>> parameter. More important, DerValue is used much wider than pkcs12
>> keystore.
>>
>>>
>>>> Is it possible to create a new method readAllAvailables() here? It
>>>> kept read data until available() is zero and return the
>>>> concatenated byte array. I think this method does not block and is
>>>> able to read everything available from a SequenceInputStream.
>>>
>>> Yes, I think it is possible to read data by portions, but
>>> unfortunately straight forward approach like
>>> "while(is.available() !=0)" won't work. As soon as we reach the end
>>> of the first stream in SequenceInputStream sequence is.available()
>>> will return 0 until we switch to the next stream by calling read().
>>> I'm afraid it will look a little bit confusing.
>>
>> Well, this is unfortunate.
>>
>> I also cannot think of a simple fix now. Maybe after see that "not
>> all indef len BER resolved" error you can try read more.
>>
>>>
>>>> In fact, IMO the DerValue is not implemented correctly. It should
>>>> have been able to read the input data chunk by chunk until a BER
>>>> is fully consumed, leaving data after the BER untouched, and block
>>>> if the BER is not complete. To achieve this, it should read the
>>>> header, read zero or more sub-data, read the EOC, recursively if
>>>> necessary.
>>>
>>> It asks for some investigation. Should I create a separate issue?
>>
>> You can, but please set the target version to major-tbd unless you
>> want to work on it now.
>>
>> Thanks
>> Max
>>
>>>
>>> Thank you,
>>> Svetlana
>>>
>>>>
>>>> Thanks
>>>> Max
>>>>
>>>> On 8/30/2016 2:52, Artem Smotrakov wrote:
>>>>> Hi Svetlana,
>>>>>
>>>>> It looks fine, but I am not an official reviewer.
>>>>>
>>>>> "keystorePath" in readTest() can be a static field.
>>>>>
>>>>> I also meant that one test with SequenceInputStream seems to be
>>>>> enough, so you could just add a new test case to
>>>>> ReadP12Test.java. But it's fine.
>>>>>
>>>>> I am not sure how DerIndefLenConverter works, but it looks a
>>>>> little strange to me that it needs to extend an array before
>>>>> passing it to DerIndefLenConverter. I see that convert() method
>>>>> also uses arraycopy() method. But it seems to be out of scope
>>>>> here.
>>>>>
>>>>> Artem
>>>>>
>>>>>
>>>>> On 08/29/2016 11:23 AM, Svetlana Nikandrova wrote:
>>>>>> Hi Artem,
>>>>>>
>>>>>> thank you for your replay. I've updated copyright and made
>>>>>> separate test for this bug.
>>>>>> As for Arrays.copyOfRange() unfortunately it won't simplify code
>>>>>> in my case. I need to extend an array, not to get a sub-array of
>>>>>> existing one.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~snikandrova/8157404/webrev.01/
>>>>>> <http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.01/>
>>>>>>
>>>>>> Thanks,
>>>>>> Svetlana
>>>>>>
>>>>>> On 26.08.2016 23:48, Artem Smotrakov wrote:
>>>>>>>
>>>>>>> Hi Svetlana,
>>>>>>>
>>>>>>> DerValue class may be implicitly used in different areas (x509,
>>>>>>> SSL/TLS, keystores, maybe krb5, etc). Please make sure that
>>>>>>> tests from jdk_security pass.
>>>>>>>
>>>>>>> I'll leave the main review to someone who is more knowledgeable
>>>>>>> in this area, here are a couple of comments:
>>>>>>> - Please update copyright year
>>>>>>> - You may want to replace new byte[] + System.arraycopy() by
>>>>>>> Arrays.copyOfRange()
>>>>>>> - It may be better to add a separate test case in
>>>>>>> ReadP12Test.java for SequenceInputStream instead of loading a
>>>>>>> keystore twice in each call to readTest(). One test with
>>>>>>> SequenceInputStream seems to be enough, and it would make the
>>>>>>> logic of readTest() clearer.
>>>>>>>
>>>>>>> Artem
>>>>>>>
>>>>>>> On 08/26/2016 10:58 AM, Svetlana Nikandrova wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> please review this fix. It's not possible to read PKCS12
>>>>>>>> keystore with big undefined length DER value in it from
>>>>>>>> SequenceInputStream. Root cause of the problem is that
>>>>>>>> sun.security.util.DerValue relays on InputStream.available()
>>>>>>>> to get a complete 'indefinite.length' section length and then
>>>>>>>> read it, but for SequenceInputStream this method returns
>>>>>>>> number of available bytes only for current input stream, not
>>>>>>>> the whole sequence. Fixed to read all available data.
>>>>>>>>
>>>>>>>> JBS:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8157404
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~snikandrova/8157404/webrev.00/
>>>>>>>> <http://cr.openjdk.java.net/%7Esnikandrova/8157404/webrev.00/>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Svetlana
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>