8309841: Jarsigner should print a warning if an entry is removed #19599

wangweij · 2024-06-07T15:11:29Z

There ~~are two~~ is one changes:

In jarsigner -verify, check a .SF file contains un-existing entries and print them out as

Warning: nonexistent signed entries detected: [a]

~~2. In JarSigner::sign0, when creating a new .SF file, only include signed file entries.~~

Update: Even when the JAR file is re-signed, the hash entry for the missing file will be in the new .SF file. There is no way to tell if this is for a file entry or a user-defined entry.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change requires CSR request JDK-8334261 to be approved
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8309841: Jarsigner should print a warning if an entry is removed (Enhancement - P3)
JDK-8334261: Jarsigner should print a warning if an entry is removed (CSR)

Reviewers

Hai-May Chao (@haimaychao - Committer) Review applies to 8311e890
Sean Mullan (@seanjmullan - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19599/head:pull/19599
$ git checkout pull/19599

Update a local copy of the PR:
$ git checkout pull/19599
$ git pull https://git.openjdk.org/jdk.git pull/19599/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19599

View PR using the GUI difftool:
$ git pr show -t 19599

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19599.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-07T15:11:53Z

👋 Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-06-07T15:13:17Z

@wangweij 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:

8309841: Jarsigner should print a warning if an entry is removed

Reviewed-by: mullan, hchao

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 287 new commits pushed to the master branch:

dc0ce1b: 8341336: Fix -Wzero-as-null-pointer-constant warnings in PRODUCT-only code
c43202b: 8341037: Use standard layouts in DefaultFrameIconTest.java and MenuCrash.java
76283dd: 8341246: Test com/sun/tools/attach/PermissionTest.java fails access denied after JDK-8327114
0bdfe88: 8328313: Archived module graph should allow identical --module-path to be specified during dump time and run time
9fc1c68: 8339850: Restore the interrupt status in FileSystemPreferences.lockFile()
5063494: 8340785: Update description of PassFailJFrame and samples
85f0442: 8317116: Provide layouts for multiple test UI in PassFailJFrame
49501fe: 8341412: Various test failures after JDK-8334305
8838048: 8324259: Classes used by CDS at runtime should be archived
5e98007: 8307532: Implement LM_LIGHTWEIGHT for Zero
... and 277 more: https://git.openjdk.org/jdk/compare/1b17e0b133cab44029333c832bd046b338ede581...master

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 master branch, type /integrate in a new comment.

openjdk · 2024-06-07T15:13:45Z

@wangweij The following labels will be automatically applied to this pull request:

core-libs
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

wangweij · 2024-06-07T19:28:53Z

/label remove core-libs

openjdk · 2024-06-07T19:29:25Z

@wangweij
The core-libs label was successfully removed.

bridgekeeper · 2024-08-02T21:41:15Z

@wangweij This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 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!

wangweij · 2024-09-12T13:30:35Z

Ask again for review.

mlbridge · 2024-09-12T13:38:19Z

Webrevs

test/jdk/sun/security/tools/jarsigner/RemovedFiles.java

test/lib/jdk/test/lib/util/JarUtils.java

test/jdk/sun/security/tools/jarsigner/RemovedFiles.java

driverkt · 2024-09-12T15:37:35Z

You can mark my above comments as resolved.

haimaychao · 2024-09-12T15:59:48Z

CSR looks good to me. Clarifying that non-file entries in MANIFEST.MF could cause false positive is useful in the CSR.

nibjen · 2024-09-12T16:01:13Z

src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java


        {"history.with.ts", "- Signed by \"%1$s\"\n    Digest algorithm: %2$s\n    Signature algorithm: %3$s, %4$s\n  Timestamped by \"%6$s\" on %5$tc\n    Timestamp digest algorithm: %7$s\n    Timestamp signature algorithm: %8$s, %9$s"},
        {"history.without.ts", "- Signed by \"%1$s\"\n    Digest algorithm: %2$s\n    Signature algorithm: %3$s, %4$s"},
+        {"history.nonexistent.entries", "    Warning: nonexistent signed entries: "},


Inline with existing warning message, can it be "WARNING: Nonexistent signed entries: "

This warning is a part of a block on signer info and I'd like the word does not stand out too much. For example, the test shows:

- Signed by "CN=x" Digest algorithm: SHA-384 Signature algorithm: Ed25519, 255-bit key Warning: nonexistent signed entries: [a]

haimaychao · 2024-09-12T21:41:32Z

Suggest to add testcases to remove more than one entry or file ending with "*" in RemovedFiles.

haimaychao · 2024-09-12T22:14:26Z

Do you think it may be useful to have a testcase that falsely reports the missing entry?

wangweij · 2024-09-12T22:37:12Z

Suggest to add testcases to remove more than one entry or file ending with "*" in RemovedFiles.

This can be a test to the JarUtils method itself. I can add one.

Do you think it may be useful to have a testcase that falsely reports the missing entry?

I'll add one.

haimaychao · 2024-09-13T17:29:27Z

Thanks for adding more testcases for this PR. Changes look good.

seanjmullan · 2024-09-27T13:49:57Z

src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resources.java

        {"key.bit.disabled", "%d-bit key (disabled)"},
        {"key.bit.eccurve.disabled", "%1$d-bit %2$s key (disabled)"},
        {"unknown.size", "unknown size"},
+        {"nonexistent.entries.found", "Nonexistent signed entries detected. See details in -verbose output."},


For the second sentence, in other warning messages, we say "Re-run jarsigner with the -verbose option for more details." Perhaps we should be consistent?

Actually, I see the output will also contain the message "Re-run with the -verbose and -certs options for more details." so I take back my comment above.

For this summary message, I suggest we be a bit more descriptive like in other messages:

"This jar contains signed entries for files that do not exist. See the -verbose output for more details."

Updated. Thanks.

seanjmullan · 2024-09-30T13:50:23Z

test/lib-test/jdk/test/lib/util/JarUtilsTest.java

+        // Note: b* covers everything starting with b, even bx/x
+        JarUtils.deleteEntries(Path.of("a.jar"), "b*");
+        Asserts.assertEquals(Set.of("c"), content("a.jar"));
+    }


You could also add a test where the pattern doesn't match, and a test where there is more than one pattern, and the matching pattern is not the first one.

wangweij · 2024-10-02T22:54:23Z

/integrate

openjdk · 2024-10-02T22:55:50Z

Going to push as commit bdfb41f.
Since your change was applied there have been 288 commits pushed to the master branch:

57c1db5: 8332697: ubsan: shenandoahSimpleBitMap.inline.hpp:68:23: runtime error: signed integer overflow: -9223372036854775808 - 1 cannot be represented in type 'long int'
dc0ce1b: 8341336: Fix -Wzero-as-null-pointer-constant warnings in PRODUCT-only code
c43202b: 8341037: Use standard layouts in DefaultFrameIconTest.java and MenuCrash.java
76283dd: 8341246: Test com/sun/tools/attach/PermissionTest.java fails access denied after JDK-8327114
0bdfe88: 8328313: Archived module graph should allow identical --module-path to be specified during dump time and run time
9fc1c68: 8339850: Restore the interrupt status in FileSystemPreferences.lockFile()
5063494: 8340785: Update description of PassFailJFrame and samples
85f0442: 8317116: Provide layouts for multiple test UI in PassFailJFrame
49501fe: 8341412: Various test failures after JDK-8334305
8838048: 8324259: Classes used by CDS at runtime should be archived
... and 278 more: https://git.openjdk.org/jdk/compare/1b17e0b133cab44029333c832bd046b338ede581...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-02T22:55:57Z

@wangweij Pushed as commit bdfb41f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

the code change

e50f330

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jun 7, 2024

some wording

e1fc79f

openjdk bot removed the core-libs core-libs-dev@openjdk.org label Jun 7, 2024

revert the re-sign change, add one more warning

8cdb178

openjdk bot added the csr Pull request needs approved CSR before integration label Jun 13, 2024

rename

2f8841d

merge

8559b87

wangweij marked this pull request as ready for review September 12, 2024 13:30

openjdk bot added the rfr Pull request is ready for review label Sep 12, 2024

driverkt reviewed Sep 12, 2024

View reviewed changes

test/jdk/sun/security/tools/jarsigner/RemovedFiles.java Outdated Show resolved Hide resolved

driverkt reviewed Sep 12, 2024

View reviewed changes

test/lib/jdk/test/lib/util/JarUtils.java Show resolved Hide resolved

driverkt reviewed Sep 12, 2024

View reviewed changes

test/jdk/sun/security/tools/jarsigner/RemovedFiles.java Show resolved Hide resolved

test enhancements

7533fd6

nibjen reviewed Sep 12, 2024

View reviewed changes

more tests

26d93e8

remove another entry

8311e89

haimaychao approved these changes Sep 13, 2024

View reviewed changes

expected result should be the 1st argument

17fdd52

seanjmullan reviewed Sep 27, 2024

View reviewed changes

update warning message

097b99c

seanjmullan reviewed Sep 30, 2024

View reviewed changes

more testcases in JarUtilsTest.java

7ad36e1

seanjmullan approved these changes Oct 1, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Oct 1, 2024

wangweij changed the title ~~8309841: Jarsigner should print a warning if an entry is removed~~ 8337496: Improve jarsigner utility handling of missing content Oct 2, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Oct 2, 2024

wangweij added 2 commits October 2, 2024 12:01

modify bug id

85c57dc

revert previous change

0a9a8cb

wangweij changed the title ~~8337496: Improve jarsigner utility handling of missing content~~ 8309841: Jarsigner should print a warning if an entry is removed Oct 2, 2024

openjdk bot added the ready Pull request is ready to be integrated label Oct 2, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 2, 2024

openjdk bot closed this Oct 2, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 2, 2024

wangweij deleted the 8309841 branch October 4, 2024 18:02

8309841: Jarsigner should print a warning if an entry is removed #19599

8309841: Jarsigner should print a warning if an entry is removed #19599

Uh oh!

Conversation

wangweij commented Jun 7, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jun 7, 2024

Uh oh!

openjdk bot commented Jun 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 7, 2024

Uh oh!

wangweij commented Jun 7, 2024

Uh oh!

openjdk bot commented Jun 7, 2024

Uh oh!

bridgekeeper bot commented Aug 2, 2024

Uh oh!

wangweij commented Sep 12, 2024

Uh oh!

mlbridge bot commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

driverkt commented Sep 12, 2024

Uh oh!

haimaychao commented Sep 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haimaychao commented Sep 12, 2024

Uh oh!

haimaychao commented Sep 12, 2024

Uh oh!

wangweij commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haimaychao commented Sep 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij commented Oct 2, 2024

Uh oh!

openjdk bot commented Oct 2, 2024

Uh oh!

openjdk bot commented Oct 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

wangweij commented Jun 7, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 7, 2024 •

edited

Loading

mlbridge bot commented Sep 12, 2024 •

edited

Loading

wangweij commented Sep 12, 2024 •

edited

Loading