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

Tue Oct 22 21:39:31 UTC 2024

On Fri, 18 Oct 2024 19:03:30 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 97 commits:
> 
>  - Merge remote-tracking branch 'jdk-sandbox/jep486' into JDK-8338411
>  - Change apiNote to deprecated annotation on checkAccess methods. Change method dedescription to "Does nothing".
>  - Sanitize the class descriptions of DelegationPermission and ServicePermission
>    by removing text that refers to granting permissions, but avoid changes that
>    affect the API specification, such as the description and format of input
>    parameters.
>  - Restored methods in RMIConnection to throw SecurityExceptions again but
>    with adjusted text that avoids the word "permission".
>  - Add text to class description of MBeanServer stating that implementations
>    may throw SecurityException if authorization doesn't allow access to resource.
>  - Restore text about needing permissions from the desktop environment in the
>    getPixelColor and createScreenCapture methods.
>  - Add api note to getClassContext to use StackWalker instead and
>    add DROP_METHOD_INFO option to StackWalker.
>  - Change checkAccess() methods to be no-ops, rather than throwing
>    SecurityException.
>  - Merge
>  - Merge
>  - ... and 87 more: https://git.openjdk.org/jdk/compare/f50bd0d9...f89d9d09

Reviewed test/jdk/java/lang/** and test/jdk/sun/reflect/* tests.

test/jdk/java/lang/Class/getDeclaredField/ClassDeclaredFieldsTest.java line 31:

> 29:  * @summary test that all fields returned by getDeclaredFields() can be
> 30:  *          set accessible if the right permission is granted; this test
> 31:  *          also verifies that Class.classLoader final private field is

"if the right permission is granted" can be replaced with "package java.lang is open to unnamed module".

test/jdk/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java line 338:

> 336: 
> 337:     public static enum TestCase {
> 338:         UNSECURE;

Better to drop this enum entirely.   Simply call `FieldSetAccessibleTest::run` as it's the only test case.

test/jdk/java/lang/StackWalker/CallerSensitiveMethod/csm/jdk/test/CallerSensitiveTest.java line 45:

> 43: 
> 44:     public static void main(String... args) throws Throwable {
> 45:         System.err.println("Test without security manager.");

Security manager is not relevant any more.  Suggest to drop this println.

test/jdk/java/lang/invoke/RevealDirectTest.java line 33:

> 31:  * @test
> 32:  * @summary verify Lookup.revealDirect on a variety of input handles, with security manager
> 33:  * @run main/othervm/policy=jtreg.security.policy/secure=java.lang.SecurityManager -ea -esa test.java.lang.invoke.RevealDirectTest

line 36 can also be removed.

* $ $JAVA8X_HOME/bin/java -cp $JUNIT4_JAR:../../../.. -ea -esa -Djava.security.manager test.java.lang.invoke.RevealDirectTest

test/jdk/java/lang/invoke/callerSensitive/csm/jdk/test/MethodInvokeTest.java line 77:

> 75:             @Override
> 76:             public boolean implies(ProtectionDomain domain, Permission p) {
> 77:                 return perms.implies(p) || DEFAULT_POLICY.implies(domain, p);

static variable `DEFAULT_POLICY` (line 48) can be removed.

test/jdk/java/lang/reflect/Proxy/nonPublicProxy/NonPublicProxyClass.java line 83:

> 81:     }
> 82: 
> 83:     private static final String NEW_PROXY_IN_PKG = "newProxyInPackage.";

This constant is no longer needed.

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

PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2383401067
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1809627796
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1809631220
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1811445313
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1811458290
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1811462419
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1809602637