RFR 8172975: SecurityTools.keytool() needs to accept user input

[Artem Smotrakov]

Hi Max,

Please see inline.


On 01/19/2017 05:15 PM, Weijun Wang wrote:
>
>
> On 01/19/2017 09:40 PM, Artem Smotrakov wrote:
>> Hi Max,
>>
>> In general, looks okay.
>>
>> Would it be better if it called redirectInput() only if the response
>> file exists? keytool() method might also delete the response file after
>> reading it. These two measures may prevent situations when the response
>> file is unnecessary used.
>
> Yes, it's good to remove the file after reading it, so this means 
> people need to set it for every call.
>
> On the other hand, I still think creating an empty file and call 
> redirectInput() on it is necessary when the response file does not 
> exist. This prevents the keytool() call from hanging forever if it 
> actually needs user input but the test has not provided any.
It looks like a test issue with particular test, I don't remember many 
tests which failed because of this.
>
> Also, I am feeling that the jarsigner-related calls are quite 
> complicated. I suggest we use the same method for signing and 
> verifying and ask the user to provide all arguments in their order. So 
> instead of calling
>
>    SecurityTools.jarsigner("x.jar", "alias", "...");
>
> we just call
>
>    SecurityTools.jarsigner("... x.jar alias");
This looks better to me as well.

I am fine with current webrevs.

Artem
>
> This is consistent with keytool(), and we can also provide 3 
> overloaded forms.
>
> What do you think? :-)
>
> Thanks
> Max
>
>>
>> What do you think?
>>
>> Artem
>>
>>
>> On 01/18/2017 05:50 PM, Weijun Wang wrote:
>>> Please review the code changes at
>>>
>>>    root: http://cr.openjdk.java.net/~weijun/8172975/root/webrev.00/
>>>    jdk: http://cr.openjdk.java.net/~weijun/8172975/webrev.00/
>>>
>>> The fix is in root repo. This is not an elegant solution because it
>>> uses a separate method to provide the response. This means you have to
>>> clear the response if the next keytool call does not need it. This
>>> also means you cannot run keytool in multiple threads.
>>>
>>> I didn't provide the response as an extra argument because there are
>>> already many overloaded keytool() methods, and I do not want to invent
>>> a new method name (say, keytoolWithResponse) and implement the same
>>> number of overloaded methods.
>>>
>>> If you are strongly against this solution, I'll think of another way.
>>>
>>> The jdk change includes a test for this change, as well as a trivial
>>> fix for keytool's getYesNoReply() method. Otherwise an NPE is thrown
>>> with the following command:
>>>
>>>    cat untrusted.cert | keytool -importcert -alias a
>>>
>>> Thanks
>>> Max
>>