On Mon, 14 Oct 2024 13:52:24 GMT, Sean Mullan <mul...@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
