RFR: 8338411: Implement JEP 486: Permanently Disable the Security Manager

Tue Oct 15 15:19:23 UTC 2024

On Mon, 14 Oct 2024 13:52:24 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, security, etc) that you are most f...

Impressive work. I had a look at everything source in `net`, `logging`, `jmx` and `httpclient`.

Mostly good, but I was surprised to see an explicit new `throw new SecurityException()` in `java.util.logging`; Also JMX still supports authentication and coarse authorisation, which means that `SecurityException` can still be thrown by the `JMXAuthenticator` / `MBeanServerAccessController` on the server side and thrown on the client side. I have made some suggestions.

src/java.base/share/classes/java/net/URLClassLoader.java line 667:

> 665:      * @param codesource the codesource
> 666:      * @throws    NullPointerException if {@code codesource} is {@code null}.
> 667:      * @return the permissions for the codesource

Since there is no SecurityManager any more, I guess this method could be, in the future, degraded to return an empty collection? Then when that's done the API documentation above could be trivially simplified to `{@return an empty {@code PermissionCollection}}`?

src/java.logging/share/classes/java/util/logging/LogManager.java line 2430:

> 2428:     @Deprecated(since="17", forRemoval=true)
> 2429:     public void checkAccess() {
> 2430:         throw new SecurityException();

Though this method is no longer called in the JDK, this is a change of behaviour that could affect subclasses of `LogManager`, or code using the `LogManager` that might still be calling this method. This method is deprecated for removal, and degrading it to always throw an exception is a logical step prior to removing it. However, I wonder if this shouldn't better be done separately, outside of this JEP?

src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java line 159:

> 157:      * is specified for the MBean.
> 158:      * @throws SecurityException if the client does not have permission
> 159:      * to perform this operation.

Maybe we should revert those changes, or word them differently. AFAIU, is is still possible for a JMXConnectorServer to implement coarse grained authorization by setting up an `MBeanServerAccessController`, and in fact, the default JMX Agent does that. The JDK has a built-in implementation of `MBeanServerAccessController`,  `MBeanServerFileAccessController`, which will throw `SecurityException` if access is denied by the `MBeanServerFileAccessController`. I believe this will result in the `RMIConnection` throwing `SecurityException` if the operation is denied by the `MBeanServerAccessController`.

So I believe - in all methods here and in `RMIConnectionImpl`, we should leave the door open for `SecurityException` to get thrown.

An alternative could be to cover that use case with a blanket statement, here, in `RMIConnectionImpl`, in `MBeanServer`, and in `MBeanServerConnection`.

src/java.management/share/classes/javax/management/remote/JMXAuthenticator.java line 67:

> 65:      *
> 66:      * @exception SecurityException if the server cannot authenticate the user
> 67:      * with the provided credentials.

Should this be reverted? Authentication has not been removed.

src/java.management/share/classes/javax/management/remote/JMXConnectorFactory.java line 225:

> 223:      *
> 224:      * @exception SecurityException if the connection cannot be made
> 225:      * for security reasons.

I wonder if these changes should also be reverted, on the ground that a server may have authentication in place and may refuse the connection. Same below.

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

Changes requested by dfuchs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21498#pullrequestreview-2369425602
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1801215698
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1801291195
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1801341618
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1801357691
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1801362306