Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 14:48:14 GMT, Weijun Wang wrote: > > Does noPermissionsACC add anything? > > I don't know. My principal for this code change is that nothing is changed > for the SM-is-allowed case. I've put back the noPermissionsACC for this change, it does not have to be removed in this change. It was introduced in jdk6, and we will remove it next time we are here when we drop ACC usage. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2172948152
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 14:51:53 GMT, Weijun Wang wrote: >> It needs to recognise and throw RuntimeException so that a SecurityException >> isn't wrapped in a PrivilegedActionException (which gets caught by those >> blocks of code which call extractException(pe) and look at what Exception >> it contains). >> >> (test/jdk/javax/management/remote/mandatory/notif/NotificationAccessControllerTest.java >> tests this) >> >> I can add a check to make sure PrivilgedActionException doesn't get wrapped. >> Daniel I think mentioned this also. > > I still don't understand how a SecurityException could be wrapped into a PAE > without these lines. I misunderstood the comment, yes I think I can remove this block now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639983304
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 14:00:58 GMT, Kevin Walls wrote:
>> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
>> line 1461:
>>
>>> 1459: throw rte;
>>> 1460: } else {
>>> 1461: throw new PrivilegedActionException(e);
>>
>> The 4 lines above seems unnecessary now. Plus, do not wrap
>> `PrivilegedActionException` inside `PrivilegedActionException`.
>
> It needs to recognise and throw RuntimeException so that a SecurityException
> isn't wrapped in a PrivilegedActionException (which gets caught by those
> blocks of code which call extractException(pe) and look at what Exception it
> contains).
>
> (test/jdk/javax/management/remote/mandatory/notif/NotificationAccessControllerTest.java
> tests this)
>
> I can add a check to make sure PrivilgedActionException doesn't get wrapped.
> Daniel I think mentioned this also.
I still don't understand how a SecurityException could be wrapped into a PAE
without these lines.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639950681
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:41:20 GMT, Kevin Walls wrote: > Does noPermissionsACC add anything? I don't know. My principal for this code change is that nothing is changed for the SM-is-allowed case. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2168203868
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:03:07 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Separate SM allowed and not allowed cases
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1461:
>
>> 1459: throw rte;
>> 1460: } else {
>> 1461: throw new PrivilegedActionException(e);
>
> The 4 lines above seems unnecessary now. Plus, do not wrap
> `PrivilegedActionException` inside `PrivilegedActionException`.
It needs to recognise and throw RuntimeException so that a SecurityException
isn't wrapped in a PrivilegedActionException (which gets caught by those blocks
of code which call extractException(pe) and look at what Exception it
contains).
(test/jdk/javax/management/remote/mandatory/notif/NotificationAccessControllerTest.java
tests this)
I can add a check to make sure PrivilgedActionException doesn't get wrapped.
Daniel I think mentioned this also.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639876260
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:44:17 GMT, Kevin Walls wrote:
>> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
>> line 353:
>>
>>> 351: } else {
>>> 352: return Subject.getSubject(AccessController.getContext());
>>> 353: }
>>
>> `Subject.current()` should work for both cases. See the impl of it.
>
> It will work to get the Subject yes.
> Do I not need the SM-enabled case, in case there some complex ACC in place?
> (combiner)
OK if it was actually getting specifically the ACC to use it maybe, but this
just needs to resolve a Subject.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639806380
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:04:06 GMT, Weijun Wang wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Separate SM allowed and not allowed cases
>
> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
> line 353:
>
>> 351: } else {
>> 352: return Subject.getSubject(AccessController.getContext());
>> 353: }
>
> `Subject.current()` should work for both cases. See the impl of it.
ok!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639766973
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Fri, 14 Jun 2024 12:09:46 GMT, Weijun Wang wrote: > I don't quite understand why there is no more `noPermissionsACC` in > `Monotor.java`. This looks like the only behavior change when SM is allowed. > The other source change looks fine to me. Does noPermissionsACC add anything? Maybe I'm rushing the removal of ACCs where I can. Start sets acc to the current context (when SM permitted, use Subject otherwise) Set acc to noPermissionsACC when stop is called (or forget the Subject). acc is accessed within the run method. - PR Comment: https://git.openjdk.org/jdk/pull/19624#issuecomment-2167949489
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
On Thu, 13 Jun 2024 20:54:25 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 one additional
> commit since the last revision:
>
> Separate SM allowed and not allowed cases
I don't quite understand why there is no more `noPermissionsACC` in
`Monotor.java`. This looks like the only behavior change when SM is allowed.
The other source change looks fine to me.
src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
line 1461:
> 1459: throw rte;
> 1460: } else {
> 1461: throw new PrivilegedActionException(e);
The 4 lines above seems unnecessary now. Plus, do not wrap
`PrivilegedActionException` inside `PrivilegedActionException`.
src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
line 353:
> 351: } else {
> 352: return Subject.getSubject(AccessController.getContext());
> 353: }
`Subject.current()` should work for both cases. See the impl of it.
-
PR Review: https://git.openjdk.org/jdk/pull/19624#pullrequestreview-2118220033
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639720192
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1639721194
Re: RFR: 8333344: JMX attaching of Subject does not work when security manager not allowed [v10]
> 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: Separate SM allowed and not allowed cases - Changes: - all: https://git.openjdk.org/jdk/pull/19624/files - new: https://git.openjdk.org/jdk/pull/19624/files/f70047a4..09c2e38f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19624&range=08-09 Stats: 52 lines in 1 file changed: 31 ins; 8 del; 13 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
