RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager [v3]

Fri Oct 25 21:04:41 UTC 2024

On Thu, 24 Oct 2024 13:19:55 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> This is the implementation of JEP 486: Permanently Disable the Security Manager. See [JEP 486](https://openjdk.org/jeps/486) for more details. The [CSR](https://bugs.openjdk.org/browse/JDK-8338412) describes in detail the main changes in the JEP and also includes an apidiff of the specification changes.
>> 
>> NOTE: the majority (~95%) of the changes in this PR are test updates (removal/modifications) and API specification changes, the latter mostly to remove `@throws SecurityException`. The remaining changes are primarily the removal of the `SecurityManager`, `Policy`, `AccessController` and other Security Manager API implementations. There is very little new code.
>> 
>> The code changes can be broken down into roughly the following categories:
>> 
>> 1. Degrading the behavior of Security Manager APIs to either throw Exceptions by default or provide an execution environment that disallows access to all resources by default.
>> 2. Changing hundreds of methods and constructors to no longer throw a `SecurityException` if a Security Manager was enabled. They will operate as they did in JDK 23 with no Security Manager enabled.
>> 3. Changing the `java` command to exit with a fatal error if a Security Manager is enabled.
>> 4. Removing the hotspot native code for the privileged stack walk and the inherited access control context. The remaining hotspot code and tests related to the Security Manager will be removed immediately after integration - see [JDK-8341916](https://bugs.openjdk.org/browse/JDK-8341916).
>> 5. Removing or modifying hundreds of tests. Many tests that tested Security Manager behavior are no longer relevant and thus have been removed or modified.
>> 
>> There are a handful of Security Manager related tests that are failing and are at the end of the `test/jdk/ProblemList.txt`, `test/langtools/ProblemList.txt` and `test/hotspot/jtreg/ProblemList.txt` files - these will be removed or separate bugs will be filed before integrating this PR. 
>> 
>> Inside the JDK, we have retained calls to `SecurityManager::getSecurityManager` and `AccessController::doPrivileged` for now, as these methods have been degraded to behave the same as they did in JDK 23 with no Security Manager enabled. After we integrate this JEP, those calls will be removed in each area (client-libs, core-libs, security, etc).
>> 
>> I don't expect each reviewer to review all the code changes in this JEP. Rather, I advise that you only focus on the changes for the area (client-libs, core-libs, net, ...
>
> Sean Mullan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 150 commits:
> 
>  - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
>  - Merge
>  - Update @summary to replace "if the right permission is granted" can be replaced with "package java.lang is open to unnamed module".
>  - Remove println about Security Manager.
>  - Remove unused static variable NEW_PROXY_IN_PKG.
>  - Remove static variable `DEFAULT_POLICY` and unused imports.
>  - Remove hasSM() method and code that calls it, and remove comment about
>    running test manually with SM.
>  - clientlibs: import order
>  - warning-string
>  - java/net/httpclient/websocket/security/WSURLPermissionTest.java: integrated review feedback in renamed WSSanityTest.java
>  - ... and 140 more: https://git.openjdk.org/jdk/compare/f7a61fce...cb50dfde

Reviewed java/util/* and corresponding tests.
Logging tests should refactor to eliminate use of doPrivileged(fn);

test/jdk/java/util/PluggableLocale/PermissionTest.java line 52:

> 50: import com.foo.NumberFormatProviderImpl;
> 51: 
> 52: public class PermissionTest{

This test can be deleted entirely. What remains is just constructing each provider impl.
The summary mentions a RuntimePermission and would need to be revised to a new description.

test/jdk/java/util/ResourceBundle/modules/security/src/test/jdk/test/Main.java line 48:

> 46:             throw new RuntimeException("unexpected resource bundle");
> 47:         }
> 48: 

This main and TestPermission.java duplicate the basic resource location mechanisms.
This test/Main.java an test/TestPermission can be removed entirely.

test/jdk/java/util/concurrent/Executors/PrivilegedCallables.java line 28:

> 26:  * @bug 6552961 6558429
> 27:  * @summary Test privilegedCallable, privilegedCallableUsingCurrentClassLoader
> 28:  * @run main PrivilegedCallables

There's nothing privileged here; the test should be renamed or deleted if it duplicates other non-privileged call tests.

test/jdk/java/util/logging/FileHandlerLongLimit.java line 157:

> 155:     static class Configure {
> 156:         static void setUp(Properties propertyFile) {
> 157:             doPrivileged(() -> {

The doPrivileged() could be factored out.
And callPrivileged().

test/jdk/java/util/logging/FileHandlerPath.java line 149:

> 147:     static class Configure {
> 148:         static void setUp(Properties propertyFile) {
> 149:             doPrivileged(() -> {

doPrivileged() could be refactored; it is no longer necessary.

test/jdk/java/util/logging/FileHandlerPatternExceptions.java line 106:

> 104:     static class Configure {
> 105:         static void setUp(Properties propertyFile) {
> 106:             doPrivileged(() -> {

doPrivileged() is no longer necessary.

test/jdk/java/util/logging/TestAppletLoggerContext.java line 40:

> 38:  * @modules java.base/jdk.internal.access
> 39:  *          java.logging
> 40:  * @run main/othervm TestAppletLoggerContext LoadingApplet

Rename these?  What's really being tested, there are no more Applets.

test/jdk/java/util/logging/TestLogConfigurationDeadLockWithConf.java line 83:

> 81:      * then the test is considered a success and passes.
> 82:      *
> 83:      * Note that 4sec may not be enough to detect issues if there are some.

Where is this timeout enforced or measured; this is just a comment, not a parameter.

test/micro/org/openjdk/bench/java/security/ProtectionDomainBench.java line 125:

> 123:     }
> 124: 
> 125:     @Benchmark

Is this benchmark still useful if it is not comparing the SM vs the Non-SM case?

-------------

PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2396279786
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817269899
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817285899
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817291039
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817304993
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817307433
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817310090
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817323513
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817327555
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817352471