[RFC][icedtea-web] Major rework of JarCertVerifier
Danesh Dadachanji
ddadacha at redhat.com
Fri Aug 24 08:00:31 PDT 2012
On 21/08/12 07:00 PM, Omair Majid wrote:
> Hi Danesh,
>
> * Danesh Dadachanji <ddadacha at redhat.com> [2012-08-21 16:28]:
>> For the last few months, I've been working on completely redoing
>> JarCertVerifier to allow for the ability to handle multiple signers
>> and loosen the restriction ITW has on applets requiring to be
>> completely signed. So naturally, this is a pretty big patch. =)
>
> This is fantastic to see. It makes lots of things much easier for us
> now. Thanks for sticking through with it!
>
>> This patch is heavily *untested*. I'm only posting it to get a head
>> start on review and because my term's going to end on the 24th. :( I
>> am trying to write up tests now and make sure that this acts sanely.
>> So if you find horrible mistakes that I've overlooked, please let me
>> know!
>
> So I haven't applied this patch or anything; I am just going to point
> out things that catch my eye.
Awesome, thanks!
>
>> Here's a more informal overview of the changes that I hope will help
>> make reviewing easier. I've listed them in an order that, to me,
>> makes it easier to understand all the heavy changes in JCV itself.
>> The ChangeLog is a bit more specific regarding various method
>> changes so refer to that too.
>
> Thanks; this definitely helps.
>
>> ===========================================================================
>>
>> * Strategy pattern classes *
>> - netx/net/sourceforge/jnlp/security/AppVerifier.java
>> - netx/net/sourceforge/jnlp/security/JNLPAppVerifier.java
>> - netx/net/sourceforge/jnlp/security/PluginAppVerifier.java
>>
>> These are used to determine certain verification algorithms. For
>> example, figuring out if the publisher is already trusted. JNLPs can
>> just be checked for a publisher that is trusted and has signed every
>> entry in the app. Applets on the other hand can have different
>> signers across jars so I had to iterate over jars and figure out if
>> they were individually trusted by one signer. These are used to make
>> decisions based on runtime.
>
> Great. Having fewer instanceof checks but supporting all the different
> corner cases of applets and jnlp applications is definitely very nice.
>
>> ===========================================================================
>>
>> * 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?
>
Sorry, that was wrongly worded. This is per JNLPClassLoader, not even per-app*. Reason being JNLP extensions get their own classloader
and those resources should be verified as a separate set.
>> 2. Made it use the strategy pattern to create the global JCV.
>> 3. Updated the checkTrustWithUser method to wrap around JCV's one.
>>
>> ===========================================================================
>>
>> * UI class changes *
>> - netx/net/sourceforge/jnlp/security/CertWarningPane.java
>> - netx/net/sourceforge/jnlp/security/CertsInfoPane.java
>> - netx/net/sourceforge/jnlp/security/MoreInfoPane.java
>>
>> My original idea for dialogs was to have them store their respective
>> CertPath that they were looking at. However, there are so many
>> coupled links between their super classes. Replacing this JCV
>> instance var with a CertPath is a task for which I no longer have
>> time. :( Instead, I've created a currentlyUsed CertPath instance var
>> in JCV that is set before each dialog is called, telling them which
>> one is used. However, I've left the parameter in place for things
>> like getDetails/getRoot/getPublisher and simply passed in null in
>> the above few calls. In a future patch, we can then remove
>> currentlyUsed and go straight for the CertPath passed through.
>
> I still think this is better than what we had before. It can be made
> much better, but at least this is a start.
Agreed.
>
>> 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?
>> + verifier = new PluginAppVerifier();
>> + } else {
>> + verifier = new JNLPAppVerifier();
>> + }
>> +
>> + jcv = new JarCertVerifier(verifier);
>> +
>> // initialize extensions
>> initializeExtensions();
>>
>> @@ -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)?
>> -
>> - if (signer.allJarsSigned()) { // If the jar is signed
>> + if (jcv.isFullySigned()) { // If the jar is signed
>>
>> Enumeration<JarEntry> entries = jarFile.entries();
>> JarEntry je;
>> @@ -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. :/
>> @@ -1663,14 +1630,14 @@ public class JNLPClassLoader extends URL
>>
>> AccessController.doPrivileged(new PrivilegedExceptionAction<Void>() {
>> public Void run() throws Exception {
>> - signer.verifyJars(jars, tracker);
>> + jcv.add(jars, tracker);
>>
>> - if (signer.anyJarsSigned() && !signer.getAlreadyTrustPublisher()) {
>> - checkTrustWithUser(signer);
>> + if (jcv.isFullySigned() && !jcv.getAlreadyTrustPublisher()) {
>> + checkTrustWithUser();
>
> Can we move this logic (!jcv.getAlreadyTrustPublisher) into
> checkTrustWithUser() (or another common method) too?
>
Good idea, I can also add a debug message for when the prompt isn't needed (makes future tests easier). The catch is the other call to
checkTrustWithUser has an additional check (!jcv.isTriviallySigned()), I will refactor everything but this for now.
>> diff --git a/netx/net/sourceforge/jnlp/security/AppVerifier.java b/netx/net/sourceforge/jnlp/security/AppVerifier.java
>> new file mode 100644
>> --- /dev/null
>> +++ b/netx/net/sourceforge/jnlp/security/AppVerifier.java
>> +/**
>> + * An interface that provides various details about an app's signers.
>> + */
>> +public interface AppVerifier {
>> +
>
> 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?
>> + /**
>> + * 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.
>> 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.
>> 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).
>> diff --git a/netx/net/sourceforge/jnlp/security/JNLPAppVerifier.java b/netx/net/sourceforge/jnlp/security/JNLPAppVerifier.java
>> new file mode 100644
>> --- /dev/null
>> +++ b/netx/net/sourceforge/jnlp/security/JNLPAppVerifier.java
>
>> +public class JNLPAppVerifier implements AppVerifier {
>> +
>> + @Override
>> + /* (non-Javadoc)
>> + * @see net.sourceforge.jnlp.security.AppVerifier#hasAlreadyTrustedPublisher
>> + */
>
> Ugh. Just remove this "non-Javadoc".
>
They're gone. =)
>> + public void checkTrustWithUser(JarCertVerifier jcv, JNLPFile file)
>> + throws LaunchException {
>> +
>> + int sumOfSignableEntries = JarCertVerifier.getTotalJarEntries(jcv.getJarSignableEntries());
>> + for (CertPath cPath : jcv.getCertsList()) {
>> + jcv.setCurrentlyUsedCertPath(cPath);
>> + CertInformation info = jcv.getCertInformation(cPath);
>> + if (hasCompletelySignedApp(info, sumOfSignableEntries)) {
>> + if (info.isAlreadyTrustPublisher()) {
>> + return;
>> + }
>> +
>> + AccessType dialogType;
>> + if (info.isRootInCacerts() && !info.hasSigningIssues()) {
>> + dialogType = AccessType.VERIFIED;
>> + } else if (info.isRootInCacerts()) {
>> + dialogType = AccessType.SIGNING_ERROR;
>> + } else {
>> + dialogType = AccessType.UNVERIFIED;
>> + }
>> +
>> + if (SecurityDialogs.showCertWarningDialog(
>> + dialogType, file, jcv)) {
>> + return;
>> + }
>> + }
>> + }
>> +
>> + throw new LaunchException(null, null, R("LSFatal"), R("LCLaunching"),
>> + R("LCancelOnUserRequest"), "");
>> + }
>
> Please consider moving this method out of here - the rest of the class
> seems to fit together, but this one doesnt seem to belong (it touches
> lots of other classes that nothing else in this class does and it
> accepts arguments that are unrelated to anything else in this class).
>
This is along the lines of the bit mentioned above about creating another set of strategy pattern classes. If I go down that route,
I'll be sure to extract this (and others) into those new classes.
>> 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).
>> +public class CertInformation {
>
>> + /** Code signer properties of the certificate. */
>
> Nit: make this a normal comment, or else this might get associated as
> a javadoc comment for the next variable in the file.
>
Ah yes, good call. I think I originally had one for each var but it became a bit too verbose. Thanks for pointing it out!
>> + private ArrayList<String> details = new ArrayList<String>();
>
> I would have gone with a Set of enum here to keep things type-safe and
> avoid duplicates without doing contains checks ourselves.
>
Alright, I had the wrong impression about what you said when we spoke on IRC then. Done!
>> + /**
>> + * Return whether or not the publisher is already trusted.
>> + *
>> + * @return True if the publisher is trusted already.
>> + */
>> + public boolean isAlreadyTrustPublisher() {
>> + return alreadyTrustPublisher;
>> + }
>
> Perhaps you can name this method isPublisherAlreadyTrusted, and leave
> out the comments which are just duplicating this information?
>
Done.
>> +
>> + /**
>> + * Set whether or not the publisher is already trusted.
>> + *
>> + * @param alreadyTrustPublisher
>> + * True if the publisher is to be trusted.
>> + */
>> + public void setAlreadyTrustPublisher() {
>> + alreadyTrustPublisher = true;
>> + }
>
> And this is why you should minimize javadocs: there is no parameter here
> :)
>
Bah this one slipped by as well. Thanks, done.
>> + /**
>> + * Get all the jars this cert has signed along with the number of entries
>> + * in each jar.
>> + * @return
>> + */
>> + public HashMap<String, Integer> getSignedJars() {
>> + return signedJars;
>> + }
>
> If it's possible, avoid returning concrete types. A Map<String, Integer>
> should be sufficient. The less details we specify in the public methods,
> the more we can change this class without affecting others.
>
Will definitely keep this in mind for the future! Done for this and the similar one in JCV. Also changed a param of
JCV#getTotalJarEntries to map for the same reason.
>> +
>> + /**
>> + * Get the details regarding issue(s) with this certificate.
>> + *
>> + * @return A list of issues.
>> + */
>> + public ArrayList<String> getDetails() {
>> + return details;
>> + }
>
> Same here: a List<String> would suffice.
>
I changed this method to account for enums but I still followed the s/ArrayList/List style.
>> + /**
>> + * 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.
>> 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
>
>> +
>> /* (non-Javadoc)
>> - * @see net.sourceforge.jnlp.tools.CertVerifier2#getAlreadyTrustPublisher()
>> + * @see net.sourceforge.jnlp.security.CertVerifier#getAlreadyTrustPublisher
>> */
>
> I would just remove these comments.
>
Done before.
>> + private void checkTrustedCerts(CertPath certPath) throws Exception {
>> + try {
>> + X509Certificate publisher = (X509Certificate) getPublisher(certPath);
>> + KeyStore[] certKeyStores = KeyStores.getCertKeyStores();
>> +
>> + if (CertificateUtils.inKeyStores(publisher, certKeyStores))
>> + certs.get(certPath).setAlreadyTrustPublisher();
>> + X509Certificate root = (X509Certificate) getRoot(certPath);
>> + KeyStore[] caKeyStores = KeyStores.getCAKeyStores();
>> + // Check entire cert path for a trusted CA
>> + for (Certificate c : certPath.getCertificates()) {
>> + if (CertificateUtils.inKeyStores((X509Certificate) c,
>> + caKeyStores)) {
>> + certs.get(certPath).setRootInCacerts();
>> + return;
>> }
>> - } catch (Exception e) {
>> - // TODO: Warn user about not being able to
>> - // look through their cacerts/trusted.certs
>> - // file depending on exception.
>> - throw e;
>> }
>> + } catch (Exception e) {
>> + // TODO: Warn user about not being able to
>> + // look through their cacerts/trusted.certs
>> + // file depending on exception.
>> + throw e;
>> + }
>>
>> - if (!rootInCacerts)
>> - addToDetails(R("SUntrustedCertificate"));
>> - else
>> - addToDetails(R("STrustedCertificate"));
>> - }
>> + // Otherwise a parent cert was not found to be trusted.
>> + certs.get(certPath).addToDetails(R("SUntrustedCertificate"));
>
> Make this a method in the CertificateInformation class instead:
> certs.get(certPath).setUntrusted();
>
Done, even easier with addToDetails removed. :P
>> 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
>> + @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.
> Hope this helps. Thanks again for fixing this.
It does tremendously! Thanks for taking the time to review it!
I will post up a new patch in reply to the original email with the tests I ran and the bugs I found.
Cheers,
Danesh
More information about the distro-pkg-dev
mailing list