[RFC][PATCH][icedtea-web]: Added support for signed JNLP file- Updated Patch
Saad Mohammad
smohammad at redhat.com
Mon Jul 25 08:32:04 PDT 2011
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.
>
>> 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 Thu
>> Jul 14 17:11:49 2011 -0400
>
>> @@ -471,8 +482,151 @@
>> //otherwise this jar is simply unsigned -- make
>> sure to ask
>> //for permission on certain actions
>> }
>
> I have a number of questions about this next bit, but that could just
> be because I am not clear on what you are trying to do.
>
> You may want to split code into a new method and/or add more comments.
Sure, I will add a few more comments!
>
>> +
>> + 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.
>
>> + tracker.addResource(jars[i].getLocation(),
>> jars[i].getVersion(),
>> + getDownloadOptionsForJar(jars[i]),
>> + jars[i].isCacheable() ?
>> JNLPRuntime.getDefaultUpdatePolicy()
>> + : UpdatePolicy.FORCE);
>> +
>
> Unless I am mistaken, this is already done a few lines above this.
Yes, I can remove this code. Just tested it!
>
>> + 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.
>
> Also, note that calling JarSigner.verify() for all jars means that all
> jars are downloaded (this method blocks) - even jars marked as lazy.
> This makes the lazy/eager-loading mechanism useless. I am not a fan of
> lazy loading myself, but I am not sure if that's what you meant. If
> you really mean to do this, than it should be a lot more explicit (a
> separate patch?).
I am new with lazy and eager. But I will look into that and get back to you.
>
>> + if (signer.allJarsSigned()) { // If the jar
>> is signed
>> + URL location = jars[i].getLocation();
>> + File localFile =
>> tracker.getCacheFile(location);
>> +
>> + if (localFile == null) {
>> + throw new JNLPMatcherException(
>> + "Could not locate jar file,
>> returned null");
>> + }
>> +
>
> Looking at the code that catches this exception, it sounds like if the
> jar could not be cached the application aborts? Perhaps you should
> just assume failure in matching jnlp files? Or treat it the same way
> as if signed jnlp files are not present?
>
Yes, you are right. I will make this change.
>> + else {
>> + 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)) {
>> +
>
> Is this intentional? Do we want case-insensitive matching? If so,
> please add a comment here (or add/modify variable names to make it
> explicit).
Yes, the changelog states that the filename should be all in UPPERCASE
but should also be recognized regardless of the case
Change #3:
(http://jcp.org/aboutJava/communityprocess/maintenance/jsr056/jnlp-7_0-changes.html).
I will add a comment to make it more specific.
>
>> + if (JNLPRuntime.isDebug())
>> + System.out
>> +
>> .println("\tCreating Jar InputStream from Jar Entry");
>> +
>> + InputStream inStream =
>> jarFile
>> +
>> .getInputStream(je);
>> + InputStreamReader
>> inputReader = new InputStreamReader(
>> + inStream);
>> +
>> + if (JNLPRuntime.isDebug())
>> + System.out
>> +
>> .println("\tCreating File InputStream from lauching JNLP file");
>> +
>> + 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 cached file
>> + jn = new
>> File(url.getPath());
>> + else
>> + jn =
>> CacheUtil.getCacheFile(url, null);
>> +
>> + FileReader fr = new
>> FileReader(jn);
>> + 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...");
>> +
>> + matcher = new
>> JNLPMatcher(
>> +
>> inputReader, jnlpReader, true);
>> + }
>> +
>> + if (!matcher.isMatch())
>> + 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) {
>> + throw new
>> JNLPMatcherException(
>> +
>> e.getMessage(), e);
>> + }
>> + }
>> +
>> + }
>
> So here's what I am really confused about: what does matching do? I
> see that if there is no signed jnlp file, the code runs normally. If
> there is a correctly signed jnlp file the code also continues
> normally. So why are signed jnlp files needed at all? What problem are
> they solving?
>
> I guess I am missing something obvious; an explanation of what this
> code is trying to do would be appreciated.
If there is no signed JNLP file, the code runs normally. (as you
mentioned above)
If there is a matching signed JNLP file, the code runs normally. (as you
mentioned above)
If there is an unmatched signed JNLP file, the application is not
initialized and fails to start. The reason why I found this beneficial
is because the signer may not want their signed jars to be used if the
launching JNLP file is not the same as the one the signer provides.
Anyone can just create their own JNLP file to launch the application as
long as they know the location of the resource(s). I think this way the
signer has the option to put restrictions onto their signed jar file; so
under certain conditions the jar files can be used only if the launching
JNLP file is "approved" by the signer. I think another reason why a
signer might do this is so the signer does not have another person
running their code and using their signed jar files as resource (using
the API? If they know it). I am not sure about this one, but it makes sense.
>
> Thanks,
> Omair
Thanks Andrew, Jiri, Omair and Deepak for your input on this patch.
Sorry, I couldn't reply much sooner, I was on PTO last Thursday and
Friday. =(
Unfortunately I did commit one of the patches, but I will create another
patches with the updated changes. I will also send a patch with the
updated JNLPClassLoader with the new changes.
Thanks again. =)
--
Cheers,
Saad Mohammad
More information about the distro-pkg-dev
mailing list