code review request: 7081783: jarsigner error when no $HOME/.keystore

[Xuelei Fan]

I understand the code, fine to me. But the loadKeyStore() method looks
really ugly and lazy. :-)

Just for your reference in the inline comments.

On 8/31/2011 4:00 PM, Weijun Wang wrote:
> 
> 
> On 08/30/2011 09:52 PM, Xuelei Fan wrote:
>> 1. Do you want to add more debug info? line 1509-1511:
>> +            if (debug) {
>> +                e.printStackTrace();
>> +            }
> 
> Yes. The -debug option is used to show exception full stack when an
> error occurs. I want the exception printed for warnings also.
> 
>>
>> 2. It looks a little strange to me that even if there is Runtime
>> exception, we still do some additional work in final block.
> 
> Well, the handling of exception at signing and verification are
> different (compare line 212 and 227). There must be a keystore at
> signing time but not necessary at verification. Therefore, the exception
> handling is outside of this method.
> 
>>
>> 3. I try hard to understand the code how to solve the issue. It is not
>> easy for me to understand why the update works.
> 
> It's because for verification the exception is ignored in the test case
> (line 214-218), so even if ~/.keystore does not exist, the new finally
> block makes sure the CertPathValidator is properly initialized. Before
> this fix, although the exception is also ignored, the CertPathValidator
> lines are totally ignored, and the CertPath validation shows an NPE and
> a warning is shown.
> 
>> I'm thinking, is it
>> possible to ignore user_home/.keystore instead of try to load it when
>> the file does not exist (File.exists() or catch FileNotFoundException,
>> etc)?
>>
>>     The current logic is, "loading keystore from user_home/.keystore".
>> Can we change to use a refined logic, "if user_home/.keystore exists, we
>> load the keysore; otherwise, ignore it". I'm not sure the new logic get
>> the code more complicated, or more intuitive.
> 
> Well, it looks more correct, but is complicated in 2 senses:
> 
> 1. ~/.keystore and user-specified -keystore are not treated the same.
> You can ignore ~/.keystore, but if a user-specified -keystore does not
> exist, it's an error.
> 
We can handle the logic simply at the following blok, roght?
1562 if (!nullStream && keyStoreName == null) {
1563     keyStoreName = System.getProperty("user.home") + File.separator
1564         + ".keystore";
+        // check file existence, ignore it if non-exist
1565 }

> 2. signing and verification have different behaviors on exception
> handling. See above.
> 
We don't need to make more significant update in other blocks because of
we have ignore the non-exist keystore, right? If no required private
key, the following steps will throw the expected exception.

Or is there any other thing that I missed?

Thanks,
Xuelei

> Therefore I prefer my current code changes more. It's focused on only
> one place where the bug is: make sure CertPathValidator is always
> initialized, and leave all old logic unchanged.
> 
> Thanks
> Max
> 
>>
>> Xuelei
>>
>> On 8/30/2011 12:56 PM, Weijun Wang wrote:
>>> Hi All
>>>
>>> 7081783: jarsigner error when no $HOME/.keystore
>>>
>>> Webrev is at --
>>>     http://cr.openjdk.java.net/~weijun/7081783/webrev.00/
>>>
>>> Description:
>>>
>>> jarsigner includes a certpath validation check, and shows a warning when
>>> the check fails. The CertPathValidator object, unfortunately, is
>>> initialized in a method that can only be executed if a local keystore is
>>> found (either ~/.keystore or specified by -keystore). Therefore, if
>>> there is no local keystore but the jarfile's signer can be directly
>>> verified by a cert in cacerts, we still see:
>>>
>>>     Warning:
>>>     This jar contains entries whose certificate chain is not validated.
>>>
>>> The code changes make sure the CertPathValidator object is always
>>> initialized.
>>>
>>> For reg test, it's a simple call --
>>>
>>> ${TESTJAVA}${FS}bin${FS}jarsigner \
>>>          -J-Duser.home=. \
>>>          -verify -strict ${TESTSRC}${FS}bootstrap.jar
>>>
>>> Here I override user.home so that even if the test machine has a
>>> ./keystore, it won't be affected. The bootstrap.jar file is a small
>>> signed jar that is signed by a real CA that can be chained into an item
>>> in cacerts.
>>>
>>> Thanks
>>> Max
>>>
>>