[RFC][icedtea-web] Major rework of JarCertVerifier
Omair Majid
omajid at redhat.com
Tue Aug 21 16:00:09 PDT 2012
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.
> 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?
> 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.
> 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.
> + 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?
> -
> - 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.
> @@ -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?
> 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.
> + /**
> + * 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?
> 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.
> 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?
> 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".
> + 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).
> 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.
> +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.
> + 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.
> + /**
> + * 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?
> +
> + /**
> + * 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
:)
> + /**
> + * 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.
> +
> + /**
> + * 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.
> + /**
> + * 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.
> 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.
> + 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();
> 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?
> + @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?
Hope this helps. Thanks again for fixing this.
Thanks,
Omair
More information about the distro-pkg-dev
mailing list