[RFC][PATCH][icedtea-web]: Added support for signed JNLP file- Updated Patch

[Omair Majid]

On 07/28/2011 04:53 PM, Saad Mohammad wrote:
> On 07/28/2011 03:01 PM, Omair Majid wrote:
>> On 07/28/2011 02:33 PM, Saad Mohammad wrote:
>>> On 07/25/2011 12:30 PM, Omair Majid wrote:
>>>> On 07/25/2011 11:32 AM, Saad Mohammad wrote:
>>>>> On 07/21/2011 09:34 AM, Omair Majid wrote:
>>>>>> On 07/15/2011 10:21 AM, Saad Mohammad wrote:
>>>>>>> I have updated both the patches that includes the change that you
>>>>>>> have
>>>>>>> recommended and requested.
>>>>>>
>>>>>> I haven't looked into this in too much detail, but I have a few
>>>>>> questions (and concerns) after reading this patch. They are included
>>>>>> in line, below.
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> + if (js.anyJarsSigned()) {
>>>>>>> + // If there are any signed Jars, check if JNLP file is signed
>>>>>>> +
>>>>>>> + if (JNLPRuntime.isDebug())
>>>>>>> + System.out.println("STARTING check for signed JNLP file...");
>>>>>>> +
>>>>>>> + for (int i = 0; i< jars.length; i++) {
>>>>>>> + List<JARDesc> eachJar = new ArrayList<JARDesc>();
>>>>>>> + JarSigner signer = new JarSigner();
>>>>>>> + eachJar.add(jars[i]); // Adds only the single jar to check
>>>>>>> + // if the jar has a valid signature
>>>>>>> +
>>>>>>
>>>>>> There is already JarSigner object that JNLPClassLoader is using. Is
>>>>>> there a reason for creating this new JarSigner object?
>>>>>
>>>>> I can use the JarSigner object that already exists.
>>>>>
>>>>
>>>> Well, I am not sure if that's the right thing. Perhaps it maintains
>>>> some state that you would not be fine with? I was really hoping you
>>>> would explain why you are using a new JarSigner object :)
>>>
>>> I will not need the new JarSigner object because the only purpose of it
>>> is verifying individual jars. I thought every time you wanted to verify
>>> a jar with the same JarSigner object, it would append each jar file as
>>> part of the arraylists of unverifiedJars or verifiedJars. Therefore if I
>>> used the JarSigner that already exists, it would break my IF condition
>>> that verifies the signature of each jar(allJarsSigned()) because it
>>> would have previous cached jar files that were verified using the same
>>> JarSigner object before.
>>>
>>> However, I was incorrect, JarSigner intializes a new arraylist every
>>> time verifyJars() is called. Therefore, I can still use the method,
>>> allJarsSigned(), for checking the signature of each jar file. That's why
>>> I will not need a new JarSigner object. :)
>>>
>>
>> Given that js is an instance variables of the class, things may go
>> wrong if js is used somewhere and that code assumes js contains all
>> jars. Even if it's quite safe, the internal state modification means I
>> am leaning towards a new JarSigner.
>>
>>>>
>>>>>>
>>>>>>> + try {
>>>>>>> + signer.verifyJars(eachJar, tracker);
>>>>>>> +
>>>>>>
>>>>>> The jarsigner js has already verified a subset of these earlier. Do
>>>>>> you really want to verify everything again?
>>>>>
>>>>> This is the method that is checking each jar file individually. So
>>>>> 'eachJar' stores only one jar file at a time and then checks if
>>>>> that jar
>>>>> file is signed or not. It then uses allJarsSigned() to validate if the
>>>>> jar file is signed. I added this because I did not find a solution
>>>>> that
>>>>> can track whether an individual jar file is signed or not. The
>>>>> previous
>>>>> check that JarSigner does uses all the jar resources (passed as
>>>>> parameter) and does not keep track which jar files are signed and
>>>>> which
>>>>> are not. Unless I am mistaken and there is a way of determining this.
>>>>>
>>>>
>>>> Ah, that makes sense now. I suppose you only want to make sure that
>>>> only a signed jar file contains the signed jnlp file.
>>>
>>> I did a bit more of research. The link you have sent me & some other
>>> website mentions that the jar file with the main class must be the ONLY
>>> one that is checked for a signed JNLP file.
>>>
>>
>> What if there is no main class? Can you please verify that the
>> proprietary javaws behaves this way?
>
> Sorry, I am not sure what you exactly mean by having 'no main class'.
> Wouldn't javaws have nothing to start and fail at initialization?
> Therefore a check for signed JNLP file will not be executed.
> So definitely, a JNLP file would need a main class that will be called
> to run the application. So a main class is a requirement, right? Please
> correct me if I am mistaken.
>

Oh, a main class is needed to run alright. If a main class is not found, 
javaws will fail to start. But it may not be clear which jar this main 
class is coming from. Take a look at Launcher.launchApplication() for 
the gory details. You might be surprised to see what the classloader 
does on ClassLoader.loadClass(mainName).

>>
>> Anyway, do you intend to post an updated patch to address this?
>> Otherwise, as I mentioned before, your patch will download all jars
>> (which makes the 'lazy' download mechanism useless).
>
> I will update the patch but only for the main jar file (I am guessing
> the 'main jar file' means the jar file that has the main class) If the
> main jar file is 'lazy', it still needs to be downloaded first and
> checked right away. In this case, I will just need to update the patch
> and force it to only download the main jar file regardless to whether
> it's 'lazy' or 'eagar'.
>
> There is a bug for the 'lazy' download mechanism that I will need to
> address after this (PR765)
> [http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=765].
>
> Before I do this, I am going to confirm that it only checks the main jar
> file for a signed JNLP file and not the other signed jar files that may
> be used as a resource. If it does NOT ONLY check the main jar file, then
> I will update the patch that will download only 'eagar' type jar files
> and check them for signed JNLP file. I will then need to add the check
> for signed JNLP file for 'lazy' jar files with the PR765 fix.
>

That sounds like a very sensible plan.

Cheers,
Omair