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

Weijun Wang weijun.wang at oracle.com
Fri Jan 20 09:23:59 UTC 2017



On 01/20/2017 05:01 PM, Artem Smotrakov wrote:
> 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.

It's certainly a test issue, but I'd like it to fail fast.

There were no test failing like this now. I notice this because I am 
working on JDK-8171319 to add more warnings and promptings to keytool 
and several tests timeout.

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

A little confused. Do you think the current webrev is fine or is it 
better to rewrite jarsigner()?

Thanks
Max

>
> 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
>>>
>


More information about the security-dev mailing list