On Tue, 15 Oct 2024 14:50:54 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> 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 > > src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnection.java > line 159: > >> 157: * is specified for the MBean. >> 158: * @throws IOException if a general communication exception >> occurred. >> 159: * @throws UnsupportedOperationException if {@code >> delegationSubject} is non-null. > > 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`. I restored the changes to `RMIConnection` to throw `SecurityException` but adjusted the text to say "is not authorized" instead of "does not have permission". See https://github.com/openjdk/jdk/pull/21498/commits/86ff71461ef1d695c02497626facda63c496a287. As we discussed offline, I also added a sentence to the `MBeanServer` class description to state that it and its subclasses may throw `SecurityException`s if the implementation doesn't authorize access to the underlying resource: https://github.com/openjdk/jdk/pull/21498/commits/44432e56a91a992150ee873e81282d1fe21e69ea. > src/java.management/share/classes/javax/management/remote/JMXAuthenticator.java > line 67: > >> 65: */ >> 66: public Subject authenticate(Object credentials); >> 67: } > > Should this be reverted? Authentication has not been removed. Yes. See the fix in https://github.com/openjdk/jdk/pull/21498/commits/23a43e0d90aff8754909785f582ba0666046cf6c. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1806952922 PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1806953524
