code review request: 7012160: read SF file in signed jar in streaming mode

[Sean Mullan]

Max,

A few more comments. Did you consider instead creating an internal subclass of 
Manifest (ex: sun.security.util.SignatureFileManifest) and adding the update 
method and other logic to the subclass? I think this might be a better design. 
Even though we would have to be careful to monitor changes to the Manifest class 
in the future, it avoids making API changes to the Manifest class.

* SignatureFileVerifier

[121]: Can you rename this to "needSignatureFile"? I think that matches the 
description of the method better.

[138]: Same comment as above. I think setSignatureFile is a better name. Also 
can you add a comment describing what this method does?

* JarVerifier

[267]: this should be printed in debug only

--Sean


On 2/10/11 3:08 AM, Weijun Wang wrote:
>
>
> On 02/02/2011 04:42 AM, Sean Mullan wrote:
>> Hi Max,
>>
>> This is looking good. I'm about halfway through. Here are some code
>> comments to start with. I think we'll need a couple more rounds of code
>> reviews before we finish so I wanted to get these to you now.
>>
>> * JarFile
>>
>> [329-330]: How about adding a SignatureFileVerifier.isBlock method
>> instead? That would be cleaner.
>
> Good.
>
>>
>> [338]: I believe you can replace the argument to getBytes with the local
>> variable e :
>>
>> s/getBytes(getJarEntry(names[i])),/getBytes(e),
>
> Correct.
>
>>
>> * PKCS7
>>
>> Can we avoid duplicating code across SignerInfo and PKCS7Verifier? Can
>> SignerInfo.verify invoke methods of PKCS7Verifier?
>
> I thought SignerInfo.verify() could be removed at all, and only keep it there
> for your code review. Yes. It can be rewritten to --
>
> SignerInfo verify(PKCS7 block, byte[] data)
> throws NoSuchAlgorithmException, SignatureException {
>
> PKCS7.PKCS7Verifier p7v = PKCS7.PKCS7Verifier.from(block, this);
> if (p7v == null) return null;
> if (data == null) {
> try {
> data = block.getContentInfo().getContentBytes();
> } catch (IOException e) {
> throw new SignatureException(
> "IO error verifying signature:\n" +
> e.getMessage());
> }
> }
> p7v.update(data, 0, data.length);
> return p7v.verify();
> }
>
>>
>> [538-540]: fields should be marked final
>
> OK.
>
>>
>> [542]: Why is a static method necessary here? Since the method always
>> creates a PKCS7Verifier object, it doesn't seem like it is that useful
>> or necessary.
>
> The original verify() method in SignerInfo sometimes return null and sometimes
> throws an Exception. I don't exactly understand why there is such a difference
> and use this static method to precisely mimic the behavior.
>
>>
>> [661-665]: replace this code with MessageDigest.isEqual.
>
> Yes.
>
> All changes are trivial except for the new SignerInfo.verify() method. I guess a
> webrev is not needed.
>
> Thanks
> Max
>
>>
>> --Sean
>>
>>
>> On 1/14/11 3:31 AM, Weijun Wang wrote:
>>> Hi Sean
>>>
>>> http://cr.openjdk.java.net/~weijun/7012160/webrev.00/
>>>
>>> I've made changes to the following classes to enable streaming mode SF
>>> file
>>> reading:
>>>
>>> - java/util/jar/JarVerifier.java:
>>>
>>> 1. New verifyBlock method.
>>>
>>> 2. Change the constructor from JarVerifier(byte[]) to JarVerifier(byte[],
>>> Manifest). In SignatureFileVerifier.processImpl(), if we already
>>> confirm the
>>> *-Digest-Manifest header in the SF file matches the whole MANIFEST.MF,
>>> there'se
>>> no need to parse the rest of the SF file, since we can be sure that
>>> entries in
>>> the SF file are identical to those in MANIFEST.MF. Of course, the
>>> content of the
>>> SF file still needs to be fed into PKCS7Verifier to verify the signature.
>>>
>>> - java/util/jar/JarFile.java:
>>>
>>> Read DSA file in byte[] and SF file in InputStream, and call
>>> JarVerifier.verifyBlock() to verify.
>>>
>>> - java/util/jar/Manifest.java:
>>>
>>> Adding update(byte[]) to read manifest in streaming mode. This is a
>>> new public API.
>>>
>>> - sun/security/pkcs/PKCS7.java:
>>>
>>> New PKCS7Verifier class to verify SignedData in streaming mode. I
>>> basically
>>> divide the SignerInfo.verify(PKCS7 block, byte[] data) method into 3
>>> parts and
>>> make them the 3 methods of this class.
>>>
>>> - sun/security/util/SignatureFileVerifier.java:
>>>
>>> Rewrite the processImpl(*) method to make use of new methods in PKCS7
>>> and Manifest.
>>>
>>> No new regression tests, use existing ones.
>>>
>>> I've tried NetBeans profiler to look at the memory. The program simply
>>> calls
>>> JarSigner.main(new String[]{"-verify", "x.jar"}) and the signed jar
>>> x.jar has
>>> 10000 files inside.
>>>
>>> Before After
>>> byte[] 3.6MB 2.8MB
>>> char[] 2.0MB 1.3MB
>>> String 1.1MB 650KB
>>>
>>> So it does have some difference.
>>>
>>> Thanks
>>> Max
>>>
>>>
>>> -------- Original Message --------
>>> *Change Request ID*: 7012160
>>> *Synopsis*: read SF file in signed jar in streaming mode
>>>
>>>
>>> === *Description*
>>> ============================================================
>>> When a signed jar is verified, its SF file is read into a byte array and
>>> verified against the signature. When there are many files in the jar,
>>> the SF
>>> file can be very big. It will be better if the file can be read in
>>> streaming mode.
>>>
>>> *** (#1 of 1): 2011-01-13 12:23:25 GMT+00:00 weijun.wang at oracle.com
>>>