Support for timestamped signed modules

[Sean Mullan]

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.

Vinnie is the best person to review it but he is busy with JDK 7 right now. I 
would like to request that if all jprt tests pass, I push it now and he will 
review later.

> org/openjdk/jigsaw/ModuleFileFormat.java
> There are some leftover commented lines to be removed (L614, L671, etc)

Yes, these were from debugging the signature length issue. I will remove them.

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

Sure, I moved this code out of ModuleFileFormat because that file was getting 
too large. I will rename this to SignedModule and rename the static classes to 
PKCS7Signer, PKCS7Verifier, ...

> L256-260: should it only call signerCert.getNotAfter() if there is a timestamp?

Yes, but I think I was thinking ahead here. I will need to remember if the 
certificate is expired. There are some additional changes that will come later. 
Right now when there is a validation failure, the SignatureException is not 
helpful enough. We will need to provide additional information about the 
validation failure (cert expired, revoked, etc) so that security dialogs, etc 
can be presented and displayed according to policies. I am thinking about a 
callback mechanism of some sorts - TBD.

I'll adjust the code for now though.

> And also tsaValidator is only needed when there is a timestamp.

Yes, good catch.

> L120: looks like a debugging code to be removed?

Yes.

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

It's possible this code may get invoked under a SecurityManager at some later 
point, maybe when untrusted applets are being installed. So I'd rather leave the 
doPrivs in for now.

> Otherwise, System.getProperty also needs to be wrapped by doPrivileged? I'm
> thinking that you can use try-with-resource resulting in cleaner code.

Yes, getProperty needs to be wrapped in doPriv. Good catch.

Any good pointers on try-with-resource? I haven't used it yet.

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

Ok, sure. I wanted to make the long forms of these options consistent with 
jarsigner and keytool as I think developers familiar with those tools would 
appreciate that. I think all of the short options need to be revisited at some 
later point.

> L235-235: (X509Certificate[])signerEntry.getCertificateChain() should be in the
> same line. Perhaps you can break tsaURI in another line.

Ok.

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

Hmm, yes but let me think about that a bit more.

> 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];

Yes, that looks better.

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

Yes, well the first option anyway as it speeds up the random number generation.

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

Sure.

Will post new webrev later today.

Thanks,
Sean

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