[RFC][icedtea-web] Unit tests for the new updates to JarSIgner (incomplete)
ddadacha at redhat.com
Fri Apr 6 15:40:29 PDT 2012
On 06/04/12 05:57 PM, Danesh Dadachanji wrote:
> Hi Omair,
> Thanks for the review!
> On 04/04/12 07:36 PM, Omair Majid wrote:
>> Hi Danesh,
>> On 04/02/2012 04:46 PM, Danesh Dadachanji wrote:
>>> I've been working on updating JarSigner to be able to handle multiple
>>> signers properly. Currently, it mashes and saves all of the signers'
>>> properties (flags for expired/badly signed/not yet valid) as one. In
>>> other words, if one signer has expired, all signers are seen by
>>> IcedTea-Web as expired. If one has signing issues, then they all have
>>> signing issues. This is definitely not an ideal solution since we're
>>> telling users that verifiable applications are not verified.
>>> The attached patch contains a set of unit tests for the (former)
>>> verifyJar method. I'm going to avoid including the actual code for these
>>> changes just yet because the entire class isn't fully cleaned up. Here's
>>> a bit of background info on what I've changed though.
>>> * What verifyJar should be doing:
>>> My understanding of the idea behind verifyJar is that it does 2 somewhat
>>> separate tasks. First, given a jar, iterate over all of its entries. For
>>> every entry that can be signed, go through the list of signers and see
>>> if any have issues. If they do, setup the flags accordingly (issues
>>> could be expiry/bad key usage/not yet valid start date etc). Also, keep
>>> track of the number of entries each signer has signed, as well as the
>>> total number of possible signable entries. The second step is going
>>> through all of the signers after all the entries have been iterated over
>>> to see if there is a common signer that has signed them all. If there is
>>> any entry that is not signed but should be, or if there is one entry
>>> that does not share a common signer between all other entries, then this
>>> jar is considered UNSIGNED. If all entries (that should be signed) share
>>> a common signer, then the signer itself is checked. If the signer has
>>> signing issues, the jar is considered SIGNED_NOT_OKAY, if it has no
>>> signing issues, the jar is considered SIGNED_OK.
>> This does make sense to me. Something that sort of jumps out at me is
>> that these two responsibilities are fairly distinct. It seems like it
>> might be worth it to split it into two distinct objects, each taking
>> care of one responsibility. That is, one objects determines if it should
>> be considered signed or unsigned, and another checks that (if signed)
>> the signing certificate is good.
> Hmm interesting idea, it makes sense to separate the checking/setup of the signers and the checking of the jar. However, I would like
> to get back to you on this once I have wondered into the mess that is verifyJars(). I think there is definitely potential here to move
> bits into another class but I don't fully know what is needed by the rest of JarCertVerifier. Until I do, I would rather avoid
> refactoring this until it is needed.
>>> * Summary of my changes to verifyJar:
>>> I've split verifyJar into 2 methods, all the logic/implementations from
>>> line 265 and onwards is now in a method named verifyJarEntryCerts.
>>> Here's a quick snippet of its javadoc and method declaration:
>>> * Checks through all the jar entries for signers, storing all the
>>> * ones in the certs hash map.
>>> * @param jarHasManifest Whether or not the associated jar has a
>>> * @param entries The list of entries in the associated jar.
>>> * @return If there is at least one signable entry that is not
>>> signed by a
>>> * common signer, return UNSIGNED. Otherwise every signable entry is
>>> * signed by at least one common signer. If the signer has no issues,
>>> * return SIGNED_OK. If there are any signing issues, return
>>> * @throws Exception Will be thrown if there are issues with entries.
>>> VerifyResult verifyJarEntryCerts(boolean jarHasManifest,
>>> Vector<JarEntry> entries) throws Exception
>>> The reason I've split this method up is so that JarSigner can be made
>>> unit testable. The method verifyJar takes in a jar name and
>>> creating/commiting one to upstream isn't suitable. Instead, I've created
>>> an extra helper method that takes in a list of jar entries and tests
>>> everything the original code tests. The new unit test class then hard
>>> codes these entries, along with their signers, to make this method
>> Good stuff!
>>> For the entries' signers, I've created a helper class that creates
>>> X509Certificate objects with specified domain names/start dates/validity
>>> periods. This code is essentially taken from
>>> sun.security.tools.KeyTool#doGenKeyPair (which calls
>>> KeyTool#doSelfCert), I've removed the bits that aren't needed
>>> (KeyStore/alias etc) and left the bare minimum that is needed to create
>>> an X509Certificate. I'm not 100% sure of the copyright header that is
>>> needed for this class so I used the same one provided by KeyTool from
>>> here  (with s/2011/2012). I would appreciate any insight as to if
>>> this is correct. =)
>> IANAL. That said, you should note all the copyright owners. I believe
>> that would be Oracle (since they own the original code in KeyTool), and
>> Red Hat (since that's who owns the newer/modified code). Since both
>> Oracle and Red Hat have licensed the code under GPLv2+Classpath, it
>> should be okay to replace this version of the header with the one more
>> commonly used in icedtea-web (they do refer to the same license after all).
>>> The unit tests themselves are pretty self-explanatory, to save on
>>> space/repetition, I've kept testing 1 entry and then 3 entries with the
>>> same scenario under one method (with different asserts). I think this is
>>> cleanest but am open to criticism! I've covered everything I could think
>>> of except for the badKeyUsage/badExtendedKeyUsage/badNetscapeCertType
>>> cases, I have yet to figure out how to setup certs using them. Is there
>>> any corner case I've missed?
>>> Comments are very appreciated!
>> Some more whitespace might be nice (to separate the asserts from the
>> code that sets up the preconditions and code that tests the object). We
>> may also want to split off the separate responsibilities into separate
>> classes. Still, I think it looks fantastic for now. Nicely done!
> Separate responsibilities WRT what exactly?
> The whitespaces definitely do help! =)
> I've attached a new patch that loosens up the code a bit, I've padded newlines above (and below) the chunk of asserts. There are a few
> more modifications as follows.
> 1) The new patch also brings in the changeset that renames JarSigner to JarCertVerifier so hopefully it won't throw reviewers off
> too much.
> 2) In the tests that run 1 entry than 3 entries, I've reset JarCertVerifier to a new instance when the three-entry vector is used. I
> think this may cause confusion later on because there would have been cases where the JarCertVerifier would have totalSignableEntries =
> 4 instead of 3.
> 3) Lastly, I've added a new set of asserts using the following two methods:
> * Checks if the provided cert path has signed any of the jars associated
> * with the cert verifier.
> * @param certPath The cert path of the potential signer.
> * @return Whether or not the cert path represents one of the signers of
> * a jar associated with the verifier.
> boolean hasJarSigner(CertPath certPath)
> * Calculate the number of signers all the jars have in total.
> * Note this is not the number of common signers across all jars. It is
> * the number of common signers each jar has with respect to itself,
> * totalled together across the other jars.
> * @return The number of signers that have signed at least one jar entirely.
> int numSigners()
> These check that the right number of certs were added and clarifies that the right signer's cert path was stored in JarCertVerifier.
Knew I forgot something..Fridays :/
More information about the distro-pkg-dev