Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Tue, 31 Aug 2021 02:05:06 GMT, Weijun Wang wrote: >> This change modifies the default value of the `java.security.manager` system >> property from "allow" to "disallow". This means unless it's explicitly set >> to "allow", any call to `System.setSecurityManager()` would throw an UOE. >> >> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are >> updated to confirm this behavior change. Two other tests are updated because >> they were added after JDK-8267184 and do not have >> `-Djava.security.manager=allow` on its `@run` line even it they need to >> install a `SecurityManager` at runtime. >> >> Please note that this code change requires jtreg to be upgraded to 6.1, >> where a security manager [will not be >> set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). > > New commit pushed. Sections added. > @wangweij This pull request has been inactive for more than 4 weeks and will > be automatically closed if another 4 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! Still waiting for jtreg upgraded to 6.1. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
On Tue, 31 Aug 2021 02:08:48 GMT, Weijun Wang wrote: >> This change modifies the default value of the `java.security.manager` system >> property from "allow" to "disallow". This means unless it's explicitly set >> to "allow", any call to `System.setSecurityManager()` would throw an UOE. >> >> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are >> updated to confirm this behavior change. Two other tests are updated because >> they were added after JDK-8267184 and do not have >> `-Djava.security.manager=allow` on its `@run` line even it they need to >> install a `SecurityManager` at runtime. >> >> Please note that this code change requires jtreg to be upgraded to 6.1, >> where a security manager [will not be >> set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > sections etc Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
On Tue, 31 Aug 2021 02:08:48 GMT, Weijun Wang wrote: >> This change modifies the default value of the `java.security.manager` system >> property from "allow" to "disallow". This means unless it's explicitly set >> to "allow", any call to `System.setSecurityManager()` would throw an UOE. >> >> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are >> updated to confirm this behavior change. Two other tests are updated because >> they were added after JDK-8267184 and do not have >> `-Djava.security.manager=allow` on its `@run` line even it they need to >> install a `SecurityManager` at runtime. >> >> Please note that this code change requires jtreg to be upgraded to 6.1, >> where a security manager [will not be >> set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > sections etc Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
On Tue, 31 Aug 2021 02:08:48 GMT, Weijun Wang wrote: >> This change modifies the default value of the `java.security.manager` system >> property from "allow" to "disallow". This means unless it's explicitly set >> to "allow", any call to `System.setSecurityManager()` would throw an UOE. >> >> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are >> updated to confirm this behavior change. Two other tests are updated because >> they were added after JDK-8267184 and do not have >> `-Djava.security.manager=allow` on its `@run` line even it they need to >> install a `SecurityManager` at runtime. >> >> Please note that this code change requires jtreg to be upgraded to 6.1, >> where a security manager [will not be >> set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > sections etc Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow [v2]
> This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: sections etc - Changes: - all: https://git.openjdk.java.net/jdk/pull/5204/files - new: https://git.openjdk.java.net/jdk/pull/5204/files/63b1b7c9..08635b91 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5204=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5204=00-01 Stats: 7 lines in 2 files changed: 2 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5204.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5204/head:pull/5204 PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). New commit pushed. Sections added. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). In the class description of `java/lang/SecurityManager` I think it would be useful to add a couple of sub-sections, 1. **Setting a Security Manager** just before the paragraph that starts with "Environments using a security manager will typically set the security manager at startup." and ends with "The current security manager is returned by the getSecurityManager method." 2. **Checking permissions** which starts after the section above and continues to the end. The reason I think this is useful is that you can then add a link from `System.setSecurityManager` to the subsection on **Setting a Security Manager** as I think it will be useful to link those together. The best place for that link is probably in the `@implNote` where it describes the JDK behavior for the `java.security.manager` system property. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Mon, 23 Aug 2021 12:09:42 GMT, Weijun Wang wrote: > > Would this then allow the security manager to be used? In other words, can > > the value of `java.security.manager` be changed dynamically at runtime or > > is it restricted to be set only at launch time (via command line arugment > > `-Djava.security.manager`)? > > Whether to allow a SecurityManager to be installed later is determined at > system initialization time, so there is no use to set it to "allow" inside a > program. See > > https://github.com/openjdk/jdk/blob/3a690240336bda8582a15ca52f4dcb78be323dcd/src/java.base/share/classes/java/lang/System.java#L2191 > > The spec in `SecurityManager.java` uses the words "if the Java virtual > machine **is started with** the java.security.manager system property..." to > describe this. Thank you for that clarification @wangweij. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Mon, 23 Aug 2021 03:22:18 GMT, Jaikiran Pai wrote: > Would this then allow the security manager to be used? In other words, can > the value of `java.security.manager` be changed dynamically at runtime or is > it restricted to be set only at launch time (via command line arugment > `-Djava.security.manager`)? Whether to allow a SecurityManager to be installed later is determined at system initialization time, so there is no use to set it to "allow" inside a program. See https://github.com/openjdk/jdk/blob/3a690240336bda8582a15ca52f4dcb78be323dcd/src/java.base/share/classes/java/lang/System.java#L2191 The spec in `SecurityManager.java` uses the words "if the Java virtual machine **is started with** the java.security.manager system property..." to describe this. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. > > Please note that this code change requires jtreg to be upgraded to 6.1, where > a security manager [will not be > set](https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). This issue (https://bugs.openjdk.java.net/browse/JDK-8270380) is blocked by jtreg 6.1 (https://bugs.openjdk.java.net/browse/CODETOOLS-7902990). Precisely, it should be blocked by a JDK bug that updates the jtreg required version to 6.1. I should have mentioned this in the PR. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. Thanks for the clarification @AlanBateman . @wangweij my apologies as I misunderstood the affect this change was going to have on the existing warnings - which is none. David - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On 23/08/2021 05:45, David Holmes wrote: : @wangweij there are many tests that can call setSecurityManager() and will presumably need to be fixed before this switch can be applied. And all testing will need to be updated to require jtreg 6.1 (which no longer uses the SM) once it is released. Most of the preparation work for the tests was done via JDK-8267184 in JDK 17. JDK-8270380 isn't going to be integrated until after the upgrade the jtreg 6.1 (which I think is one part of Jai's mail). -Alan.
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. @wangweij there are many tests that can call setSecurityManager() and will presumably need to be fixed before this switch can be applied. And all testing will need to be updated to require jtreg 6.1 (which no longer uses the SM) once it is released. Thanks, David - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote: > This change modifies the default value of the `java.security.manager` system > property from "allow" to "disallow". This means unless it's explicitly set to > "allow", any call to `System.setSecurityManager()` would throw an UOE. > > The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are > updated to confirm this behavior change. Two other tests are updated because > they were added after JDK-8267184 and do not have > `-Djava.security.manager=allow` on its `@run` line even it they need to > install a `SecurityManager` at runtime. It looks to me that the failures reported in the GitHub jobs are genuine and related to this change. It looks like the `jtreg` framework itself is impacted by this change because it calls the `System.setSecurityManager()` while launching the tests. 2021-08-21T01:41:42.5731927Z stderr: 2021-08-21T01:41:42.5733295Z java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release 2021-08-21T01:41:42.5734843Zat java.base/java.lang.System.setSecurityManager(System.java:409) 2021-08-21T01:41:42.5736946Zat com.sun.javatest.regtest.agent.RegressionSecurityManager.install(RegressionSecurityManager.java:56) 2021-08-21T01:41:42.5739224Zat com.sun.javatest.regtest.agent.AgentServer.(AgentServer.java:211) 2021-08-21T01:41:42.5740879Zat com.sun.javatest.regtest.agent.AgentServer.main(AgentServer.java:70) 2021-08-21T01:41:42.5741805Z 2021-08-21T01:41:42.5743348Z TEST RESULT: Error. Agent communication error: java.net.SocketException: Connection reset; check console log for any additional details 2021-08-21T01:41:42.5745030Z -- 2021-08-21T01:41:51.2539413Z Test results: passed: 5; error: 879 2021-08-21T01:41:59.0887042Z Report written to /home/runner/work/jdk/jdk/build/run-test-prebuilt/test-results/jtreg_test_jdk_tier1_part1/html/report.html 2021-08-21T01:41:59.0892404Z Results written to /home/runner/work/jdk/jdk/build/run-test-prebuilt/test-support/jtreg_test_jdk_tier1_part1 2021-08-21T01:41:59.0895498Z Error: Some tests failed or other problems occurred. 2021-08-21T01:41:59.1235748Z Finished running test 'jtreg:test/jdk:tier1_part1' 2021-08-21T01:41:59.1237631Z Test report is stored in build/run-test-prebuilt/test-results/jtreg_test_jdk_tier1_part1 2021-08-21T01:41:59.1650100Z 2021-08-21T01:41:59.1667939Z == 2021-08-21T01:41:59.1668727Z Test summary 2021-08-21T01:41:59.1669635Z == 2021-08-21T01:41:59.1670187ZTEST TOTAL PASS FAIL ERROR 2021-08-21T01:41:59.1670795Z >> jtreg:test/jdk:tier1_part1 884 5 0 879 << - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote:
> This change modifies the default value of the `java.security.manager` system
> property from "allow" to "disallow". This means unless it's explicitly set to
> "allow", any call to `System.setSecurityManager()` would throw an UOE.
>
> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are
> updated to confirm this behavior change. Two other tests are updated because
> they were added after JDK-8267184 and do not have
> `-Djava.security.manager=allow` on its `@run` line even it they need to
> install a `SecurityManager` at runtime.
A somewhat broader question - I looked at the javadocs of this latest update to
`SecurityManager` in this PR. One thing I'm unclear about is, consider the case
where the `java.security.manager` is _not_ set to anything at the command line.
Then in some application code, let's say we have this:
String oldVal = System.getProperty("java.security.manager");
try {
System.setProperty("java.security.manager", "allow");
System.setSecurityManager(someSecurityManager);
// do something
} finally {
System.setProperty("java.security.manager", oldVal);
}
Would this then allow the security manager to be used? In other words, can the
value of `java.security.manager` be changed dynamically at runtime or is it
restricted to be set only at launch time (via command line arugment
`-Djava.security.manager`)?
-
PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Sat, 21 Aug 2021 16:59:39 GMT, Alan Bateman wrote: >> This is the same as the existing words for "disallow". > > I think I agree with Lance that "Throws UOE" would be clearer in this table. Suggestion accepted. - PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Sat, 21 Aug 2021 11:23:54 GMT, Weijun Wang wrote:
>> src/java.base/share/classes/java/lang/SecurityManager.java line 128:
>>
>>> 126: * null
>>> 127: * None
>>> 128: * Always throws {@code UnsupportedOperationException}
>>
>> Not sure "Always" is needed, could just be "Throws UOE"
>
> This is the same as the existing words for "disallow".
I think I agree with Lance that "Throws UOE" would be clearer in this table.
-
PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 23:01:27 GMT, Lance Andersen wrote:
>> This change modifies the default value of the `java.security.manager` system
>> property from "allow" to "disallow". This means unless it's explicitly set
>> to "allow", any call to `System.setSecurityManager()` would throw an UOE.
>>
>> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are
>> updated to confirm this behavior change. Two other tests are updated because
>> they were added after JDK-8267184 and do not have
>> `-Djava.security.manager=allow` on its `@run` line even it they need to
>> install one at runtime.
>
> src/java.base/share/classes/java/lang/SecurityManager.java line 128:
>
>> 126: * null
>> 127: * None
>> 128: * Always throws {@code UnsupportedOperationException}
>
> Not sure "Always" is needed, could just be "Throws UOE"
This is the same as the existing words for "disallow".
-
PR: https://git.openjdk.java.net/jdk/pull/5204
Re: RFR: 8270380: Change the default value of the java.security.manager system property to disallow
On Fri, 20 Aug 2021 22:44:34 GMT, Weijun Wang wrote:
> This change modifies the default value of the `java.security.manager` system
> property from "allow" to "disallow". This means unless it's explicitly set to
> "allow", any call to `System.setSecurityManager()` would throw an UOE.
>
> The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are
> updated to confirm this behavior change. Two other tests are updated because
> they were added after JDK-8267184 and do not have
> `-Djava.security.manager=allow` on its `@run` line even it they need to
> install one at runtime.
Looks good Max.
One minor suggestion below
src/java.base/share/classes/java/lang/SecurityManager.java line 128:
> 126: * null
> 127: * None
> 128: * Always throws {@code UnsupportedOperationException}
Not sure "Always" is needed, could just be "Throws UOE"
-
Marked as reviewed by lancea (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5204
RFR: 8270380: Change the default value of the java.security.manager system property to disallow
This change modifies the default value of the `java.security.manager` system property from "allow" to "disallow". This means unless it's explicitly set to "allow", any call to `System.setSecurityManager()` would throw an UOE. The `AllowSecurityManager.java` and `SecurityManagerWarnings.java` tests are updated to confirm this behavior change. Two other tests are updated because they were added after JDK-8267184 and do not have `-Djava.security.manager=allow` on its `@run` line even it they need to install one at runtime. - Commit messages: - 8270380: Change the default value of the java.security.manager system property to disallow Changes: https://git.openjdk.java.net/jdk/pull/5204/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5204=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270380 Stats: 24 lines in 6 files changed: 3 ins; 8 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/5204.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5204/head:pull/5204 PR: https://git.openjdk.java.net/jdk/pull/5204
