Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Fri, 16 Jun 2023 11:09:23 GMT, Sean Coffey wrote: > Logged https://bugs.openjdk.org/browse/JDK-8310201 to track the Locale > changes Alan. Thanks, probably should treat this as a bug for JDK 21 so that a new addition doesn't change behavior between 21 and 22. - PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1232123081
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 11:39:14 GMT, Sean Coffey wrote: >> New functionality in the -XshowSettings menu to display relevant information >> about JDK security configuration > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Incorporate review comments from Roger and tweak some code Incorporated all comments to date. Took Tony's point on board. The provider output format is now `type.service` the refactoring of code into new security helper class might make it harder to determine recent changes. I committed the main changes before the refactor operation. Might make review easier : https://github.com/openjdk/jdk/pull/14394/commits/1af66eb6362c15a92ce03b0ab8667d6d3a0cb673 Need to update details in CSR also. - PR Comment: https://git.openjdk.org/jdk/pull/14394#issuecomment-1594515340
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 14:06:33 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 424:
>>
>>> 422:if (!services.isEmpty()) {
>>> 423:services.stream()
>>> 424:
>>> .sorted(Comparator.comparing(Provider.Service::getType))
>>
>> How about sorting by algorithm within the same type?
>>
>> Another suggestion: adding aliases to output, for example: `Signature:
>> Ed25519 (1.3.101.112, OID.1.3.101.112)`.
>
> +1 to sorting by algorithm name within same type.
good point. done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1232107493
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Thu, 15 Jun 2023 06:32:06 GMT, Alan Bateman wrote: >> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 183: >> >>> 181: case "locale": >>> 182: printLocale(); >>> 183: break; >> >> printLocale might need the same treatment to avoid printing the list of all >> locales by default. > > Have you tried putting the new code into say sun.launcher.SecuritySettings to > avoid adding so much security conf code into LauncherHelper? Maybe eventually > this will need to a more pluggable as -XshowSettings has grown a bit beyond > what it was originally for. Logged https://bugs.openjdk.org/browse/JDK-8310201 to track the Locale changes Alan. Good suggestion on factoring out the security code to a helper class. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1232109645
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 12:46:36 GMT, Weijun Wang wrote:
>> Sean Coffey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Incorporate review comments from Roger and tweak some code
>
> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 357:
>
>> 355: private static void printSecuritySummarySettings() {
>> 356: ostream.println("Security settings summary: " + "\n" +
>> 357: INDENT + "Use \"-XshowSettings:security\" for verbose
>> details\n");
>
> Is it helpful to list sub-options here?
I think it might be better to delegate to `java -X` help output actually.
tweaked code in this area. (I could see the subcommand options becoming
inaccurate if we add more such subcommands in the future). -X help output
should be our reference
> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 378:
>
>> 376: ostream.println(TWOINDENT + key + "=");
>> 377: List.of(val.split(",")).forEach(
>> 378: s -> ostream.println(THREEINDENT + s.trim() +
>> ","));
>
> Will this print a comma for the last line?
yes, I was hoping it wouldn't be an issue! modified code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1232108994
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1232107328
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 11:39:14 GMT, Sean Coffey wrote: >> New functionality in the -XshowSettings menu to display relevant information >> about JDK security configuration > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Incorporate review comments from Roger and tweak some code The output from -XshowSettings vs. the large dump from -XshowSettings:security look good, I think the default vs. all split is quite good. The format/style of the output is a bit different to the other sections in the -XshowSettings output. The other sections are more like name = value. What you have is okay, it just looks a bit different. There are a few blank lines in the the "Security settings summary" output that you might want to cull. Also "Use -XshowSettings:security for verbose details" might be better as "... for security details". Also what would you think about the earlier suggesting to do the same for the locale output? The dump of all available locales seems excessive to print out by default. - PR Comment: https://git.openjdk.org/jdk/pull/14394#issuecomment-1592507948
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Thu, 15 Jun 2023 06:29:09 GMT, Alan Bateman wrote: >> Sean Coffey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Incorporate review comments from Roger and tweak some code > > src/java.base/share/classes/sun/launcher/LauncherHelper.java line 183: > >> 181: case "locale": >> 182: printLocale(); >> 183: break; > > printLocale might need the same treatment to avoid printing the list of all > locales by default. Have you tried putting the new code into say sun.launcher.SecuritySettings to avoid adding so much security conf code into LauncherHelper? Maybe eventually this will need to a more pluggable as -XshowSettings has grown a bit beyond what it was originally for. - PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1230499915
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 11:39:14 GMT, Sean Coffey wrote: >> New functionality in the -XshowSettings menu to display relevant information >> about JDK security configuration > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Incorporate review comments from Roger and tweak some code src/java.base/share/classes/sun/launcher/LauncherHelper.java line 183: > 181: case "locale": > 182: printLocale(); > 183: break; printLocale might need the same treatment to avoid printing the list of all locales by default. - PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1230497367
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 11:39:14 GMT, Sean Coffey wrote: >> New functionality in the -XshowSettings menu to display relevant information >> about JDK security configuration > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Incorporate review comments from Roger and tweak some code I would recommend using the service syntax that is similar to Preferred Provider and is also similar to a [services control](https://mail.openjdk.org/pipermail/security-dev/2023-May/035819.html) on the security mailing list that is not yet approved. `Signature.SHA1withRSA` instead of `Signature : SHA1withRSA` - PR Comment: https://git.openjdk.org/jdk/pull/14394#issuecomment-1592008732
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 11:52:35 GMT, Weijun Wang wrote:
>> Sean Coffey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Incorporate review comments from Roger and tweak some code
>
> src/java.base/share/classes/sun/launcher/LauncherHelper.java line 424:
>
>> 422:if (!services.isEmpty()) {
>> 423:services.stream()
>> 424:
>> .sorted(Comparator.comparing(Provider.Service::getType))
>
> How about sorting by algorithm within the same type?
>
> Another suggestion: adding aliases to output, for example: `Signature:
> Ed25519 (1.3.101.112, OID.1.3.101.112)`.
+1 to sorting by algorithm name within same type.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1229680104
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 11:39:14 GMT, Sean Coffey wrote:
>> New functionality in the -XshowSettings menu to display relevant information
>> about JDK security configuration
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Incorporate review comments from Roger and tweak some code
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 357:
> 355: private static void printSecuritySummarySettings() {
> 356: ostream.println("Security settings summary: " + "\n" +
> 357: INDENT + "Use \"-XshowSettings:security\" for verbose
> details\n");
Is it helpful to list sub-options here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1229560168
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 11:39:14 GMT, Sean Coffey wrote:
>> New functionality in the -XshowSettings menu to display relevant information
>> about JDK security configuration
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Incorporate review comments from Roger and tweak some code
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 424:
> 422:if (!services.isEmpty()) {
> 423:services.stream()
> 424:
> .sorted(Comparator.comparing(Provider.Service::getType))
How about sorting by algorithm within the same type?
Another suggestion: adding aliases to output, for example: `Signature: Ed25519
(1.3.101.112, OID.1.3.101.112)`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1229473561
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
On Wed, 14 Jun 2023 11:39:14 GMT, Sean Coffey wrote:
>> New functionality in the -XshowSettings menu to display relevant information
>> about JDK security configuration
>
> Sean Coffey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Incorporate review comments from Roger and tweak some code
src/java.base/share/classes/sun/launcher/LauncherHelper.java line 378:
> 376: ostream.println(TWOINDENT + key + "=");
> 377: List.of(val.split(",")).forEach(
> 378: s -> ostream.println(THREEINDENT + s.trim() +
> ","));
Will this print a comma for the last line?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1229466419
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v3]
> New functionality in the -XshowSettings menu to display relevant information > about JDK security configuration Sean Coffey has updated the pull request incrementally with one additional commit since the last revision: Incorporate review comments from Roger and tweak some code - Changes: - all: https://git.openjdk.org/jdk/pull/14394/files - new: https://git.openjdk.org/jdk/pull/14394/files/c60a4ed4..55a85e38 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14394=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14394=01-02 Stats: 14 lines in 1 file changed: 0 ins; 3 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/14394.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14394/head:pull/14394 PR: https://git.openjdk.org/jdk/pull/14394
