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

Danesh Dadachanji ddadacha at redhat.com
Fri Apr 6 14:57:22 PDT 2012 

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.

Cheers,
Danesh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jar-signer-tests-02.patch
Type: text/x-patch
Size: 30481 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20120406/452e0cc9/jar-signer-tests-02.patch

More information about the distro-pkg-dev mailing list