RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v3]

[Weijun Wang]

On Tue, 11 Jun 2024 16:18:23 GMT, Kevin Walls <kevinw at openjdk.org> 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