Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Tue, 18 Jun 2024 11:33:51 GMT, Daniel Fuchs wrote: >> Agree with Kevin. To minimize risk, especially since this is to fix a >> significant regression and we are in RDP1, we are really trying to preserve >> the existing code as much as possible, even though it could be improved. > > It is also more error prone (which is why I suggested using a single well > tested utility method to implement the temporary hackery rather than > spreading it at different places in different forms). But I'm OK with the > code in this form. Thanks Daniel! - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1644374757
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 20:27:05 GMT, Sean Mullan wrote: >> Hi, >> We almost always check >> !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the >> RMIConnectionImpl contstructor) >> >> This keeps the "modern" case first (except in that constructor, where it is >> only one line for each side of the if...). >> >> This extra checking of >> SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the >> modern and old style cases distinct, based on other comments here. It keeps >> it simple to read I hope, but yes it is a little more verbose than it could >> be. >> >> I'm hoping you'll agree that as these checks will be removed anyway, >> probably with a release, we should delay more extensive cleanup until then. >> (I've also rolled back my noPermissionsACC removal to keep this change away >> from being a general cleanup.) >> >> I had a bunch of additional Exception handling for e.g. >> PrivilegedActionException I think in here, and it wasn't very pretty. But, >> it might look better when there are fewer nested ifs in these methods, and >> when we are properly in the post-SecurityManager world... 8-) >> >> Whether it checks acc != null or acc == null is based on the existing code. >> I think that makes this diff easier to read, but also could be reworked and >> be made more consistent after SM removal. > > Agree with Kevin. To minimize risk, especially since this is to fix a > significant regression and we are in RDP1, we are really trying to preserve > the existing code as much as possible, even though it could be improved. It is also more error prone (which is why I suggested using a single well tested utility method to implement the temporary hackery rather than spreading it at different places in different forms). But I'm OK with the code in this form. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1644303322
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 15:53:19 GMT, Kevin Walls wrote: >> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java >> line 1314: >> >>> 1312: return AccessController.doPrivileged(action, acc); >>> 1313: } >>> 1314: } >> >> This piece of code is repeated at several places in similar but different >> forms - sometime we check for >> `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! >> SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check >> for `acc == null`, sometime for `acc != null` etc.. >> I wonder if we could abstract that to some utility method somewhere. Would >> that make sense? Maybe that's not possible due to using sometime >> `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we >> could use `PrivilegedExceptionAction` everywhere at the cost of handling >> `PrivilegedActionException`? >> If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds >> again an equivalent amount of clutter) then maybe we could at least make >> sure we do the same checks in the same way (typically `acc == null` vs `acc >> != null`)? > > Hi, > We almost always check > !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the > RMIConnectionImpl contstructor) > > This keeps the "modern" case first (except in that constructor, where it is > only one line for each side of the if...). > > This extra checking of > SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the > modern and old style cases distinct, based on other comments here. It keeps > it simple to read I hope, but yes it is a little more verbose than it could > be. > > I'm hoping you'll agree that as these checks will be removed anyway, probably > with a release, we should delay more extensive cleanup until then. (I've > also rolled back my noPermissionsACC removal to keep this change away from > being a general cleanup.) > > I had a bunch of additional Exception handling for e.g. > PrivilegedActionException I think in here, and it wasn't very pretty. But, > it might look better when there are fewer nested ifs in these methods, and > when we are properly in the post-SecurityManager world... 8-) > > Whether it checks acc != null or acc == null is based on the existing code. > I think that makes this diff easier to read, but also could be reworked and > be made more consistent after SM removal. Agree with Kevin. To minimize risk, especially since this is to fix a significant regression and we are in RDP1, we are really trying to preserve the existing code as much as possible, even though it could be improved. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1643395182
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 13:58:11 GMT, Daniel Fuchs wrote: >> Kevin Walls has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - style update >> - whitespace > > src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java > line 1314: > >> 1312: return AccessController.doPrivileged(action, acc); >> 1313: } >> 1314: } > > This piece of code is repeated at several places in similar but different > forms - sometime we check for > `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! > SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check > for `acc == null`, sometime for `acc != null` etc.. > I wonder if we could abstract that to some utility method somewhere. Would > that make sense? Maybe that's not possible due to using sometime > `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we > could use `PrivilegedExceptionAction` everywhere at the cost of handling > `PrivilegedActionException`? > If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds > again an equivalent amount of clutter) then maybe we could at least make sure > we do the same checks in the same way (typically `acc == null` vs `acc != > null`)? Hi, We almost always check !SharedSecrets.getJavaLangAccess().allowSecurityManager() (except in the RMIConnectionImpl contstructor) This keeps the "modern" case first (except in that constructor, where it is only one line for each side of the if...). This extra checking of SharedSecrets.getJavaLangAccess().allowSecurityManager() is to keep the modern and old style cases distinct, based on other comments here. It keeps it simple to read I hope, but yes it is a little more verbose than it could be. I'm hoping you'll agree that as these checks will be removed anyway, probably with a release, we should delay more extensive cleanup until then. (I've also rolled back my noPermissionsACC removal to keep this change away from being a general cleanup.) I had a bunch of additional Exception handling for e.g. PrivilegedActionException I think in here, and it wasn't very pretty. But, it might look better when there are fewer nested ifs in these methods, and when we are properly in the post-SecurityManager world... 8-) Whether it checks acc != null or acc == null is based on the existing code. I think that makes this diff easier to read, but also could be reworked and be made more consistent after SM removal. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1643036701
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
On Mon, 17 Jun 2024 12:54:44 GMT, Kevin Walls 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 two additional > commits since the last revision: > > - style update > - whitespace The new version looks much better to me. I wonder if we can abstract away some of the repeated logic though. src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java line 1314: > 1312: return AccessController.doPrivileged(action, acc); > 1313: } > 1314: } This piece of code is repeated at several places in similar but different forms - sometime we check for `SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime for `! SharedSecrets.getJavaLangAccess().allowSecurityManager()`, sometime we check for `acc == null`, sometime for `acc != null` etc.. I wonder if we could abstract that to some utility method somewhere. Would that make sense? Maybe that's not possible due to using sometime `PrivelegedAction` and sometimes `PrivilegedExceptionAction` - but maybe we could use `PrivilegedExceptionAction` everywhere at the cost of handling `PrivilegedActionException`? If that doesn't prove useful (if handling `PrivilegedExceptionAction` adds again an equivalent amount of clutter) then maybe we could at least make sure we do the same checks in the same way (typically `acc == null` vs `acc != null`)? - PR Review: https://git.openjdk.org/jdk/pull/19624#pullrequestreview-2118725434 PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1642864523
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v16]
> 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 two additional commits since the last revision: - style update - whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/2f2179e7..597264b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=14-15 Stats: 11 lines in 2 files changed: 4 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/19624.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19624/head:pull/19624 PR: https://git.openjdk.org/jdk/pull/19624
