[RFC][icedtea-web] Major rework of JarCertVerifier
Omair Majid
omajid at redhat.com
Fri Aug 24 09:18:13 PDT 2012
Hi Danesh,
* Danesh Dadachanji <ddadacha at redhat.com> [2012-08-24 11:00]:
>
>
> On 21/08/12 07:00 PM, Omair Majid wrote:
> >>===========================================================================
> >>
> >>* Classloader changes *
> >> - netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> >>
> >>This has 3 updates:
> >> 1. Updated it to handle a global JCV rather than creating instances on its own.
> >
> >Just to clarify, this is not global, but per-application/per-applet
> >only, right?
> >
Ok, just wanted to make sure everyone was on the same page.
> >>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
> >>@@ -222,6 +218,16 @@ public class JNLPClassLoader extends URL
> >>
> >> this.mainClass = mainName;
> >>
> >>+ AppVerifier verifier;
> >>+
> >>+ if (file instanceof PluginBridge && !((PluginBridge)file).useJNLPHref()) {
> >
> >I will be happier if we can remove this instanceof check too.
> >
>
> I had an idea for a flag but I shot it down when I realized there
> could be more than 2 runtime types in the future. Do you have any
> suggestions on handling it in case Application + Applet + some other
> runtime app ever is implemented?
The standard way to handle this is to create a factory class. That class
is responsible only for creating the right type of AppVerifier. That
class will change whenever we add more types of runtimes or change when
to use an appverifier. I wouldnt mind so much if that class had similar
hacks. In JNLPClassLoader, they look a little out of place.
> >>@@ -874,9 +877,7 @@ public class JNLPClassLoader extends URL
> >> InputStreamReader jnlpReader = null;
> >>
> >> try {
> >>- signer.verifyJars(desc, tracker);
> >
> >Is this intentional? Do we verify this jar earlier? If so, can we do
> >something so the absence of this line doesn't stick out as a bug?
> >
>
> Yes, this was intentional, we always verify them before that hunk is
> reached. I can add a comment mentioning this as a requirement. Is
> this enough or did you want a check for verification (and throw a
> fatal exception if it has not been verified)?
Hmm..on thinking more about it, they only we can verify that the jardesc
has been verified is adding a call like signer.ensureHasSeenJar(desc). I
would like that, TBH, but maybe this is a sign that our code here is too
complex. I can't make up my mind here :)
> >>@@ -990,29 +991,17 @@ public class JNLPClassLoader extends URL
> >> e.printStackTrace(System.err);
> >> }
> >> }
> >>-
> >>- private void checkTrustWithUser(JarCertVerifier jcv) throws LaunchException {
> >>+
> >>+ /**
> >>+ * Prompt the user for trust on all the signers that require approval.
> >>+ * @throws LaunchException if the user does not approve every dialog prompt.
> >>+ */
> >>+ private void checkTrustWithUser() throws LaunchException {
> >> if (JNLPRuntime.isTrustAll()){
> >> return;
> >> }
> >>- if (!jcv.getRootInCacerts()) { //root cert is not in cacerts
> >>- boolean b = SecurityDialogs.showCertWarningDialog(
> >>- AccessType.UNVERIFIED, file, jcv);
> >>- if (!b)
> >>- throw new LaunchException(null, null, R("LSFatal"),
> >>- R("LCLaunching"), R("LNotVerified"), "");
> >>- } else if (jcv.getRootInCacerts()) { //root cert is in cacerts
> >>- boolean b = false;
> >>- if (jcv.noSigningIssues())
> >>- b = SecurityDialogs.showCertWarningDialog(
> >>- AccessType.VERIFIED, file, jcv);
> >>- else if (!jcv.noSigningIssues())
> >>- b = SecurityDialogs.showCertWarningDialog(
> >>- AccessType.SIGNING_ERROR, file, jcv);
> >>- if (!b)
> >>- throw new LaunchException(null, null, R("LSFatal"),
> >>- R("LCLaunching"), R("LCancelOnUserRequest"), "");
> >>- }
> >>+
> >>+ jcv.checkTrustWithUser(file);
> >
> >TBH, I am not sure how I feel about moving this gui logic into JCV. I
> >would like to keep it as gui-free as possible.
> >
>
> I was a bit on the edge about doing this. I tried doing it all in
> JCV but the algorithms for Plugin VS JNLPs are just too different. I
> couldn't see a clean way of taking care of both through the strategy
> but still keeping all the UI stuff in the classloader. :/
>
I thought they were supposed to handle similar problems (unverified, not
signed and so on). If they are different enough, putting them in JCV is
fine for now (though maybe it would be better if there was a dedicated
class for that).
> >Minor nit: javadocs of the form "@param foo" is not useful at all
> >without some description of what "foo" is. It's up to you whether you
> >want to add more details for "foo" in the code below or just remove such
> >"foo". I am fine with it either way - though I prefer more descriptive
> >names and fewer comments.
> >
>
> Woops, sorry, I meant to fill those out. I can't think of a name that's short TBH.. jarsWithNumSignedEntries or numSignedEntriesOfJars?
Something like signedEntriesInJars makes sense to me. But maybe that's a
sign that you might want to use a dedicated data structure (not a map)
for this (though you dont have to do it in this patch).
> >>+ /**
> >>+ * Prompt the user with requests for trusting the certificates used by this app
> >>+ * @throws LaunchException
> >>+ */
> >>+ public void checkTrustWithUser(JarCertVerifier jcv, JNLPFile file)
> >>+ throws LaunchException;
> >
> >Hm.. Not sure how I feel about this. Does this UI-related method really
> >need to be part of this interface?
> >
>
> The only other solution that I had was to create another interface
> for using strategy patterns for UI. I figured though that 3 new
> classes for 1 method wasn't quite worth it. If you prefer, I can do
> so.
TBH, I would prefer that. No reason to mix unrelated things in one class
(that's how we ended up with the current JNLPClassLoader mess). But this
can be done later.
> >>diff --git a/netx/net/sourceforge/jnlp/security/CertWarningPane.java b/netx/net/sourceforge/jnlp/security/CertWarningPane.java
> >>--- a/netx/net/sourceforge/jnlp/security/CertWarningPane.java
> >>+++ b/netx/net/sourceforge/jnlp/security/CertWarningPane.java
> >>@@ -96,7 +96,7 @@ public class CertWarningPane extends Sec
> >> private void addComponents() {
> >> AccessType type = parent.getAccessType();
> >> JNLPFile file = parent.getFile();
> >>- Certificate c = parent.getCertVerifier().getPublisher();
> >>+ Certificate c = parent.getCertVerifier().getPublisher(null);
> >
> >What does null mean in this context? Perhaps we can have a method like
> >getDefaultPublisher or getAnyPublisher() instead? Please avoid using
> >nulls if possible.
> >
>
> This null and all the other similar calls are just a hack to gain
> easy access to the currentlyUsed CertPath instance var. The null is
> a place holder until we can get a proper UI patch in. I left it
> there as a sign that it should be replaced by the proper CertPath. I
> didn't want to get rid of the parameter itself because I believe
> it's all that is needed to fill out the right UI info. Changing it
> (or adding another method as you said) would mean modifying the
> interface as well (and in turn, HttpsCertVerifier). I thought this
> was the better of the two because the future patch could then use
> these. I can redo the interface if you want.
Is a good fix here too complex for now? How about adding a comment to
clarify what null means?
.....getPublisher(null /* = current certpath */);
Or maybe add a constant to CertVerfifier called CURRENT_CERTPATH (with
value = null) and change the code to:
.....getPublisher(CURRENT_CERTPATH);
> >>diff --git a/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java b/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java
> >>--- a/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java
> >>+++ b/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java
> >>- public boolean hasSigningIssues() {
> >>+ @Override
> >>+ public boolean hasSigningIssues(CertPath certPath) {
> >> return false;
> >
> >Are all https certs always good?
> >
>
> I believe so. I didn't look into it because I assumed that what we
> have had all along was correct. (Not sure if you overlooked that I
> didn't change the method functionality, just added the param).
I did, but I thought I would ask anyway. You touch a piece of code,
now you are responsible for it ;)
> >>diff --git a/netx/net/sourceforge/jnlp/tools/CertInformation.java b/netx/net/sourceforge/jnlp/tools/CertInformation.java
> >>new file mode 100644
> >>--- /dev/null
> >>+++ b/netx/net/sourceforge/jnlp/tools/CertInformation.java
> >
> >>+/**
> >>+ * Maintains information about a CertPath that has signed at least one of the
> >>+ * entries provided by a jar of the app.
> >>+ */
> >
> >A general comment: this class mixes information about certificates (it's
> >not valid yet or isnt used correctly) with information about what it has
> >signed (jar names). Perhaps those two pieces of information should be
> >kept separate.
> >
>
> I expected this class to contain any and all information regarding
> the CertPath it represents (for a given classloader). To me, this
> info will help determine how it is used for the app. The bools (is
> not yet valid) affect how it is used and so do the number of signed
> entries. That's how I saw it but if you think it should be separated
> into 2 classes, you definitely have better judgment than me. =) How
> would you go about keeping track of 2 classes then for each cert
> path? 2 maps? (CertPath to CertInfo and Certpath to SomeNewClass).
It's okay to keep this as it is for now. But please keep in mind that
a class should have one responsibility :)
> >>+ /**
> >>+ * Resets any trust of the root and publisher. Also removes unnecessary
> >>+ * details from the list of issues.
> >>+ */
> >>+ public void resetForReverification() {
> >>+ alreadyTrustPublisher = false;
> >>+ rootInCacerts = false;
> >>+ removeFromDetails(R("SUntrustedCertificate"));
> >>+ removeFromDetails(R("STrustedCertificate"));
> >>+ }
> >>+
> >>+ /**
> >>+ * Add an issue to the list of details of issues with this certificate.
> >>+ * Duplicate issues will be ignored.
> >>+ *
> >>+ * @param detail The new issue to be added about this certificate.
> >>+ */
> >>+ public void addToDetails(String detail) {
> >>+ if (!details.contains(detail))
> >>+ details.add(detail);
> >>+ }
> >>+
> >>+ /**
> >>+ * Remove an issue from the list of details of issues with this certificate.
> >>+ * List is unchanged if detail was not present.
> >>+ *
> >>+ * @param detail The issue to be removed regarding this certificate.
> >>+ */
> >>+ public void removeFromDetails(String detail) {
> >>+ details.remove(detail);
> >>+ }
> >
> >Seems to me like these 3 methods should not be public. Any specific
> >reason for making them public? There are acessor methods already in
> >this class for handling this safely.
> >
>
> I changed the last one to private and have removed addToDetails (no
> longer needed). The reset one is called in JCV though, maybe you
> overlooked this? Or were you referring to something else.
I only noticed that reset is called from JCV after writing this comment.
Please ignore it.
> >>diff --git a/tests/netx/unit/net/sourceforge/jnlp/tools/VerifyJarEntryCertsTest.java b/tests/netx/unit/net/sourceforge/jnlp/tools/VerifyJarEntryCertsTest.java
> >>new file mode 100644
> >>--- /dev/null
> >>+++ b/tests/netx/unit/net/sourceforge/jnlp/tools/VerifyJarEntryCertsTest.java
> >>+public class VerifyJarEntryCertsTest {
> >>+
> >>+ class JarCertVerifierEntry extends JarEntry {
> >>+ CodeSigner[] signers;
> >>+ public JarCertVerifierEntry(String name, CodeSigner[] codesigners) {
> >>+ super(name);
> >>+ signers = codesigners;
> >>+ }
> >>+
> >>+ public JarCertVerifierEntry(String name) {
> >>+ this(name, null);
> >>+ }
> >>+
> >>+ public CodeSigner[] getCodeSigners() {
> >>+ return signers == null ? null : signers.clone();
> >>+ }
> >>+ }
> >>+
> >>+ // Empty list to be used with JarCertVerifier constructor.
> >>+ private static final List<JARDesc> emptyJARDescList = new Vector<JARDesc>();
> >>+
> >>+ private static final String DNPARTIAL = ", OU=JarCertVerifier Unit Test, O=IcedTea, L=Toronto, ST=Ontario, C=CA";
> >>+ private static CodeSigner alphaSigner, betaSigner, charlieSigner,
> >>+ expiredSigner, expiringSigner, notYetValidSigner, expiringAndNotYetValidSigner;
> >>+
> >>+ @BeforeClass
> >>+ public static void setUp() throws Exception {
> >>+ Date currentDate = new Date();
> >>+ Date pastDate = new Date(currentDate.getTime() - (1000L * 24L * 60L * 60L) - 1000L); // 1 day and 1 second in the past
> >>+ Date futureDate = new Date(currentDate.getTime() + (1000L * 24L * 60L * 60L)); // 1 day in the future
> >>+ alphaSigner = CodeSignerCreator.getOneCodeSigner("CN=Alpha Signer" + DNPARTIAL, currentDate, 365);
> >>+ betaSigner = CodeSignerCreator.getOneCodeSigner("CN=Beta Signer" + DNPARTIAL, currentDate, 365);
> >>+ charlieSigner = CodeSignerCreator.getOneCodeSigner("CN=Charlie Signer" + DNPARTIAL, currentDate, 365);
> >>+ expiredSigner = CodeSignerCreator.getOneCodeSigner("CN=Expired Signer" + DNPARTIAL, pastDate, 1);
> >>+ expiringSigner = CodeSignerCreator.getOneCodeSigner("CN=Expiring Signer" + DNPARTIAL, currentDate, 1);
> >>+ notYetValidSigner = CodeSignerCreator.getOneCodeSigner("CN=Not Yet Valid Signer" + DNPARTIAL, futureDate, 365);
> >>+ expiringAndNotYetValidSigner = CodeSignerCreator.getOneCodeSigner("CN=Expiring and Not Yet Valid Signer" + DNPARTIAL, futureDate, 3);
> >>+ }
> >>+
> >>+ @Test
> >>+ public void testManyEntriesMultipleValidSigners() throws Exception {
> >>+ JarCertVerifier jcv = new JarCertVerifier(null);
> >>+ CodeSigner[] signers = { alphaSigner, betaSigner, charlieSigner };
> >>+ Vector<JarEntry> entries = new Vector<JarEntry>();
> >>+ entries.add(new JarCertVerifierEntry("firstSignedByThree", signers));
> >>+ entries.add(new JarCertVerifierEntry("secondSignedByThree", signers));
> >>+ entries.add(new JarCertVerifierEntry("thirdSignedByThree", signers));
> >>+ VerifyResult result = jcv.verifyJarEntryCerts("",true, entries);
> >>+
> >>+ Assert.assertEquals("Three entries signed by three signers should be considered signed and okay.",
> >>+ VerifyResult.SIGNED_OK, result);
> >>+ Assert.assertEquals("Three entries signed by three means three signers in the verifier.",
> >>+ 3, jcv.getCertsList().size());
> >>+ Assert.assertTrue("Three entries signed by three means three signers in the verifier.",
> >>+ jcv.getCertsList().contains(alphaSigner.getSignerCertPath())
> >>+ && jcv.getCertsList().contains(betaSigner.getSignerCertPath())
> >>+ && jcv.getCertsList().contains(charlieSigner.getSignerCertPath()));
> >>+ }
> >
> >I am not quite clear on what this means. This test doesn't set up
> >either of the AppVerifier strategies. What does VerifyResult.SIGNED_OK
> >mean? Is this expected?
> >
> >Perhaps it might be better to make verifyJarEntryCerts void and call
> >a method like getAllSigned() to find out the verification result?
> >
>
> I think you have the idea of this test wrong. This only unit tests
> verifyJarEntryCerts - a method that goes through only one jar. It
> doesn't go through the entire list of jars, it just verifies the one
> jar you pass. As of this patch, jars for a JNLP vs jars for an
> applet in the plugin are treated the same. Namely, the entire jar
> needs to be signed by the same signer. FWIW, This is the test I made
> way back and posted for review:
>
> http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2012-April/018023.html
Oh, I see. Please ignore my comments then.
> >>+ @Test
> >>+ public void testNoCommonSigner() throws Exception {
> >>+ JarCertVerifier jcv = new JarCertVerifier(null);
> >>+ CodeSigner[] alphaSigners = { alphaSigner };
> >>+ CodeSigner[] betaSigners = { betaSigner };
> >>+ CodeSigner[] charlieSigners = { charlieSigner };
> >>+ Vector<JarEntry> entries = new Vector<JarEntry>();
> >>+ entries.add(new JarCertVerifierEntry("firstSignedByAlpha", alphaSigners));
> >>+ entries.add(new JarCertVerifierEntry("secondSignedByBeta", betaSigners));
> >>+ entries.add(new JarCertVerifierEntry("thirdSignedByCharlie", charlieSigners));
> >>+ VerifyResult result = jcv.verifyJarEntryCerts("",true, entries);
> >>+
> >>+ Assert.assertEquals("Three entries signed by no common signers should be considered unsigned.",
> >>+ VerifyResult.UNSIGNED, result);
> >>+ Assert.assertEquals("Three entries signed by no common signers means no signers in the verifier.",
> >>+ 0, jcv.getCertsList().size());
> >>+ }
> >
> >If this was an applet, this should be considered signed, no?
> >
>
> As of this patch, no. To clarify the test, it's saying take a jar
> with 3 entries, each entry has a different signer. As of this patch,
> an applet requires every entry in a jar to be signed by the same
> signer. In the future this may change, in which case the test should
> change. However, I don't know what the final say on the
> implementation will be.
I see. Okay, no objections from me on this, then.
A potential improvement would be to add more unit tests - especially
those that are exercising the rest of the JCV api, and not just the
single-jar bits.
Cheers,
Omair
More information about the distro-pkg-dev
mailing list