[rfc][icedtea-web] Reproducer & unit test for unsigned content in META-INF/
Jiri Vanek
jvanek at redhat.com
Tue Aug 14 02:51:14 PDT 2012
On 08/13/2012 08:30 PM, Adam Domurad wrote:
> On Mon, 2012-08-13 at 10:59 +0200, Jiri Vanek wrote:
>> On 07/05/2012 05:27 PM, Adam Domurad wrote:
>>> Note, bugfix already in HEAD. Reproducer sanity checked by backing out
>>> -r429 in HEAD.
>>>
>>> Reproducer functions by copying and modifying ReadPropertiesSigned.jar
>>> to have unsigned content in META-INF/ directory. If it is felt that it
>>> should sign and create its own jar let me know.
>>>
>>> Unit test changelog:
>>> 2012-07-05 Adam Domurad<adomurad at redhat.com>
>>>
>>> Unit test for method in JCV, isMetaInfFile()
>>> * netx/net/sourceforge/jnlp/tools/JarCertVerifier.java:
>>> Made isMetaInfFile package-private for testing purposes.
>>> * tests/netx/unit/net/sourceforge/jnlp/tools/JarCertVerifierTest.java:
>>> New, tests isMetaInfFile()
>>
>> Unittest seams ok. As it is modifying also a code a bit, do you mind head + 1.3?
> Sure. OK for 1.3/HEAD I'm assuming ?
>
yap.
>>
>>>
>>> Reproducer changelog:
>>> 2012-07-05 Adam Domurad<adomurad at redhat.com>
>>>
>>> Reproducer for allowing unsigned content in META-INF/ folder.
>>> Derives from ReadPropertiesSigned test's signed jar.
>>> *
>>> tests/reproducers/custom/UnsignedContentInMETAINF/resources/UnsignedContentInMETAINF.jnlp:
>>> New, runs a modified version of ReadPropertiesSigned.jar
>>> (UnsignedContentInMETAINF.jar)
>>> *
>>> tests/reproducers/custom/UnsignedContentInMETAINF/srcs/META-INF/unsigned_file_in_metainf:
>>> New, placed into a modified version of ReadPropertiesSigned.jar
>>> (UnsignedContentInMETAINF.jar) so that there is unsigned content in the
>>> META-INF/ folder.
>>> * tests/reproducers/custom/UnsignedContentInMETAINF/srcs/Makefile:
>>> New, creates a modified version of ReadPropertiesSigned.jar, named
>>> UnsignedContentInMETAINF.jar, and places unsigned content inside its
>>> META-INF/ folder
>>> *
>>> tests/reproducers/custom/UnsignedContentInMETAINF/testcases/UnsignedContentInMETAINF.java:
>>> Test driver for jnlp file
>>>
>>> Thanks,
>>> Adam
>>>
>>
reproducer is no ok. Ok for head and wherever if you feel it relevant.
>>
>> Thanx for reproducer. Overall looks good, one clarification below however needed.
>>
>>
>>>
>>> unsigned-metainf-content-reproducer.patch
>>>
>>
>> ...snip...
>>
>>> +</jnlp>
>>> diff --git a/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/META-INF/unsigned_file_in_metainf b/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/META-INF/unsigned_file_in_metainf
>>> new file mode 100644
>>
>> Purpose of this is to create new file (I have not seen it used in reproducer) or empty directory in
>> src (which I have not seen used too)?
>>
>> If there is need for empty file or (or dir) then touch (mkdir) in custom Makefile perhaps? Although
>> I agree that have those files in file tree is little bit clearer, mercurial do not like empty
>> directories :(
> Added content so the file in the META-INF/ folder is no longer empty. I
> have not taking the mkdir + touch approach, as we discussed.
>>
>> Also - it is in src so you expect to have it compiled (as you are overwriting embedded jar's work)
>> or copied? it is not clear from makefile (but I can be dummy enough to miss it)
> It is used to manually update the jar
>>
>>
>>> diff --git a/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/Makefile b/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/Makefile
>>> new file mode 100644
>>> --- /dev/null
>>> +++ b/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/Makefile
>>> @@ -0,0 +1,17 @@
>>> +TESTNAME=UnsignedContentInMETAINF
>>> +JAVAC_CLASSPATH=$(JNLP_TESTS_ENGINE_DIR):$(NETX_DIR)/lib/classes.jar
>>> +DEPLOY_DIR=$(JNLP_TESTS_SERVER_DEPLOYDIR)
>>> +JAVAC=$(BOOT_DIR)/bin/javac
>>> +JAR=$(BOOT_DIR)/bin/jar
>>> +
>>> +prepare-reproducer:
>>> + echo PREPARING REPRODUCER $(TESTNAME)
>>
>>> + cp $(DEPLOY_DIR)/ReadPropertiesSigned.jar $(DEPLOY_DIR)/UnsignedContentInMETAINF.jar
>>> + $(JAR) uf $(DEPLOY_DIR)/UnsignedContentInMETAINF.jar META-INF
>>
>> those two lines above are what is confusing me:
>> you are coping ReadPropertiesSigned.jar and overwriting UnsignedContentInMETAINF.jar.
>> Then you are updating this copy for META-INF (directory?) which do not exists(?)
>> Although if it would exists, then it would be nice to have deirectory where this jar u is happening.
>>
>> From both above - probably wrongly placed META-INF and strange cp and jar u, I think that
>> reproducer is not testing what it should.
>>
>> Or am I wrong and pure $(JAR) uf $(DEPLOY_DIR)/UnsignedContentInMETAINF.jar META-INF, with
>> non-existent META-INF directory will cause timestamps in jar to change and so make signatures of
>> metainf invalid? But then you do not need this
>> tests/reproducers/custom/UnsignedContentInMETAINF/srcs/META-INF/unsigned_file_in_metainf misleading
>> file.
> The file is copied into the jar's META-INF/ by this command. I have
> added a redundant cd before the jar command as we discussed.
>> If this is true, then perhaps more visible approach then just modifying the time-stamp can be used?
>>
>>
>> Rest of the reproducer is nice work, thanks for it!
>>
>> J.
>>> + echo PREPARED REPRODUCER $(TESTNAME)s
>>> +
>>> +clean-reproducer:
>>> + echo CLEANING REPRODUCER $(TESTNAME)
>>> + rm -f UnsignedContentInMETAINF.jar
>>> + echo CLEANED REPRODUCER $(TESTNAME)
>>> +
>>> diff --git a/tests/reproducers/custom/UnsignedContentInMETAINF/testcases/UnsignedContentInMETAINF.java b/tests/reproducers/custom/UnsignedContentInMETAINF/testcases/UnsignedContentInMETAINF.java
>>> new file mode 100644
>>> --- /dev/null
>>> +++ b/tests/reproducers/custom/UnsignedContentInMETAINF/testcases/UnsignedContentInMETAINF.java
>>> @@ -0,0 +1,65 @@
>>> +/* UnsignedContentInMETAINF.java
>>> +Copyright (C) 2011 Red Hat, Inc.
>>> +
>>> +This file is part of IcedTea.
>>> +
>>> +IcedTea is free software; you can redistribute it and/or
>>> +modify it under the terms of the GNU General Public License as published by
>>> +the Free Software Foundation, version 2.
>>> +
>>> +IcedTea is distributed in the hope that it will be useful,
>>> +but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> +General Public License for more details.
>>> +
>>> +You should have received a copy of the GNU General Public License
>>> +along with IcedTea; see the file COPYING. If not, write to
>>> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>>> +02110-1301 USA.
>>> +
>>> +Linking this library statically or dynamically with other modules is
>>> +making a combined work based on this library. Thus, the terms and
>>> +conditions of the GNU General Public License cover the whole
>>> +combination.
>>> +
>>> +As a special exception, the copyright holders of this library give you
>>> +permission to link this library with independent modules to produce an
>>> +executable, regardless of the license terms of these independent
>>> +modules, and to copy and distribute the resulting executable under
>>> +terms of your choice, provided that you also meet, for each linked
>>> +independent module, the terms and conditions of the license of that
>>> +module. An independent module is a module which is not derived from
>>> +or based on this library. If you modify this library, you may extend
>>> +this exception to your version of the library, but you are not
>>> +obligated to do so. If you do not wish to do so, delete this
>>> +exception statement from your version.
>>> + */
>>> +
>>> +
>>> +import java.util.Arrays;
>>> +import java.util.Collections;
>>> +import java.util.List;
>>> +import net.sourceforge.jnlp.ServerAccess;
>>> +import org.junit.Assert;
>>> +import org.junit.Test;
>>> +import net.sourceforge.jnlp.ProcessResult;;
>>> +
>>> +public class UnsignedContentInMETAINF {
>>> +
>>> + private static ServerAccess server = new ServerAccess();
>>> + private final List<String> l=Collections.unmodifiableList(Arrays.asList(new String[] {"-Xtrustall"}));
>>> +
>>> + String accessMatcher = "(?s).*java.security.AccessControlException.{0,5}access denied.{0,5}java.util.PropertyPermission.{0,5}" + "user.name.{0,5}read" + ".*";
>>> +
>>> + @Test
>>> + public void ReadSignedPropertiesWithUnsignedContentInMETAINF() throws Exception {
>>> + //request for allpermissions
>>> + ProcessResult pr=server.executeJavawsHeadless(l,"/UnsignedContentInMETAINF.jnlp");
>>> + Assert.assertFalse("Stderr should NOT match "+accessMatcher+" but did",pr.stderr.matches(accessMatcher));
>>> + String ss="ClassNotFoundException";
>>> + Assert.assertFalse("Stderr should not contain "+ss+" but did",pr.stderr.contains(ss));
>>> + Assert.assertTrue("Stdout length should be>= 4 but was "+pr.stdout.length(),pr.stdout.length()>=4); // /home/user or /root or anything else :(
>>> + Assert.assertFalse("Should not be terminated but was",pr.wasTerminated);
>>> + Assert.assertEquals((Integer)0, pr.returnValue);
>>> + }
>>> + }
>>>
>>>
>>> JCVIsMetaInfFile.patch
>>>
>>>
>>> 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
>>> @@ -441,7 +441,7 @@ public class JarCertVerifier implements
>>> * . META-INF/*.DSA
>>> * . META-INF/*.RSA
>>> */
>>> - static private boolean isMetaInfFile(String name) {
>>> + static boolean isMetaInfFile(String name) {
>>> String ucName = name.toUpperCase();
>>> return ucName.startsWith(META_INF);
>>> }
>>> diff --git a/tests/netx/unit/net/sourceforge/jnlp/tools/JarCertVerifierTest.java b/tests/netx/unit/net/sourceforge/jnlp/tools/JarCertVerifierTest.java
>>> new file mode 100644
>>> --- /dev/null
>>> +++ b/tests/netx/unit/net/sourceforge/jnlp/tools/JarCertVerifierTest.java
>>> @@ -0,0 +1,17 @@
>>> +package net.sourceforge.jnlp.tools;
>>> +
>>> +import static org.junit.Assert.*;
>>> +
>>> +import org.junit.Test;
>>> +
>>> +public class JarCertVerifierTest {
>>> +
>>> + @Test
>>> + public void testIsMetaInfFile() {
>>> + final String METAINF ="META-INF";
>>> + assertFalse(JarCertVerifier.isMetaInfFile("some_dir/" + METAINF + "/filename"));
>>> + assertFalse(JarCertVerifier.isMetaInfFile(METAINF + "filename"));
>>> + assertTrue(JarCertVerifier.isMetaInfFile(METAINF + "/filename"));
>>> + }
>>> +
>>> +}
>>>
>>
>
More information about the distro-pkg-dev
mailing list