RFR: 8339280: jarsigner -verify performs cross-checking between CEN and LOC [v13]

[Hai-May Chao]

On Tue, 25 Mar 2025 20:13:16 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update test with more ZipEntry in the jar
>
> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 1158:
> 
>> 1156:                 if (cenEntry == null) {
>> 1157:                     crossChkWarnings.add(String.format(rb.getString(
>> 1158:                             "Entry.missing.in.JarFile.1"), entryName));
> 
> Would it be more precise to change this warning to "Entry %s is present when reading via JarInputStream but missing when reading via JarFile"?

Updated this warning as suggested.

> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 1163:
> 
>> 1161: 
>> 1162:                 readEntry(jis);
>> 1163:                 try (InputStream cenInputStream = jarFile.getInputStream(cenEntry)) {
> 
> What if this returns null? Shouldn't you also emit a warning that the entry was read by JarInputStream but not JarFile? Also, I think `readEntry()` would throw an NPE.

Yes, added checking on `cenInputStream` to emit a warning if it is null.

> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 1241:
> 
>> 1239:         boolean locHasSigners = locSigners != null;
>> 1240: 
>> 1241:         if (cenHasSigners && locHasSigners) {
> 
> So, it's ok if one entry has code signers but the other doesn't?

Fixed. Added the checking.

> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 1245:
> 
>> 1243:             List<CodeSigner> locSignerList = Arrays.asList(locSigners);
>> 1244: 
>> 1245:             if (!cenSignerList.equals(locSignerList)) {
> 
> I think you can just call `Arrays.equals()` here.

Done.

> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 1247:
> 
>> 1245:             if (!cenSignerList.equals(locSignerList)) {
>> 1246:                 crossChkWarnings.add(String.format(rb.getString(
>> 1247:                         "signature.mismatch.for.entry.1.when.comparing.jarfile.and.jarinputstream"),
> 
> "Signature mismatch" is not accurate in my opinion. This is really just about the code signers. Can we change this warning to "Code signers are different for entry %s when reading from JarFile and JarInputStream".
> 
> I like the words "reading from" instead of "comparing" as it seems to better describe what the JarFile and JarInputStream APIs for and how to diagnose the issue.

Updated this warning as suggested.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296602
PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296685
PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296741
PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296799
PR Review Comment: https://git.openjdk.org/jdk/pull/23532#discussion_r2013296907