[RFC][PATCH][icedtea-web]: Added support for signed JNLP file- Updated Patch
Deepak Bhole
dbhole at redhat.com
Wed Jul 20 13:10:35 PDT 2011
* Saad Mohammad <smohammad at redhat.com> [2011-07-06 15:38]:
> This is the updated patch for adding support of signed JNLP file. I
> have attached two patches.
>
> More Info:
> http://jcp.org/aboutJava/communityprocess/maintenance/jsr056/jnlp-7_0-changes.html
> (Change 3)
>
>
Hi Saad,
Generally the patch is good. I have a few minor corrections, please see
below.
Cheers,
Deepak
> diff -r 86abbf8be0b1 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Thu Jun 23 15:29:45 2011 +0200
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Tue Jul 05 16:31:01 2011 -0400
> @@ -19,8 +19,10 @@
>
> import java.io.File;
> import java.io.FileOutputStream;
> +import java.io.FileReader;
> import java.io.IOException;
> import java.io.InputStream;
> +import java.io.InputStreamReader;
> import java.net.MalformedURLException;
> import java.net.URL;
> import java.net.URLClassLoader;
> @@ -51,6 +53,8 @@
> import net.sourceforge.jnlp.ExtensionDesc;
> import net.sourceforge.jnlp.JARDesc;
> import net.sourceforge.jnlp.JNLPFile;
> +import net.sourceforge.jnlp.JNLPMatcher;
> +import net.sourceforge.jnlp.JNLPMatcherException;
> import net.sourceforge.jnlp.LaunchException;
> import net.sourceforge.jnlp.ParseException;
> import net.sourceforge.jnlp.PluginBridge;
> @@ -80,7 +84,11 @@
> // todo: initializePermissions should get the permissions from
> // extension classes too so that main file classes can load
> // resources in an extension.
> -
> +
> + /** Signed JNLP File and Template */
> + final public static String template = "JNLP-INF/APPLICATION_TEMPLATE.JNLP";
> + final public static String application = "JNLP-INF/APPLICATION.JNLP";
> +
> /** map from JNLPFile url to shared classloader */
> private static Map<String, JNLPClassLoader> urlToLoader =
> new HashMap<String, JNLPClassLoader>(); // never garbage collected!
> @@ -158,8 +166,9 @@
> * Create a new JNLPClassLoader from the specified file.
> *
> * @param file the JNLP file
> + * @throws JNLPMatcherException if signed JNLP file failed to be verified or did not match
> */
> - protected JNLPClassLoader(JNLPFile file, UpdatePolicy policy) throws LaunchException {
> + protected JNLPClassLoader(JNLPFile file, UpdatePolicy policy) throws LaunchException, JNLPMatcherException {
> super(new URL[0], JNLPClassLoader.class.getClassLoader());
>
I don't think the constructor needs to throw a JNLPME. It should be
caught, processed and a LaunchException should be the only think it
throws.
> if (JNLPRuntime.isDebug())
> @@ -273,8 +282,9 @@
> *
> * @param file the file to load classes for
> * @param policy the update policy to use when downloading resources
> + * @throws JNLPMatcherException if signed JNLP file failed to be verified or did not match
> */
> - public static JNLPClassLoader getInstance(JNLPFile file, UpdatePolicy policy) throws LaunchException {
> + public static JNLPClassLoader getInstance(JNLPFile file, UpdatePolicy policy) throws LaunchException, JNLPMatcherException {
Once above is resolved, the JNLPME throw will be gone.
> JNLPClassLoader baseLoader = null;
> JNLPClassLoader loader = null;
> String uniqueKey = file.getUniqueKey();
> @@ -341,9 +351,10 @@
> * @param location the file's location
> * @param version the file's version
> * @param policy the update policy to use when downloading resources
> + * @throws JNLPMatcherException if signed JNLP file failed to be verified or did not match
> */
> public static JNLPClassLoader getInstance(URL location, String uniqueKey, Version version, UpdatePolicy policy)
> - throws IOException, ParseException, LaunchException {
> + throws IOException, ParseException, LaunchException, JNLPMatcherException {
> JNLPClassLoader loader = urlToLoader.get(uniqueKey);
>
Same.
> if (loader == null || !location.equals(loader.getJNLPFile().getFileLocation()))
> @@ -403,7 +414,7 @@
> * Load all of the JARs used in this JNLP file into the
> * ResourceTracker for downloading.
> */
> - void initializeResources() throws LaunchException {
> + void initializeResources() throws LaunchException, JNLPMatcherException {
> JARDesc jars[] = resources.getJARs();
> if (jars == null || jars.length == 0)
> return;
> @@ -471,8 +482,135 @@
> //otherwise this jar is simply unsigned -- make sure to ask
> //for permission on certain actions
> }
> +
> + 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...");
> +
Above should print to STDERR, not STDOUT.
> + 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
> +
> + tracker.addResource(jars[i].getLocation(), jars[i].getVersion(),
> + getDownloadOptionsForJar(jars[i]),
> + jars[i].isCacheable() ? JNLPRuntime.getDefaultUpdatePolicy()
> + : UpdatePolicy.FORCE);
> +
> + try {
> + signer.verifyJars(eachJar, tracker);
> +
> + if (signer.allJarsSigned()) { //If the jar is signed
> + URL location = jars[i].getLocation();
> + File localFile = tracker.getCacheFile(location);
> +
> +
> + if(localFile == null)
Minor nit-pick.. there should be a space between the if and the (
> + {
> + throw new JNLPMatcherException("Could not locate jar file, returned null");
> + }
> +
> + else{
Space between else and {
> + try {
> + JarFile jarFile = new JarFile(localFile);
> + Enumeration<JarEntry> entries = jarFile.entries();
> + JarEntry je;
> +
> + while (entries.hasMoreElements()) {
> + je = entries.nextElement();
> + String jeName = je.getName().toUpperCase();
> +
> + if (jeName.equals(template) || jeName.equals(application)) {
> +
> + if (JNLPRuntime.isDebug())
> + System.out.println("\tCreating Jar InputStream from Jar Entry");
> +
> + InputStream inStream = jarFile.getInputStream(je);
There is a resource leak here.. this stream is opened, but who is closing it?
> + InputStreamReader inputReader = new InputStreamReader(
> + inStream);
> +
> + if (JNLPRuntime.isDebug())
> + System.out.println("\tCreating File InputStream from lauching JNLP file");
> +
STDERR instead of STDOUT
> + JNLPFile jnlp = this.getJNLPFile();
> + URL url = jnlp.getFileLocation();
> + File jn = null;
> +
> + if (url.getProtocol().equals("file")) // If the file is on the local file system, use original path, otherwise find cache file
> + jn = new File(url.getPath());
> + else
> + jn = CacheUtil.getCacheFile(url, null);
> +
> + FileReader fr = new FileReader(jn);
Same as above.. resource leak. Stream is opened but not closed.
> + InputStreamReader jnlpReader = fr;
> + JNLPMatcher matcher;
> +
> + try {
> +
> + if (jeName.equals(application)) { // If application was found
> +
> + if (JNLPRuntime.isDebug())
> + System.out.println("\tAPPLICATION.JNLP has been located within signed JAR. Starting verfication...");
> +
> + matcher = new JNLPMatcher(inputReader, jnlpReader, false);
> + } else // Otherwise template was
> + // found
> + {
> + if (JNLPRuntime.isDebug())
> + System.out
> + .println("\tAPPLICATION_TEMPLATE.JNLP has been located within signed JAR. Starting verfication...");
> +
Print should be to STDERR (I will skip there from here on.. just change
them all though please :))
> + matcher = new JNLPMatcher(inputReader,jnlpReader, true);
> + }
> +
> + if (!matcher.match())
> + throw new JNLPMatcherException(
> + "Signed Application did not match launching JNLP File");
> +
> + if (JNLPRuntime.isDebug())
> + System.out
> + .println("\t** Signed Application Verification Successful **");
> +
> + break; // break while loop
> +
> + } catch (Exception e) {
If there a reason why it is catching 'Exception' rather than specific checked ones?
> + throw new JNLPMatcherException(e.getMessage(), e);
> + }
> + }
> +
break should probably be moved here.. if there is an exception, do we
want it to keep looping?
> + }
> + } catch (IOException e) { //'new JarFile(localFile)' throws an IOException
> + if (JNLPRuntime.isDebug())
> + e.printStackTrace();
> +
> + //After this exception is caught, it is escaped.
> + //If this exception is thrown when creating an instance of JarFile,
> + //it will skip that jarFile and move on to the next jarFile (if there are any)
> + }
> + }
> + }
> + } catch (JNLPMatcherException e) {
> + //Throw e if signed JNLP file failed to be verified
> + //Throwing this exception will fail to initialize the application resulting in the termination of the application
> + throw e;
> +
> + } catch (Exception e) {
> + if (JNLPRuntime.isDebug())
> + e.printStackTrace();
> +
> + //After this exception is caught, it is escaped.
> + //If an exception is thrown while handling the jar file (mainly for JarSigner.verifyJars)
> + //it will consider the jar file to be unsigned and skip on to the next jar file (if there are any)
> + }
> + }
> + if (JNLPRuntime.isDebug())
> + System.out.println("ENDING check for signed JNLP file...");
> + }
> }
>
> +
Above new-line is not needed :)
Cheers,
Deepak
> for (JARDesc jarDesc : file.getResources().getJARs()) {
> try {
> File cachedFile = tracker.getCacheFile(jarDesc.getLocation());
More information about the distro-pkg-dev
mailing list