[RFC][icedtea-web]: PR1049 fix - extension jnlp with empty jars

[Saad Mohammad]

Hi Adam,

Thanks for looking into the patch, I have a few comments below.
Updated patch is attached.

CHANGELOG:

2012-07-23  Saad Mohammad  <smohammad at redhat.com>

	* netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java (initializeResources):
	Removes the display of the security dialog for loaders with only empty jars.
	* netx/net/sourceforge/jnlp/tools/JarCertVerifier.java:
	(JarCertVerifier): Tracks whether all jars verified are empty jars.
	(hasAllEmptyJars): Returns true if all jars verified are empty jars.
	(verifyJars): Checks whether signable entries and certificates are found and
	decides if all jars are empty jars.
	(isFullySignedByASingleCert): If all jars are emptyJars, returns true.
	* tests/reproducers/signed/EmptySignedJar/resources/EmptySignedJar.jnlp:
	Launching jnlp with the resource of a the main jar and an extension jnlp.
	* tests/reproducers/signed/EmptySignedJar/resources/EmptySignedJarExtension.jnlp:
	Extension jnlp containing only an empty jar.
	* tests/reproducers/signed/EmptySignedJar/srcs/META-INF/emptyFile:
	Empty file within META-INF that is required to generate EmptySignedJar.jar
	by the test engine.
	* tests/reproducers/signed/EmptySignedJar/testcases/EmptySignedJarTest.java:
	Testcase that tests jnlp files with empty jars.
	* tests/reproducers/signed/SignedJarResource/resources/SignedJarResource.jnlp:
	Launches SignedJarResource class directly.

[snip]
> Reproducer patch:
> 
[snip]
>> diff --git a/tests/reproducers/signed/EmptySignedJar/testcases/EmptySignedJarTest.java b/tests/reproducers/signed/EmptySignedJar/testcases/EmptySignedJarTest.java
>> new file mode 100644
>> --- /dev/null
>> +++ b/tests/reproducers/signed/EmptySignedJar/testcases/EmptySignedJarTest.java
>> @@ -0,0 +1,66 @@
>> +/* EmptySignedJar.java
>>
>> +> [snipped]
>> +
>> +import java.util.Arrays;
>> +import java.util.Collections;
>> +import java.util.List;
>> +import net.sourceforge.jnlp.ServerAccess;
>> +import net.sourceforge.jnlp.annotations.Bug;
>> +
>> +import org.junit.Assert;
>> +import org.junit.Test;
>> +
>> +public class EmptySignedJarTest {
>> +
>> +    private static ServerAccess server = new ServerAccess();
>> +    private final List<String> l = Collections.unmodifiableList(Arrays.asList(new String[] { "-Xtrustall" }));
>> +    private final String jarOutput = "Running SignedJarResource..";
>> +
>> +    @Test
>> +    public void checkingForRequiredResources() throws Exception {
>> +        String s = "Running SignedJarResource..";
>> +        ServerAccess.ProcessResult pr = server.executeJavawsHeadless(l, "/SignedJarResource.jnlp");
>> +        Assert.assertTrue("Could not locate SignedJarResource class within SignedJarResource jar", pr.stdout.contains(s));
>> +    }
>> +
>> +    @Bug(id = "PR1049")
>> +    @Test
>> +    public void usingExtensionWithEmptyJar() throws Exception {
>> +        ServerAccess.ProcessResult pr = server.executeJavawsHeadless(l, "/EmptySignedJar.jnlp");
>> +        Assert.assertTrue("Stdout should contain " + jarOutput + " but did not", pr.stdout.contains(jarOutput));
>> +    }
>> +}
> This test 'passes' with HEAD ? It does bring up a pop up window, however 
> this is not a proper test if it claims a pass if you close the window...

Thanks for pointing this out. The new patch resolves this issue.

[snip]
> Patch itself:
> 
> 
>> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> @@ -600,7 +600,7 @@
>>                      file.setSignedJNLPAsMissing();
>>                  
>>                  //user does not trust this publisher
>> -                if (!jcv.getAlreadyTrustPublisher()) {
>> +                if (!jcv.getAlreadyTrustPublisher() && !jcv.hasAllEmptyJars()) {
>>                      checkTrustWithUser(jcv);
>>                  } else {
>>                      /**
>> diff --git a/netx/net/sourceforge/jnlp/tools/JarCertVerifier.java b/netx/net/sourceforge/jnlp/tools/JarCertVerifier.java
>> --- a/netx/net/sourceforge/jnlp/tools/JarCertVerifier.java
>> +++ b/netx/net/sourceforge/jnlp/tools/JarCertVerifier.java
>> @@ -103,6 +103,16 @@
>>  
>>      private int totalSignableEntries = 0;
>>  
>> +    /** Whether all jars are empty (jars with no content or only META-INF/*) */
>> +    private boolean allEmptyJars = false;
> I think this isn't something JCV should concern itself with. 
> I think a better solution would be to check if there are non-META_INF/ entries, and only run JarCertVerifier then.
> Or, as an alternative, you could remain this to 'triviallySigned', to align it more with the intent of JCV.
> 
I went with your alternative solution and changed the method and variable names
instead. I think JCV handling this check is fine since it already keeps track of
the number of sign-able entries found in each jar. It would be redundant to have
another class go through a similar check.

[snip]

Thanks again!

-- 
Cheers,
Saad Mohammad


-------------- next part --------------
A non-text attachment was scrubbed...
Name: changelogEntry-2.patch
Type: text/x-patch
Size: 1514 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120723/a7866f10/changelogEntry-2.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR1049-2.patch
Type: text/x-patch
Size: 2385 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120723/a7866f10/PR1049-2.patch 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: reproducers_2.patch
Type: text/x-patch
Size: 11769 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120723/a7866f10/reproducers_2.patch