Code Review Request : signed module digest verification

[Mandy Chung]

  On 03/10/11 12:20, Sean Mullan wrote:
> 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?

We should start using it :)

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

That's right.  ByteArrayInputStream will read hashLength number of bytes 
if available.

Should you check the returned value == hashLength?  Is it possible that 
the signedData is corrupted (i.e. the number of bytes is less than 
hashLength)?

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

What I meant was that it should check if expectedHashes.size() < 2 and 
throws an exception in case the signedData is invalid.   It's fine to 
add that check 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

26 of them are in ModuleFileFormat.java and 1 in Library.java.  Do you 
mind fixing them?

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

That's fine with me.

Mandy

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