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

[Weijun Wang]

Webrev updated

http://cr.openjdk.java.net/~weijun/7012160/webrev.01/

Thanks
Max


On 02/17/2011 02:12 AM, Sean Mullan wrote:
> 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
>>>>