8272163: Add -version option to keytool and jarsigner #5954

haimaychao · 2021-10-14T16:04:08Z

It'd be useful to have a -version option for keytool and jarsigner. Many other JDK tools already have a -version option. This is to add -version option to keytool and jarsigner like jar command does.

CSR review:
https://bugs.openjdk.java.net/browse/JDK-8275174

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8272163: Add -version option to keytool and jarsigner

Reviewers

Weijun Wang (@wangweij - Reviewer) ⚠️ Review applies to e801c6a

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5954/head:pull/5954
$ git checkout pull/5954

Update a local copy of the PR:
$ git checkout pull/5954
$ git pull https://git.openjdk.java.net/jdk pull/5954/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5954

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5954.diff

bridgekeeper · 2021-10-14T16:04:39Z

👋 Welcome back hchao! 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 · 2021-10-14T16:08:04Z

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

compiler
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.

mlbridge · 2021-10-14T16:13:09Z

Webrevs

AlanBateman · 2021-10-14T16:25:58Z

/remove label compiler

AlanBateman · 2021-10-14T16:26:07Z

/remove label core-libs

openjdk · 2021-10-14T16:26:42Z

@AlanBateman Unknown command remove - for a list of valid commands use /help.

openjdk · 2021-10-14T16:26:53Z

@AlanBateman Unknown command remove - for a list of valid commands use /help.

AlanBateman · 2021-10-14T16:31:22Z

/label remove core-libs

openjdk · 2021-10-14T16:31:49Z

@AlanBateman
The core-libs label was successfully removed.

haimaychao · 2021-10-14T18:47:25Z

/label remove compiler

openjdk · 2021-10-14T18:48:21Z

@haimaychao
The compiler label was successfully removed.

seanjmullan · 2021-10-15T13:21:18Z

/csr

seanjmullan · 2021-10-15T13:21:38Z

This will also need a CSR so I added the csr label.

openjdk · 2021-10-15T13:22:24Z

@seanjmullan this pull request will not be integrated until the CSR request JDK-8275174 for issue JDK-8272163 has been approved.

wangweij · 2021-10-15T13:34:48Z

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

+                usageTooManyArg();
+            }
+            doPrintVersion();
+        }


What happens if -version appears elsewhere?

I tried some other commands. When both -version and other arguments are provided, java ignores the others and prints out version info. javac prints out the version info and compiles as well.

It's possible that user writes a script or alias that always run the tool with some options (Ex: -keystore, -storepass, -verbose). Maybe we should not fail if -version is used with other options (no matter at the beginning or the end).

Fixed so -version can be used with other options in jarsigner.

wangweij · 2021-10-15T13:35:32Z

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

@@ -591,17 +597,30 @@ static void usageNoArg() {
        usage();
    }

+    static void usageTooManyArg() {


Since the method is only for -version and the printed message also mentions "version", maybe it's better to choose a specific name.

Removed this method after the change for the comment as above.

wangweij · 2021-10-15T13:36:25Z

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

@@ -591,17 +597,30 @@ static void usageNoArg() {
        usage();
    }

+    static void usageTooManyArg() {
+        System.out.println(rb.getString(
+                "Option.should.not.be.specified.for.version"));


Neither arguments not options should be specified here.

Method was removed.

wangweij · 2021-10-15T13:38:09Z

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

+                .shouldContain("jarsigner")
+                .shouldHaveExitValue(0);
+
+        SecurityTools.jarsigner("-version -extraopt")


You might want to use some legal arguments here, for example, -version -help. Otherwise, the error might be because the extra option is not supported. Or, you can check if an expected error message is printed out.

Updated the test as suggested.

wangweij · 2021-10-15T13:38:30Z

test/jdk/sun/security/tools/keytool/VersionTest.java

+                .shouldContain("keytool")
+                .shouldHaveExitValue(0);
+
+        SecurityTools.keytool("-version -extraopt")


Same comment as above.

wangweij · 2021-10-15T13:49:27Z

For the CSR, is it necessary to list the -help output change?

haimaychao · 2021-10-19T01:13:14Z

@wangweij Thanks for your review! Regarding your CSR comment if it is necessary to list the -help output change, as the current keytool manage lists its commands [1], and then I’ve added an entry for -version at the end of the list in the CSR. The jarsigner manpage lists the options [2], and I also added an entry for -version to its list. Are you suggesting we should add a new output for -help in keytool and jarsigner manpages?
[1] https://docs.oracle.com/en/java/javase/17/docs/specs/man/keytool.html#synopsis
[2] https://docs.oracle.com/en/java/javase/17/docs/specs/man/jarsigner.html#synopsis

wangweij · 2021-10-19T03:40:55Z

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

@@ -308,6 +309,10 @@ public void run(String args[]) {
            Event.clearReportListener(Event.ReporterCategory.CRLCHECK);
        }

+        if (version) {
+            doPrintVersion();
+        }


So if both -version and file.jar are provided, jarsigner will both sign the jar and print the version. This is probably OK but I prefer the java style that only prints the version.

Ok, change made to have jarsigner to only print the version and ignore other options.

wangweij · 2021-10-19T13:10:15Z

Approved the PR. Thanks. I have a small comment on the CSR.

haimaychao · 2021-10-19T16:27:49Z

@wangweij Thanks for reviewing the CSR and code changes.

haimaychao · 2021-10-19T16:46:31Z

@wangweij Updated the CSR with your comment. Thanks!

openjdk · 2021-10-22T17:04:31Z

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

8272163: Add -version option to keytool and jarsigner

Reviewed-by: weijun

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

88bbf3c: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
4dec8fc: 8275645: [JVMCI] avoid unaligned volatile reads on AArch64
4e647aa: 8275416: G1: remove unnecessary make_referent_alive in precleaning phase
dd622e5: 8275783: G1: fix incorrect region type documentation in HeapRegionType
b2128a9: 8263155: Allow additional contents for DMG
1efe946: 8275712: Hashtable literal_size functions are broken
fab3d6c: 8275761: Backout: JDK-8274794 Print all owned locks in hs_err file
c978ca8: 8275344: -Xcheck:jni produces some warnings in the LCMS.c
6a466fe: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields
4e9dd4b: 8275384: Change nested classes in jdk.jconsole to static nested classes
... and 136 more: https://git.openjdk.java.net/jdk/compare/124f82377ba93359bc59118ee315ba194080fa92...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.

haimaychao · 2021-10-22T20:52:43Z

/integrate

openjdk · 2021-10-22T20:53:51Z

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

6523c55: 8198336: java/awt/FontMetrics/FontCrash.java fails in headless mode
88bbf3c: 8273111: Default timezone should return zone ID if /etc/localtime is valid but not canonicalization on linux
4dec8fc: 8275645: [JVMCI] avoid unaligned volatile reads on AArch64
4e647aa: 8275416: G1: remove unnecessary make_referent_alive in precleaning phase
dd622e5: 8275783: G1: fix incorrect region type documentation in HeapRegionType
b2128a9: 8263155: Allow additional contents for DMG
1efe946: 8275712: Hashtable literal_size functions are broken
fab3d6c: 8275761: Backout: JDK-8274794 Print all owned locks in hs_err file
c978ca8: 8275344: -Xcheck:jni produces some warnings in the LCMS.c
6a466fe: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields
... and 137 more: https://git.openjdk.java.net/jdk/compare/124f82377ba93359bc59118ee315ba194080fa92...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-22T20:54:05Z

@haimaychao Pushed as commit fec470f.

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

8272163: Add -version option to keytool and jarsigner

1d43fe2

openjdk bot added the rfr Pull request is ready for review label Oct 14, 2021

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Oct 14, 2021

openjdk bot removed the core-libs core-libs-dev@openjdk.org label Oct 14, 2021

openjdk bot removed the compiler compiler-dev@openjdk.org label Oct 14, 2021

openjdk bot added the csr Pull request needs approved CSR before integration label Oct 15, 2021

wangweij reviewed Oct 15, 2021

View reviewed changes

Fix -version in jarsigner and update tests

4593b7a

wangweij reviewed Oct 19, 2021

View reviewed changes

haimaychao added 2 commits October 18, 2021 23:10

jarsigner only prints version and ignores other options

c0a7160

Updated comment in test

e801c6a

wangweij approved these changes Oct 19, 2021

View reviewed changes

fullusage test change

e6b360e

openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 22, 2021

openjdk bot added the ready Pull request is ready to be integrated label Oct 22, 2021

openjdk bot closed this Oct 22, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 22, 2021

8272163: Add -version option to keytool and jarsigner #5954

8272163: Add -version option to keytool and jarsigner #5954

Conversation

haimaychao commented Oct 14, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

mlbridge bot commented Oct 14, 2021 • edited Loading

Webrevs

AlanBateman commented Oct 14, 2021

AlanBateman commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

AlanBateman commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

haimaychao commented Oct 14, 2021

openjdk bot commented Oct 14, 2021

seanjmullan commented Oct 15, 2021

seanjmullan commented Oct 15, 2021

openjdk bot commented Oct 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij commented Oct 15, 2021

haimaychao commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij commented Oct 19, 2021

haimaychao commented Oct 19, 2021

haimaychao commented Oct 19, 2021

openjdk bot commented Oct 22, 2021 • edited Loading

haimaychao commented Oct 22, 2021

openjdk bot commented Oct 22, 2021

openjdk bot commented Oct 22, 2021

haimaychao commented Oct 14, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 14, 2021 •

edited

Loading

openjdk bot commented Oct 22, 2021 •

edited

Loading