jpkg enhancements to create signed modules

[Vincent Ryan]

Thanks for your review.


On 05/11/10 19:53, Mark Reinhold wrote:
>> Date: Mon, 10 May 2010 17:47:53 +0100
>> From: vincent.x.ryan at oracle.com
> 
>> Please review these code changes to support the creation of signed modules:
>>
>>   http://cr.openjdk.java.net/~vinnie/6951048/webrev.00/webrev/
>>
>> It adds the following new options to the jpkg tool:
>>
>>   -S, --signer  <ID>         : module signer's identifier
>>   -k, --keystore <location>  : module signer's keystore location
>>   -t, --storetype <type>     : module signer's keystore type
>>   --nosign                   : do not sign the module
>>   --nopassword               : do not prompt for a keystore password
>>
>> Appropriate default values are supported and keystore passwords may be
>> supplied to jpkg by redirecting standard input.
> 
> Thanks; this is a good start.  Some comments:
> 
> FileConstants.java
> 
>   [156] We might support XML_DSIG or PGP someday, but we have no
>   immediate plans for those, so please remove these elements from the
>   SignatureType enum.

OK.


> 
> ModuleFileFormat.java
> 
>   Please use "//" rather than "/* .. */" for all non-javadoc comments.
> 
>   [486, and elsewhere] Indent non-initial lines of a long argument list,
>   and put the following brace on its own line:
> 
>     public void setSignatureMechanism(ModuleFileSigner mechanism,
>                                       ModuleFileSigner.Parameters parameters)
>     {
> 

OK.


>   [545] Having to shift the whole file here seems unfortunate.  Is there
>   no way to predict the size of the signature in advance?  Does it depend
>   upon anything other than the number of section hashes?

The default format for the module signature is PKCS #7 SignedData type [1],
which is an ASN.1 BER encoding of nested tag-length-value data structures.
Unfortunately it is difficult to predict its exact size in advance without
performing the actual encoding.

The design choices are:

    1) write the signature twice - first with a dummy signature to determine
       the exact size and then later with the true signature. No shift.
-or-
    2) write the signature once, at the end of the process. Shift required.

The current implementation involves 1) _plus_ the shift. I can certainly
improve on that.



[1] http://tools.ietf.org/html/rfc2315#section-9.1


> 
> Packager.java
> 
>   Why is the --nosign option needed?  Not signing a module should be the
>   default; signing should be done only when requested.
> 

I'll reverse that.


>   [137, 140] Variables named "nofoo" are confusing, and lead to clunky
>   double-negative constructions such as "!nofoo".  Please change these
>   variables to their positive forms.

OK.


> 
>   [200] The new signing code makes this method way too long to be
>   readable.  Please refactor it into one or more subsidiary methods.

OK.

> 
> Finally, you'll need a unit test for all this new functionality.
> 

I've extended an existing unit test. I'll include that in my next webrev.



> - Mark