Support for timestamped signed modules

[Sean Mullan]

New webrev: http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/timestamp/webrev.01/

I've made all of your suggested changes, except:

 > Both timestamp-test.sh and TimestampTest.java hardcodes the name of the module
 > library, module name, and location. Would it be better to pass these hardcoded
 > names from the shell test to TimestampTest as system properties or input
 > arguments? These values might unlikely be changed but I would rather avoid
 > having a hardcoded name in multiple files for the single test.

These two tests are tightly coupled. The script is used to create the keystore 
and other binaries (to avoid the "no binaries under source control" rule) used 
by the Java program. I could change them all to command-line arguments (there 
would be many) but it seems more sensible for me to spend the time to eliminate 
the script entirely. But that's a bit more work and I'll tackle that as an 
overall test cleanup task a bit later.

 > sun/security/pkcs/SignerInfo.java
 > L470: maybe assert tsa.length == 1, or it is guaranteed to be true??

An assertion seems incorrect here, since this is verifying data sent over the 
network by the server. I've added a check and it now throws an exception if 
there is not exactly one signer.

--Sean

On 4/7/11 5:13 PM, Mandy Chung wrote:
> Sean,
>
> I looked at all files and will count on Vinnie or some other who is familiar
> with the security implementation to review it.
>
> org/openjdk/jigsaw/ModuleFileFormat.java
> There are some leftover commented lines to be removed (L614, L671, etc)
>
> org/openjdk/jigsaw/PKCS7.java
> Would it be better to rename this to a different class name (PKCSSupport?) to
> avoid name conflict with sun.security.pkcs.PKCS7?
>
> L256-260: should it only call signerCert.getNotAfter() if there is a timestamp?
> And also tsaValidator is only needed when there is a timestamp.
> L120: looks like a debugging code to be removed?
> L150: is the loadCACertsKeyStore method only called by the jmod tool? If so,
> there is no security manager enabled and do you still need doPrivileged?
> Otherwise, System.getProperty also needs to be wrapped by doPrivileged? I'm
> thinking that you can use try-with-resource resulting in cleaner code.
>
> org/openjdk/jigsaw/cli/Packager.java
> At some point, we need to look at the interface of these new command-line tools.
> I was reading the Solaris guideline for the command-line utilities some time ago
> and it states that every short-option should have exactly one corresponding
> long-option and every long-option should have exactly one corresponding
> short-option. I don't know if these tools should follow this guideline. Given
> that not all long options have its corresponding short option in the current
> implementation, it's fine that you remove a few short-option in this changeset.
>
> L235-235: (X509Certificate[])signerEntry.getCertificateChain() should be in the
> same line. Perhaps you can break tsaURI in another line.
>
> sun/security/pkcs/SignerInfo.java
> L470: maybe assert tsa.length == 1, or it is guaranteed to be true??
>
> sun/security/timestamp/TSResponse.java
> L285: Is failureInfo of varying length (returned from
> statusInfo.data.getUnalignedBitString())? Is there a better way to check this
> rather than having the isSet() method to throw ArrayIndexOutOfBoundsException?
> How about:
> return position >= 0 && position < failureInfo.length && failureInfo[position];
>
> test/org/openjdk/jigsaw/cli/TimestampTest.java
> L67, 68: debugging code - if you want to leave them there, maybe better to
> include the debugging capability in the test such as adding these 2 options
> before adding signingArgs when invoking Packager.run (L164) if debugging is
> enabled (e.g. by an input argument to the main method) - rather than
> uncommenting them and recompile.
>
> Both timestamp-test.sh and TimestampTest.java hardcodes the name of the module
> library, module name, and location. Would it be better to pass these hardcoded
> names from the shell test to TimestampTest as system properties or input
> arguments? These values might unlikely be changed but I would rather avoid
> having a hardcoded name in multiple files for the single test.
>
> Mandy
>
> On 04/06/11 12:44, Sean Mullan wrote:
>> Please review the following webrev which adds support for timestamped signed
>> modules.
>>
>> webrev: http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/timestamp/webrev.00/
>>
>> A new option (tsa) has been added to jpkg to specify the timestamping
>> authority to use. I have also renamed some of the signing options to be more
>> descriptive. Here is an example of creating a timestamped signed module :
>>
>> jpkg --sign --alias duke --keystore keystore \
>> --tsa https://timestamp.geotrust.com/tsa \
>> -m lib/test.security jmod test.security
>>
>> A lot of the changes involved restructuring so that jars and modules could
>> share the same timestamping code.
>>
>> Thanks,
>> Sean
>