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.w...@oracle.com
