Support for timestamped signed modules

[Mandy Chung]

  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