-
Notifications
You must be signed in to change notification settings - Fork 141
8339280: jarsigner -verify performs cross-checking between CEN and LOC #2235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8339280: jarsigner -verify performs cross-checking between CEN and LOC #2235
Conversation
👋 Welcome back fferrari! A progress list of the required criteria for merging this PR into |
@franferrax 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 30 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jerboaa, @alexeybakhtin) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
This backport pull request has now been updated with issue from the original commit. |
/issue add 8353299 |
/issue add 8367782 |
@franferrax |
@franferrax |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove changes to Resources_de.java
, Resources_ja.java
and Resources_zh_CN.java
since those are part of a different bug and usually get updated in bulk. Looks good otherwise.
@franferrax Shouldn't we update the copyright years in src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java, src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java, and src/jdk.jartool/share/man/jarsigner.1 files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks OK to me.
Not in a backport. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you for your reviews, @alexeybakhtin and @jerboaa. |
/approval request JDK-8339280 enhances the jarsigner utility with cross-validation of JAR entries. Subsequent test updates (JDK-8353299 & JDK-8367782) are included for better reliability and coverage. Please find details about the testing in the pull request description. |
@franferrax |
/approve yes |
@jerboaa |
/integrate |
@franferrax |
/sponsor |
Going to push as commit 3e41a78.
Your commit was automatically rebased without conflicts. |
@jerboaa @franferrax Pushed as commit 3e41a78. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi, this is a backport of openjdk/jdk@bbd5b17. The backport is not clean because of the following reasons.
Internationalization files
21u doesn't have JDK-8345940: Migrate security-related resources from Java classes to properties files (openjdk/jdk@9a49418), so changes from
resources/jarsigner.properties
, were applied toResources.java
.Also, internationalized messages were added later, so they have been recovered from JDK-8359761: JDK 25 RDP1 L10n resource files update (openjdk/jdk@da7080f):New messages inresources/jarsigner_de.properties
were applied toResources_de.java
New messages inresources/jarsigner_ja.properties
were applied toResources_ja.java
New messages inresources/jarsigner_zh_CN.properties
were applied toResources_zh_CN.java
To convert these messages, I created the PropertiesToResources.java small program.Man pages
21u doesn't have JDK-8344056: Use markdown format for man pages (openjdk/jdk@475feb0), so changes from
jarsigner.md
, were applied tojarsigner.1
.Related issues ("relates to" Jira issue links)
JDK-8353299 (openjdk/jdk@acd4da4) and JDK-8367782 (openjdk/jdk@1b9a116) were also included as part of this backport. They are test-only changes that improve the reliability and coverage of
VerifyJarEntryName.java
.Since
test/hotspot/jtreg/runtime/cds/appcds/SignedJar.java
is not failing after the backport, JDK-8353330 was not included.Testing
tier1
run from the GitHub actions (all passed), I ran a regression using the following categories and individual tests:test/hotspot/jtreg/runtime/cds/appcds/SignedJar.java
test/jdk/java/security/SignedJar
test/jdk/java/util/jar
test/jdk/jdk/security/jarsigner
test/jdk/sun/security/pkcs/pkcs7
test/jdk/sun/security/tools/jarsigner
VerifyJarEntryName.java
, created for this issuetest/jdk/sun/security/tools/keytool
No regressions were found against the current
master
branch (10df11a).Progress
Warning
8339280: jarsigner -verify performs cross-checking between CEN and LOC
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk21u-dev.git pull/2235/head:pull/2235
$ git checkout pull/2235
Update a local copy of the PR:
$ git checkout pull/2235
$ git pull https://git.openjdk.org/jdk21u-dev.git pull/2235/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2235
View PR using the GUI difftool:
$ git pr show -t 2235
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk21u-dev/pull/2235.diff
Using Webrev
Link to Webrev Comment