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

[Weijun Wang]

Small non-functional changes:

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

1. comments
2. some new language usage, say, <> operator
3. Vector->List, Hashtable->Map

Thanks
Max


On 03/03/2011 08:32 AM, Weijun Wang wrote:
> 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
>>>>>