Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
On Fri, 7 Jul 2023 13:40:53 GMT, Sean Coffey wrote: >> src/java.base/share/classes/sun/launcher/SecuritySettings.java line 123: >> >>> 121: } >>> 122: >>> 123: ostream.println(INDENT + "Security TLS configuration:"); >> >> What about also noting the name of the TLS/JSSE provider in this line, for >> example: >> >> "Security TLS configuration (SunJSSE provider):" >> >> This would be useful information if a customer is using a 3rd party JSSE >> provider, and it is selected before SunJSSE, as the defaults in that case >> may be different. > > good suggestion Sean. Patch updated. I think we need to harden this area of code actually. Some JDK configurations may not have an SSL provider. e.g. (removing JSSE from config) See "java -X" for verbose security settings options Security provider static configuration: (in order of preference) Provider name: SUN Provider name: SunRsaSign Exception in thread "main" java.lang.InternalError: Failed to create SSL socket at java.base/sun.launcher.SecuritySettings.printSecurityTLSConfig(SecuritySettings.java:121) at java.base/sun.launcher.SecuritySettings.printSecuritySummarySettings(SecuritySettings.java:75) at java.base/sun.launcher.LauncherHelper.showSettings(LauncherHelper.java:188) - PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1255945485
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
On Thu, 6 Jul 2023 14:03:12 GMT, Sean Mullan wrote: >> Sean Coffey has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 15 additional >> commits since the last revision: >> >> - Avoid sharing INDENT variables >> - Merge branch 'master' into 8281658-showsettings-security >> - Don't allow bad subcommand values for security component >> - restore more informative help message >> - Split long properties for ; also >> - Pass PrintStream to security helper >> - Refactor out security code to helper class >> - Print aliases. Order Provider type/service output. >> - Incorporate review comments from Roger and tweak some code >> - Merge branch 'master' into 8281658-showsettings-security >> - ... and 5 more: https://git.openjdk.org/jdk/compare/774eb316...7e6f5090 > > src/java.base/share/classes/sun/launcher/SecuritySettings.java line 123: > >> 121: } >> 122: >> 123: ostream.println(INDENT + "Security TLS configuration:"); > > What about also noting the name of the TLS/JSSE provider in this line, for > example: > > "Security TLS configuration (SunJSSE provider):" > > This would be useful information if a customer is using a 3rd party JSSE > provider, and it is selected before SunJSSE, as the defaults in that case may > be different. good suggestion Sean. Patch updated. - PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1255842787
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
On Thu, 6 Jul 2023 18:43:41 GMT, Roger Riggs wrote: >> Sean Coffey has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 15 additional >> commits since the last revision: >> >> - Avoid sharing INDENT variables >> - Merge branch 'master' into 8281658-showsettings-security >> - Don't allow bad subcommand values for security component >> - restore more informative help message >> - Split long properties for ; also >> - Pass PrintStream to security helper >> - Refactor out security code to helper class >> - Print aliases. Order Provider type/service output. >> - Incorporate review comments from Roger and tweak some code >> - Merge branch 'master' into 8281658-showsettings-security >> - ... and 5 more: https://git.openjdk.org/jdk/compare/774eb316...7e6f5090 > > src/java.base/share/classes/sun/launcher/LauncherHelper.java line 115: > >> 113: private static StringBuilder outBuf = new StringBuilder(); >> 114: >> 115: private static final String INDENT = " ".repeat(4); > > Revert this, its just making a simple constant into a runtime expression. Thanks Roger. Fair point. Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1255844148
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
On Thu, 6 Jul 2023 17:15:20 GMT, Mandy Chung wrote: > > The changes make me wonder if `-XshowSetting:aardvark` should be an error > > rather than default to print all settings. Something we should look at > > again. Same thing for `-XshowSettings:system` on non-Linux, probably should > > have been a bit more discussion when that was added as its more about the > > container environment. > > I think throwing an error for an invalid suboption makes sense. > `-XshowSetting` without the suboption is default to print all settings, i.e. > equivalent to `-XshowSetting:all`. But the switch statment is missing the > `all` case. I think we should fix this and error for any invalid option. fair point Mandy - I hope to change such behavior in a new follow on patch that I'll deliver shortly: JDK-8311653 - PR Comment: https://git.openjdk.org/jdk/pull/14394#issuecomment-1625434995
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
On Tue, 27 Jun 2023 15:06:45 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 15 additional > commits since the last revision: > > - Avoid sharing INDENT variables > - Merge branch 'master' into 8281658-showsettings-security > - Don't allow bad subcommand values for security component > - restore more informative help message > - Split long properties for ; also > - Pass PrintStream to security helper > - Refactor out security code to helper class > - Print aliases. Order Provider type/service output. > - Incorporate review comments from Roger and tweak some code > - Merge branch 'master' into 8281658-showsettings-security > - ... and 5 more: https://git.openjdk.org/jdk/compare/da750e20...7e6f5090 Marked as reviewed by rriggs (Reviewer). src/java.base/share/classes/sun/launcher/LauncherHelper.java line 115: > 113: private static StringBuilder outBuf = new StringBuilder(); > 114: > 115: private static final String INDENT = " ".repeat(4); Revert this, its just making a simple constant into a runtime expression. - PR Review: https://git.openjdk.org/jdk/pull/14394#pullrequestreview-1517119059 PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1254799500
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
On Thu, 29 Jun 2023 08:27:43 GMT, Alan Bateman wrote: > The changes make me wonder if `-XshowSetting:aardvark` should be an error > rather than default to print all settings. Something we should look at again. > Same thing for `-XshowSettings:system` on non-Linux, probably should have > been a bit more discussion when that was added as its more about the > container environment. I think throwing an error for an invalid suboption makes sense. `-XshowSetting` without the suboption is default to print all settings, i.e. equivalent to `-XshowSetting:all`. But the switch statment is missing the `all` case. I think we should fix this and error for any invalid option. - PR Comment: https://git.openjdk.org/jdk/pull/14394#issuecomment-1624030798
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
On Tue, 27 Jun 2023 15:06:45 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 15 additional > commits since the last revision: > > - Avoid sharing INDENT variables > - Merge branch 'master' into 8281658-showsettings-security > - Don't allow bad subcommand values for security component > - restore more informative help message > - Split long properties for ; also > - Pass PrintStream to security helper > - Refactor out security code to helper class > - Print aliases. Order Provider type/service output. > - Incorporate review comments from Roger and tweak some code > - Merge branch 'master' into 8281658-showsettings-security > - ... and 5 more: https://git.openjdk.org/jdk/compare/5cbc0d11...7e6f5090 src/java.base/share/classes/sun/launcher/SecuritySettings.java line 123: > 121: } > 122: > 123: ostream.println(INDENT + "Security TLS configuration:"); What about also noting the name of the TLS/JSSE provider in this line, for example: "Security TLS configuration (SunJSSE provider):" This would be useful information if a customer is using a 3rd party JSSE provider, and it is selected before SunJSSE, as the defaults in that case may be different. - PR Review Comment: https://git.openjdk.org/jdk/pull/14394#discussion_r1254491773
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
On Tue, 27 Jun 2023 15:06:45 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 15 additional > commits since the last revision: > > - Avoid sharing INDENT variables > - Merge branch 'master' into 8281658-showsettings-security > - Don't allow bad subcommand values for security component > - restore more informative help message > - Split long properties for ; also > - Pass PrintStream to security helper > - Refactor out security code to helper class > - Print aliases. Order Provider type/service output. > - Incorporate review comments from Roger and tweak some code > - Merge branch 'master' into 8281658-showsettings-security > - ... and 5 more: https://git.openjdk.org/jdk/compare/6dc03296...7e6f5090 I looked through the current version (7e6f5090) and you've got it to a good place, the launcher changes look fine. The changes make me wonder if `-XshowSetting:aardvark` should be an error rather than default to print all settings. Something we should look at again. Same thing for `-XshowSettings:system` on non-Linux, probably should have been a bit more discussion when that was added as its more about the container environment. - PR Comment: https://git.openjdk.org/jdk/pull/14394#issuecomment-1612623972
Re: RFR: 8281658: Add a security category to the java -XshowSettings option [v8]
> New functionality in the -XshowSettings menu to display relevant information > about JDK security configuration Sean Coffey has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 15 additional commits since the last revision: - Avoid sharing INDENT variables - Merge branch 'master' into 8281658-showsettings-security - Don't allow bad subcommand values for security component - restore more informative help message - Split long properties for ; also - Pass PrintStream to security helper - Refactor out security code to helper class - Print aliases. Order Provider type/service output. - Incorporate review comments from Roger and tweak some code - Merge branch 'master' into 8281658-showsettings-security - ... and 5 more: https://git.openjdk.org/jdk/compare/698e2b5c...7e6f5090 - Changes: - all: https://git.openjdk.org/jdk/pull/14394/files - new: https://git.openjdk.org/jdk/pull/14394/files/77c9f5e2..7e6f5090 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14394=07 - incr: https://webrevs.openjdk.org/?repo=jdk=14394=06-07 Stats: 31805 lines in 1150 files changed: 12787 ins; 12829 del; 6189 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
