[RFC][icedtea-web] Unit tests for the new updates to JarSIgner (incomplete)

[Danesh Dadachanji]

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[1] 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
>>> common
>>> * ones in the certs hash map.
>>> * @param jarHasManifest Whether or not the associated jar has a
>>> manifest.
>>> * @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
>>> SIGNED_NOT_OK.
>>> * @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
>>> testable.
>>
>> Good stuff!
>>
>
> Thanks!
>
>>> 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 [2] (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[1] 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 :/

[1] http://icedtea.classpath.org/hg/icedtea-web/rev/cde6d59a2901

> Cheers,
> Danesh