Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 5 Mar 2024 19:53:46 GMT, Weijun Wang wrote: >> Subject is stored in the RMIConnectionImpl: >> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java >> >> (That is complicated by SubjectDelegation, which we deprecated for removal. >> I have the PR out to remove it: >> https://github.com/openjdk/jdk/pull/18025 ) >> >> makeClient in RMIJRMPServerImpl creates RMIConnectionImpl >> >> ..and RMIServerImpl.java has a doNewClient method calling that. This is >> what takes a Credentials Object and deals withJMXAuthenticator to get an >> authenticated Subject. None of this requires the SM. > > I see that in `RMIConnectionImpl.java` it is stored inside an > `AccessControlContext`, and there are `doPrivileged` calls on this ACC to > pass the subject into an action. So, even if there might be no SM but the > subject will never be bound to a thread using a scoped value. > > I’ll revert the change then, and this code must have SM allowed to run > correctly. If user runs it with SM disabled, at least they will see an UOE > instead of letting `current()` silently returns a null. > > Ultimately, if we want it working after SM, we should update > `RMIConnectionImpl` and rewrite the ACC-related code to using > `Subject.callAs`. Yes - the JMX implementation stores and retrieve subjects in the AccessControlContext and then uses doPrivileged. Subject.doAs is not used in the JMX implementation. There are two different uses of Subject in JMX: 1. one is a simplified role-based authentication/authorization at the default agent level. 2. The other one is a permission check where different subjects can be granted different privileges in the policy file. The latter will go away since the SM is going away, but needs to be preserved until then. The former we want to keep and needs to keep working (by changing the code to use callAs) even after the SM is gone. Subject delegation allows an authenticated subject (1. above) to perform actions on behalf of a delegation subject, where the privileged granted by 2. are the intersection of the privileges of the two subjects. @kevinjwalls is currently working on removing subject delegation. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1515896397
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 5 Mar 2024 16:49:01 GMT, Kevin Walls wrote: >> Do you know where the subject is set? If it's set by a `doAs` call then it >> will co-operate with `current()` no matter if SM is allowed. I tried to >> search in the whole module and cannot find a `doAs` call. If it is also >> through `SubjectDomainCombiner` then it only works with SM. > > Subject is stored in the RMIConnectionImpl: > src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java > > (That is complicated by SubjectDelegation, which we deprecated for removal. > I have the PR out to remove it: > https://github.com/openjdk/jdk/pull/18025 ) > > makeClient in RMIJRMPServerImpl creates RMIConnectionImpl > > ..and RMIServerImpl.java has a doNewClient method calling that. This is what > takes a Credentials Object and deals withJMXAuthenticator to get an > authenticated Subject. None of this requires the SM. I see that in `RMIConnectionImpl.java` it is stored inside an `AccessControlContext`, and there are `doPrivileged` calls on this ACC to pass the subject into an action. So, even if there might be no SM but the subject will never be bound to a thread using a scoped value. I’ll revert the change then, and this code must have SM allowed to run correctly. If user runs it with SM disabled, at least they will see an UOE instead of letting `current()` silently returns a null. Ultimately, if we want it working after SM, we should update `RMIConnectionImpl` and rewrite the ACC-related code to using `Subject.callAs`. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1513410552
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 5 Mar 2024 14:44:29 GMT, Weijun Wang wrote: >> Right, this does not depend on the SM. All we need to do is get the >> Subject. >> This method implements the basic monitor (readonly) and control (readwrite) >> access. >> accessMap maps identity String to Access, and the checkAccess() method here >> will check the Subject by using of its Principal names as keys in that map. > > Do you know where the subject is set? If it's set by a `doAs` call then it > will co-operate with `current()` no matter if SM is allowed. I tried to > search in the whole module and cannot find a `doAs` call. If it is also > through `SubjectDomainCombiner` then it only works with SM. Subject is stored in the RMIConnectionImpl: src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java (That is complicated by SubjectDelegation, which we deprecated for removal. I have the PR out to remove it: https://github.com/openjdk/jdk/pull/18025 ) makeClient in RMIJRMPServerImpl creates RMIConnectionImpl ..and RMIServerImpl.java has a doNewClient method calling that. This is what takes a Credentials Object and deals withJMXAuthenticator to get an authenticated Subject. None of this requires the SM. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1513164360
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 5 Mar 2024 11:36:53 GMT, Kevin Walls wrote: >> I think we need @kevinjwalls or @dfuch to help advise on this. > > Right, this does not depend on the SM. All we need to do is get the Subject. > This method implements the basic monitor (readonly) and control (readwrite) > access. > accessMap maps identity String to Access, and the checkAccess() method here > will check the Subject by using of its Principal names as keys in that map. Do you know where the subject is set? If it's set by a `doAs` call then it will co-operate with `current()` no matter if SM is allowed. I tried to search in the whole module and cannot find a `doAs` call. If it is also through `SubjectDomainCombiner` then it only works with SM. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1512951092
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Mon, 4 Mar 2024 19:57:25 GMT, Sean Mullan wrote: >> I was not exactly sure if we will support this functionality when there is >> no SM. The class name has `AccessControler` and the method names use >> `checkAccess`, but they actually do not always depend on security manager. > > I think we need @kevinjwalls or @dfuch to help advise on this. Right, this does not depend on the SM. All we need to do is get the Subject. This method implements the basic monitor (readonly) and control (readwrite) access. accessMap maps identity String to Access, and the checkAccess() method here will check the Subject by using of its Principal names as keys in that map. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1512676642
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Mon, 4 Mar 2024 16:17:14 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix MBeanServerFileAccessController, more test in SM
>
> src/java.base/share/classes/javax/security/auth/Subject.java line 120:
>
>> 118: * {@code getSubject(AccessControlContext)} method.
>> 119: * If a security manager is not allowed, which means it
>> 120: * {@linkplain System#setSecurityManager is not set and not allowed to
>> be set
>
> I think `SecurityManager.html#set-security-manager` is a better (more
> informative) link. Also, not sure why it is linked here but not in the "If a
> security manager is allowed" paragraph. If you link it in the first sentence
> ("These methods behave differently ...) then you probably only need one link
> and don't need this link.
OK.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511771517
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Mon, 4 Mar 2024 15:47:41 GMT, Sean Mullan wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix MBeanServerFileAccessController, more test in SM > > test/jdk/javax/security/auth/Subject/CallAsWithScopedValue.java line 27: > >> 25: * @test >> 26: * @bug 8296244 >> 27: * @enablePreview > > Can you add a comment as to why `enablePreview` is needed? (Assume it is for > `StructuredTaskScope`.) OK. > test/jdk/javax/security/auth/Subject/Compat.java line 34: > >> 32: /* >> 33: * @test >> 34: * @run main/othervm -Djava.security.manager=allow Compat > > Missing `@summary` and `@bug`. Will Add. - PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511760021 PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511761591
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Mon, 4 Mar 2024 15:15:54 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix MBeanServerFileAccessController, more test in SM
>
> test/jdk/javax/management/monitor/ThreadPoolAccTest.java line 69:
>
>> 67: }
>> 68: private void setPrincipal() {
>> 69: Subject subject = Subject.current();
>
> Why change this to `Subject.current()` if you are requiring the SM to be
> allowed?
When SM is allowed, `Subject.current()` is equivalent to
`Subject.getSubject(AccessController.getContext())`. I can revert the change.
> test/jdk/javax/security/auth/Subject/UnsupportedSV.java line 59:
>
>> 57:
>> 58: // TODO: Still has no way to reject the following code.
>> 59: // Here, AccessController::getContext returns a plan ACC without
>
> typo: s/plan/plain/
Yes.
> test/jdk/javax/security/auth/Subject/doAs/NestedActions.java line 283:
>
>> 281: static void readFile(String filename) {
>> 282: System.out.println("ReadFromFileAction: try to read " +
>> filename);
>> 283: Subject subject = Subject.current();
>
> Couldn't we have just left these calling `Subject.getSubject` for now since
> these tests run with an SM enabled?
Yes, we can.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511755373
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511756265
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511755860
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Mon, 4 Mar 2024 19:51:38 GMT, Weijun Wang wrote:
>> src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
>> line 309:
>>
>>> 307: final Subject s;
>>> 308: if (!SharedSecrets.getJavaLangAccess().allowSecurityManager())
>>> {
>>> 309: s = Subject.current();
>>
>> We may not want to call `Subject.current()` here, as this may imply that we
>> will support this functionality even if an SM is not enabled.
>
> I was not exactly sure if we will support this functionality. The class name
> has `AccessControler` and the method names use `checkAccess`, but they
> actually do not always depend on security manager.
I think we need @kevinjwalls or @dfuch to help advise on this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511721920
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Mon, 4 Mar 2024 15:28:28 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fix MBeanServerFileAccessController, more test in SM
>
> src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
> line 309:
>
>> 307: final Subject s;
>> 308: if (!SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
>> 309: s = Subject.current();
>
> We may not want to call `Subject.current()` here, as this may imply that we
> will support this functionality even if an SM is not enabled.
I was not exactly sure if we will support this functionality. The class name
has `AccessControler` and the method names use `checkAccess`, but they actually
do not always depend on security manager.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511716084
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 30 Jan 2024 21:58:28 GMT, Weijun Wang wrote:
>> This code change adds an alternative implementation of user-based
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs.
>> Depending on if the Security Manager is allowed, the methods store the
>> current subject differently. See the spec change in the `Subject.java` file
>> for details. When the Security Manager APIs are finally removed in a future
>> release, this new implementation will be only implementation for these
>> methods.
>>
>> One major change in the new implementation is that `Subject.getSubject`
>> always throws an `UnsupportedOperationException` since it has an
>> `AccessControlContext` argument but the current subject is no longer
>> associated with an `AccessControlContext` object.
>>
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to
>> `current` and `callAs`. If the user application is simply calling
>> `getSubject(AccessController.getContext())`, then switching to `current()`
>> would work. If the `AccessControlContext` argument is retrieved from an
>> earlier `getContext()` call and the associated subject might be different
>> from that of the current `AccessControlContext`, then instead of storing the
>> previous `AccessControlContext` object and passing it into `getSubject` to
>> get the "previous" subject, the application should store the `current()`
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix MBeanServerFileAccessController, more test in SM
src/java.base/share/classes/javax/security/auth/Subject.java line 112:
> 110: *
> 111: * These methods behave differently depending on
> 112: * whether a security manager is allowed or disallowed:
Suggest linking "a security manager is allowed or disallowed" to
`SecurityManager.html#set-security-manager`.
src/java.base/share/classes/javax/security/auth/Subject.java line 120:
> 118: * {@code getSubject(AccessControlContext)} method.
> 119: * If a security manager is not allowed, which means it
> 120: * {@linkplain System#setSecurityManager is not set and not allowed to
> be set
I think `SecurityManager.html#set-security-manager` is a better (more
informative) link. Also, not sure why it is linked here but not in the "If a
security manager is allowed" paragraph. If you link it in the first sentence
("These methods behave differently ...) then you probably only need one link
and don't need this link.
test/jdk/javax/security/auth/Subject/CallAsWithScopedValue.java line 27:
> 25: * @test
> 26: * @bug 8296244
> 27: * @enablePreview
Can you add a comment as to why `enablePreview` is needed? (Assume it is for
`StructuredTaskScope`.)
test/jdk/javax/security/auth/Subject/Compat.java line 34:
> 32: /*
> 33: * @test
> 34: * @run main/othervm -Djava.security.manager=allow Compat
Missing `@summary` and `@bug`.
test/jdk/javax/security/auth/Subject/UnsupportedSV.java line 59:
> 57:
> 58: // TODO: Still has no way to reject the following code.
> 59: // Here, AccessController::getContext returns a plan ACC without
typo: s/plan/plain/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511419716
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511424024
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511380094
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511393254
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511366395
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 30 Jan 2024 21:58:28 GMT, Weijun Wang wrote:
>> This code change adds an alternative implementation of user-based
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs.
>> Depending on if the Security Manager is allowed, the methods store the
>> current subject differently. See the spec change in the `Subject.java` file
>> for details. When the Security Manager APIs are finally removed in a future
>> release, this new implementation will be only implementation for these
>> methods.
>>
>> One major change in the new implementation is that `Subject.getSubject`
>> always throws an `UnsupportedOperationException` since it has an
>> `AccessControlContext` argument but the current subject is no longer
>> associated with an `AccessControlContext` object.
>>
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to
>> `current` and `callAs`. If the user application is simply calling
>> `getSubject(AccessController.getContext())`, then switching to `current()`
>> would work. If the `AccessControlContext` argument is retrieved from an
>> earlier `getContext()` call and the associated subject might be different
>> from that of the current `AccessControlContext`, then instead of storing the
>> previous `AccessControlContext` object and passing it into `getSubject` to
>> get the "previous" subject, the application should store the `current()`
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix MBeanServerFileAccessController, more test in SM
test/jdk/javax/security/auth/Subject/doAs/NestedActions.java line 283:
> 281: static void readFile(String filename) {
> 282: System.out.println("ReadFromFileAction: try to read " +
> filename);
> 283: Subject subject = Subject.current();
Couldn't we have just left these calling `Subject.getSubject` for now since
these tests run with an SM enabled?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511358336
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 30 Jan 2024 21:58:28 GMT, Weijun Wang wrote:
>> This code change adds an alternative implementation of user-based
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs.
>> Depending on if the Security Manager is allowed, the methods store the
>> current subject differently. See the spec change in the `Subject.java` file
>> for details. When the Security Manager APIs are finally removed in a future
>> release, this new implementation will be only implementation for these
>> methods.
>>
>> One major change in the new implementation is that `Subject.getSubject`
>> always throws an `UnsupportedOperationException` since it has an
>> `AccessControlContext` argument but the current subject is no longer
>> associated with an `AccessControlContext` object.
>>
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to
>> `current` and `callAs`. If the user application is simply calling
>> `getSubject(AccessController.getContext())`, then switching to `current()`
>> would work. If the `AccessControlContext` argument is retrieved from an
>> earlier `getContext()` call and the associated subject might be different
>> from that of the current `AccessControlContext`, then instead of storing the
>> previous `AccessControlContext` object and passing it into `getSubject` to
>> get the "previous" subject, the application should store the `current()`
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix MBeanServerFileAccessController, more test in SM
src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
line 309:
> 307: final Subject s;
> 308: if (!SharedSecrets.getJavaLangAccess().allowSecurityManager()) {
> 309: s = Subject.current();
We may not want to call `Subject.current()` here, as this may imply that we
will support this functionality even if an SM is not enabled.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511351340
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 30 Jan 2024 21:58:28 GMT, Weijun Wang wrote:
>> This code change adds an alternative implementation of user-based
>> authorization `Subject` APIs that doesn't depend on Security Manager APIs.
>> Depending on if the Security Manager is allowed, the methods store the
>> current subject differently. See the spec change in the `Subject.java` file
>> for details. When the Security Manager APIs are finally removed in a future
>> release, this new implementation will be only implementation for these
>> methods.
>>
>> One major change in the new implementation is that `Subject.getSubject`
>> always throws an `UnsupportedOperationException` since it has an
>> `AccessControlContext` argument but the current subject is no longer
>> associated with an `AccessControlContext` object.
>>
>> Now it's the time to migrate from the `getSubject` and `doAs` methods to
>> `current` and `callAs`. If the user application is simply calling
>> `getSubject(AccessController.getContext())`, then switching to `current()`
>> would work. If the `AccessControlContext` argument is retrieved from an
>> earlier `getContext()` call and the associated subject might be different
>> from that of the current `AccessControlContext`, then instead of storing the
>> previous `AccessControlContext` object and passing it into `getSubject` to
>> get the "previous" subject, the application should store the `current()`
>> return value directly.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix MBeanServerFileAccessController, more test in SM
test/jdk/javax/management/monitor/ThreadPoolAccTest.java line 69:
> 67: }
> 68: private void setPrincipal() {
> 69: Subject subject = Subject.current();
Why change this to `Subject.current()` if you are requiring the SM to be
allowed?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1511332276
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
On Tue, 30 Jan 2024 16:41:28 GMT, Weijun Wang wrote:
>> src/java.management/share/classes/com/sun/jmx/remote/security/MBeanServerFileAccessController.java
>> line 307:
>>
>>> 305: AccessController.doPrivileged(new PrivilegedAction<>() {
>>> 306: public Subject run() {
>>> 307: return Subject.current();
>>
>> Is the `doPrivileged` still needed here? Is there a chance that
>> `Subject.current()` will throw a `SecurityException`, or return a different
>> result if a security manager is present and `doPrivileged` is not used?
>
> When a security manager is set, `current()` still calls `getSubject()` and it
> needs a permission unless it's called inside `doPrivileged`. But, see the
> comment above.
I fixed it in the latest commit. The original code change is simply wrong.
`AccessController.getContext()` would return different ACCs inside and outside
`doPriv`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1472043888
Re: RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs [v3]
> This code change adds an alternative implementation of user-based > authorization `Subject` APIs that doesn't depend on Security Manager APIs. > Depending on if the Security Manager is allowed, the methods store the > current subject differently. See the spec change in the `Subject.java` file > for details. When the Security Manager APIs are finally removed in a future > release, this new implementation will be only implementation for these > methods. > > One major change in the new implementation is that `Subject.getSubject` > always throws an `UnsupportedOperationException` since it has an > `AccessControlContext` argument but the current subject is no longer > associated with an `AccessControlContext` object. > > Now it's the time to migrate from the `getSubject` and `doAs` methods to > `current` and `callAs`. If the user application is simply calling > `getSubject(AccessController.getContext())`, then switching to `current()` > would work. If the `AccessControlContext` argument is retrieved from an > earlier `getContext()` call and the associated subject might be different > from that of the current `AccessControlContext`, then instead of storing the > previous `AccessControlContext` object and passing it into `getSubject` to > get the "previous" subject, the application should store the `current()` > return value directly. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: fix MBeanServerFileAccessController, more test in SM - Changes: - all: https://git.openjdk.org/jdk/pull/17472/files - new: https://git.openjdk.org/jdk/pull/17472/files/a16472fb..8f270d09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17472=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17472=01-02 Stats: 18 lines in 2 files changed: 8 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/17472.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17472/head:pull/17472 PR: https://git.openjdk.org/jdk/pull/17472
