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

[Omair Majid]

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.

> * 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!

> 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!

Cheers,
Omair