8341775: Duplicate manifest files are removed by jarsigner after signing#22222
8341775: Duplicate manifest files are removed by jarsigner after signing#22222driverkt wants to merge 23 commits intoopenjdk:masterfrom
Conversation
… but potentially *multiple* manifest files of different cases, print a warning before selecting the first one and ignoring the rest.
|
👋 Welcome back kdriver! A progress list of the required criteria for merging this PR into |
|
@driverkt This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 17 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@haimaychao would you mind taking a look please? |
Webrevs
|
|
The original code uses zf.getEntry() first which is direct and may not need to iterate over all entries in the ZIP file, and it does not issue a warning for multiple manifest entries. The new change uses the zf.stream() approach to iterate on the entire ZIP file first, and will it be more costly for a large archives? But to be able to issue a warning, your change looks reasonable to me. |
|
Would you consider adding a test case to test the new warning message? |
Thank you. I agree there is some performance penalty, but (as you say) it is necessary to be able to issue the warning. |
See my comment on the JBS issue. I'm not sure how practical adding one would be. Do you have any suggestions for the best way to go about it? |
|
I’d like to suggest creating a test program (for better long term support) that generates a JAR file with multiple manifest entries and then uses JarSigner.Builder() and JarSigner.sign(). The JarSigner.sign() will ultimately invoke getManifestFile(), ensuring that the new warning about multiple manifest entries is emitted. |
Dynamically creating the needed jar files to perform a test would depend on the file-system of the platform (ie - macOS cannot create two different files with the same name but different cases). Furthermore, we cannot create a jar with two different META-INF directories at the root, both with two different all-caps MANIFEST.MF files dynamically. Unless I am mistaken. |
Here's a unit test which produces a JAR file with a duplicated META-INF/MANIFEST.MF: @Test
public void doubleManifest() throws IOException {
var name = "META-INF/MANIFEST.MF";
var nameBytes = name.getBytes(StandardCharsets.UTF_8);
var baos = new ByteArrayOutputStream();
try (var zo = new ZipOutputStream(baos)) {
// ZOS does not allow duplicated name, use lowercase
zo.putNextEntry(new ZipEntry(name.toLowerCase(Locale.ROOT)));
zo.putNextEntry(new ZipEntry(name));
}
byte[] zip = baos.toByteArray();
// Byte buffer to navigate the ZIP
ByteBuffer bb = ByteBuffer.wrap(zip).order(ByteOrder.LITTLE_ENDIAN);
// Offset of the start of the END header
int endOff = zip.length - ZipEntry.ENDHDR;
// Offset of the first CEN header
short cenOff = bb.getShort(endOff + ZipEntry.ENDOFF);
// Write uppercase name to first CEN entry
bb.put(cenOff + ZipEntry.CENHDR, nameBytes, 0, nameBytes.length);
// Write the file to disk
Path zipFile = Path.of("double-man.jar");
Files.write(zipFile, zip);
// Verify that ZipFile reads duplicated MANIFEST files
try (var zf = new ZipFile(zipFile.toFile())) {
List<? extends ZipEntry> entries = Collections.list(zf.entries());
assertEquals(2, entries.size());
assertEquals(name, entries.get(0).getName());
assertEquals(name, entries.get(1).getName());
}
} |
|
Two comments:
|
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
|
@driverkt This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
/open |
|
@driverkt This pull request is already open |
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
@wangweij: See the note that is part of the PR description. I've copied it below for convenience: --8<-- AFAICT, there isn't currently a way to access the Main class for logging/output (which incidentally also uses |
|
What I meant is that besides using |
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
openjdk
bot
removed
the
merge-conflict
| jar.treated.unsigned=WARNING: Signature is either not parsable or not verifiable, and the jar will be treated as unsigned. For more information, re-run jarsigner with debug enabled (-J-Djava.security.debug=jar). | ||
| jar.treated.unsigned.see.weak=The jar will be treated as unsigned, because it is signed with a weak algorithm that is now disabled.\n\nRe-run jarsigner with the -verbose option for more details. | ||
| jar.treated.unsigned.see.weak.verbose=WARNING: The jar will be treated as unsigned, because it is signed with a weak algorithm that is now disabled by the security property: | ||
| multiple.manifest.warning.=Duplicate manifest entries were detected in the jar file. JarSigner will operate on only one and the others will be discarded. |
There was a problem hiding this comment.
Shall we have a a past-tense phrase to make it clearer that the extra entries were actually deleted? Something like:
"Duplicate manifest entries were detected in the JAR file. JarSigner operated on only one, and the others have been discarded."
There was a problem hiding this comment.
Maybe. When this warning shows up, the extra entries have already been removed.
| certificate's expiration date (`YYYY-MM-DD`) or after any future revocation | ||
| date. | ||
|
|
||
| hasMultipleManifests |
There was a problem hiding this comment.
Suggest to place the new warning in alphabetical order.
|
/integrate |
|
Going to push as commit d809033.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
6 participants
JDK-8341775: In the case where there is a single META-INF directory but potentially multiple manifest files of different cases, print a warning before selecting the first one and ignoring the rest (the current behavior should be maintained).
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22222/head:pull/22222$ git checkout pull/22222Update a local copy of the PR:
$ git checkout pull/22222$ git pull https://git.openjdk.org/jdk.git pull/22222/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22222View PR using the GUI difftool:
$ git pr show -t 22222Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22222.diff
Using Webrev
Link to Webrev Comment