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

[Weijun Wang]

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.

2. signing and verification have different behaviors on exception 
handling. See above.

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