Code Review Request : signed module digest verification

[Sean Mullan]

On 3/10/11 2:01 AM, Mandy Chung wrote:
> On 3/9/11 6:55 AM, Sean Mullan wrote:
>> See webrev:
>> http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/digest-verification/webrev.00/
>>
> ModuleFileFormat.java
> line 1727: better to catch specific exception rather than all types.
> IOException and CertificateException, any other?

Sure. Is it ok to use multi-catch?

> line 1786: read() may not read hashLength number of bytes.
> should you use readFully() instead?

I don't think it is necessary. The underlying InputStream is a 
ByteArrayInputStream so it should not block, right?

> line 1795: looks like verifyHashes is not used. This method is for
> one phase verification?

Yes. Right now, SimpleLibrary.install reads a module file in two phases: 
readStart, readRest. But if we ever read the entire file in one go (see 
readModule), then this method would be useful.

> line 1804: should it do the sanity check on expectedHashes? Perhaps
> do the check when parseSignedData returns.

I could but I feel like that check is really part of verifying the hashes, so it 
belongs in the verifyHashesStart method.

> line 1823: the space in the argument type "byte []" can be removed.

Sure, though I should point out that there is some inconsistency in how this 
convention is followed in the org/openjdk/jigsaw package:

$ grep "byte \[\]" *.java | wc -l
27
$ grep "byte\[\]" *.java | wc -l
79

> ModuleFileVerifier.java
> line 74: The verifyHashes method is for the future?

See my comment above.

> line 87: Would be better to name this method with what's being
> verified. What about verifyModuleMetaDataHashes?

The names are consistent with the ModuleFileFormat.Reader.readStart and readRest 
methods. In other words, they verify the hashes of the data that those methods 
have read. I prefer those names in order to be consistent. I'll add some 
comments to make it more clear.

Thanks,
Sean

>
>
> Other than that, looks good to me.
> Mandy
>
>> With this change, signed module [1,2] digests are now verified as part of the
>> signed module installation and verification. This does not address the
>> ModuleFileFormat.Reader issues I previously raised [3]; those will be
>> addressed in a subsequent webrev.
>>
>> Thanks,
>> Sean
>>
>> [1]: http://cr.openjdk.java.net/~mullan/jigsaw/signed-module-file-format
>> [2]: http://cr.openjdk.java.net/~mullan/jigsaw/signed-module-functional-spec
>> [3]: http://mail.openjdk.java.net/pipermail/jigsaw-dev/2011-March/001185.html
>