On Tue, 11 Jun 2024 16:18:23 GMT, Kevin Walls <[email protected]> wrote:
>> JMX uses APIs related to the Security Mananger which are deprecated. Use of
>> AccessControlContext will be removed when Security Manager is removed.
>>
>> Until then, updates are needed to not require setting
>> -Djava.security.manager=allow to use JMX authentication.
>
> Kevin Walls has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Sean comments
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1301:
> 1299: }
> 1300: };
> 1301: if (acc == null) {
This is a comment to all the code changes in this PR. I would recommend we make
use of the `allowSecurityManager()` call everywhere when the behavior is
different, like this:
if (SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
if (acc == null)
return action.run();
else
return AccessController.doPrivileged(action, acc);
} else {
if (subject == null)
return action.run();
else
return Subject.doAs(subject, action);
}
This makes me much more confident.
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1436:
> 1434: } else {
> 1435: // ACC is present, we have a Subject and SM is
> permitted:
> 1436: return
> AccessController.doPrivileged((PrivilegedExceptionAction<Object>) () ->
> Subject.doAs(subject, op), acc);
Why is it necessary to call both `doAs` and `doPrivileged`?
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1438:
> 1436: return
> AccessController.doPrivileged((PrivilegedExceptionAction<Object>) () ->
> Subject.doAs(subject, op), acc);
> 1437: }
> 1438: } catch (PrivilegedActionException pe) {
What is this catch block for? The cause of a `PrivilegedActionException` should
not be a unchecked exception.
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1453:
> 1451: throw new RuntimeException(cause);
> 1452: }
> 1453: } catch (Exception e) {
Is this block only for the `op.run()` line? Why not keep the old try-catch
block around it and move the logic here?
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1633:
> 1631: }
> 1632: } else {
> 1633: // ACC is present, we have a Subject and SM is
> permitted:
While extract the `action` variable? The old code on lines 1590-1592 has no
problem.
src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
line 350:
> 348: @SuppressWarnings("removal")
> 349: private Subject getSubject() {
> 350: Subject subject = null;
Just call `Subject.current()`. When SM is allowed, it is equivalent to
`getSubject(AccessController.getContext())`.
src/java.management/share/classes/javax/management/monitor/Monitor.java line
718:
> 716: // The monitor tasks will be executed within this context.
> 717: //
> 718: subject = Subject.current();
If `subject` is only used when SM is not allowed, you can put the assignment
above to the else if next line.
src/java.management/share/classes/javax/management/monitor/Monitor.java line
1541:
> 1539: if (ac == null) {
> 1540: // No SecurityManager:
> 1541: Subject.doAs(s, action);
What if `s` is null?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635218675
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635202721
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635205035
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635206172
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635208088
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635209275
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635212375
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635213527
