jdk icon indicating copy to clipboard operation
jdk copied to clipboard

8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs

Open wangweij opened this issue 1 year ago • 14 comments

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.


Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue
  • [ ] Change requires CSR request JDK-8327134 to be approved

Issues

  • JDK-8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs (Enhancement - P3)
  • JDK-8327134: Alternate implementation of user-based authorization Subject APIs that doesn't depend on Security Manager APIs (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17472/head:pull/17472
$ git checkout pull/17472

Update a local copy of the PR:
$ git checkout pull/17472
$ git pull https://git.openjdk.org/jdk.git pull/17472/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17472

View PR using the GUI difftool:
$ git pr show -t 17472

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17472.diff

Webrev

Link to Webrev Comment

wangweij avatar Jan 17 '24 23:01 wangweij

:wave: Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Jan 17 '24 23:01 bridgekeeper[bot]

/csr

wangweij avatar Jan 17 '24 23:01 wangweij

@wangweij has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@wangweij please create a CSR request for issue JDK-8296244 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk[bot] avatar Jan 17 '24 23:01 openjdk[bot]

@wangweij The following labels will be automatically applied to this pull request:

  • core-libs
  • jmx
  • security
  • serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

openjdk[bot] avatar Jan 17 '24 23:01 openjdk[bot]

@wangweij This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Mar 01 '24 04:03 bridgekeeper[bot]

Mailing list message from Peter Firmstone on security-dev:

Good Evening,

Just noticed the comment below, this is a breaking change.

Recalling earlier discussions on this list about the removal of the existing Authorization API post JEP411, it was going to be assigned another overarching JEP.

Can we have the entire API destructed in one swift action? That is, all API marked for removal under JEP411 should now throw UnsupportedOperationException? Keeping the API around as unsupported operation would also allow us to maintain a fork where the API remains functional, without breaking compile time compatibility with Java, while we figure out how to migrate our software over the longer term.

Thank you for retaining Java's Authorization API in Java 21 LTS.

Regards,

Peter.

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.

-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20240307/570d169d/attachment.htm>

mlbridge[bot] avatar Mar 07 '24 18:03 mlbridge[bot]

Mailing list message from Sean Mullan on security-dev:

On 3/7/24 7:35 AM, Peter Firmstone wrote:

Good Evening,

Just noticed the comment below, this is a breaking change.

Recalling earlier discussions on this list about the removal of the existing Authorization API post JEP411, it was going to be assigned another overarching JEP.

Yes that JEP is eventually coming in the near future.

Can we have the entire API destructed in one swift action? That is, all API marked for removal under JEP411 should now throw UnsupportedOperationException? Keeping the API around as unsupported operation would also allow us to maintain a fork where the API remains functional, without breaking compile time compatibility with Java, while we figure out how to migrate our software over the longer term.

The next major step is to remove support for the Security Manager (SM) from the JDK. However, the deprecated for removal APIs will not be removed as part of that - they will be retained and removed at some later time. Some APIs will be degraded to throw UnsupportedOperationException but others (such as AccessController.doPrivileged) will be degraded to behave as if an SM had not been enabled.

JAAS is a special case in that it has dependencies on the deprecated SM APIs for uses cases that don't always require the SM to be enabled. Thus, as described in JEP 411, we introduced new replacement APIs (Subject.callAs and Subject.current) in JDK 18 (4 releases ago). Applications depending on JAAS for authentication or authorization cases that do not require an SM should be migrating to these replacement APIs.

--Sean

Thank you for retaining Java's Authorization API in Java 21 LTS.

Regards,

Peter.

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.

mlbridge[bot] avatar Mar 07 '24 18:03 mlbridge[bot]

Mailing list message from Peter Firmstone on security-dev:

Sent from my iPhone

On 8 Mar 2024, at 1:35?am, Sean Mullan <sean.mullan at oracle.com> wrote:

?On 3/7/24 7:35 AM, Peter Firmstone wrote:

Good Evening, Just noticed the comment below, this is a breaking change. Recalling earlier discussions on this list about the removal of the existing Authorization API post JEP411, it was going to be assigned another overarching JEP.

Yes that JEP is eventually coming in the near future.

Can we have the entire API destructed in one swift action? That is, all API marked for removal under JEP411 should now throw UnsupportedOperationException? Keeping the API around as unsupported operation would also allow us to maintain a fork where the API remains functional, without breaking compile time compatibility with Java, while we figure out how to migrate our software over the longer term.

The next major step is to remove support for the Security Manager (SM) from the JDK. However, the deprecated for removal APIs will not be removed as part of that - they will be retained and removed at some later time. Some APIs will be degraded to throw UnsupportedOperationException but others (such as AccessController.doPrivileged) will be degraded to behave as if an SM had not been enabled.

JAAS is a special case in that it has dependencies on the deprecated SM APIs for uses cases that don't always require the SM to be enabled. Thus, as described in JEP 411, we introduced new replacement APIs (Subject.callAs and Subject.current) in JDK 18 (4 releases ago). Applications depending on JAAS for authentication or authorization cases that do not require an SM should be migrating to these replacement APIs themselves.

--Sean

Should, when possible. Our software is deeply integrated with the deprecated Authorization API, we've determined it's not possible. Our only choice is to fork, so I'd definitely prefer the API to remain. Also if functionality can be removed rather than degraded, that will make it easier to patch in replacement functionality.

Thank you,

Peter.

Thank you for retaining Java's Authorization API in Java 21 LTS. Regards, Peter.

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.

mlbridge[bot] avatar Mar 08 '24 07:03 mlbridge[bot]

Mailing list message from Peter Firmstone on security-dev:

Can this change be documented along with others planned in the new JEP so these changes are easier to track?

There's no mention of this change in the bug id.

We've placed all future development on hold in software that depends on Java's Authorization API, (apart from bug fixes), while we figure out how to manage changes brought about by JEP411.

Thank you.

Peter.

On 8/03/2024 8:31 am, Peter Firmstone wrote:

Sent from my iPhone

On 8 Mar 2024, at 1:35?am, Sean Mullan <sean.mullan at oracle.com> wrote:

?On 3/7/24 7:35 AM, Peter Firmstone wrote:

Good Evening, Just noticed the comment below, this is a breaking change. Recalling earlier discussions on this list about the removal of the existing Authorization API post JEP411, it was going to be assigned another overarching JEP. Yes that JEP is eventually coming in the near future.

Can we have the entire API destructed in one swift action? That is, all API marked for removal under JEP411 should now throw UnsupportedOperationException? Keeping the API around as unsupported operation would also allow us to maintain a fork where the API remains functional, without breaking compile time compatibility with Java, while we figure out how to migrate our software over the longer term. The next major step is to remove support for the Security Manager (SM) from the JDK. However, the deprecated for removal APIs will not be removed as part of that - they will be retained and removed at some later time. Some APIs will be degraded to throw UnsupportedOperationException but others (such as AccessController.doPrivileged) will be degraded to behave as if an SM had not been enabled.

JAAS is a special case in that it has dependencies on the deprecated SM APIs for uses cases that don't always require the SM to be enabled. Thus, as described in JEP 411, we introduced new replacement APIs (Subject.callAs and Subject.current) in JDK 18 (4 releases ago). Applications depending on JAAS for authentication or authorization cases that do not require an SM should be migrating to these replacement APIs themselves.

--Sean Should, when possible. Our software is deeply integrated with the deprecated Authorization API, we've determined it's not possible. Our only choice is to fork, so I'd definitely prefer the API to remain. Also if functionality can be removed rather than degraded, that will make it easier to patch in replacement functionality.

Thank you,

Peter.

Thank you for retaining Java's Authorization API in Java 21 LTS. Regards, Peter.

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.

mlbridge[bot] avatar Mar 08 '24 07:03 mlbridge[bot]

Mailing list message from Alan Bateman on security-dev:

On 07/03/2024 23:30, Peter Firmstone wrote:

Can this change be documented along with others planned in the new JEP so these changes are easier to track?

As Sean said, JEP 411 already links to JDK-8267108 that introduced Subject.current/callAs methods as the replacements for getSubject/doAs.

But yes, the future JEP for removal of the security manager will likely cover many areas, including the changes to Subject that are in the API for some time.

-Alan

mlbridge[bot] avatar Mar 08 '24 14:03 mlbridge[bot]

Mailing list message from Peter Firmstone on security-dev:

JDK-8267108 doesn't mention any existing getSubject/doAs methods will be changed to throw UnsupportedOperationException, would be great if that can be documented as well, if I hadn't seen the change in the mailing list I would have missed it.?? Just helps track related changes.

Thanks,

Peter.

On 8/03/2024 5:52 pm, Alan Bateman wrote:

On 07/03/2024 23:30, Peter Firmstone wrote:

Can this change be documented along with others planned in the new JEP so these changes are easier to track?

As Sean said, JEP 411 already links to JDK-8267108 that introduced Subject.current/callAs methods as the replacements for getSubject/doAs.

But yes, the future JEP for removal of the security manager will likely cover many areas, including the changes to Subject that are in the API for some time.

-Alan

mlbridge[bot] avatar Mar 08 '24 14:03 mlbridge[bot]

Mailing list message from Alan Bateman on security-dev:

On 08/03/2024 08:20, Peter Firmstone wrote:

JDK-8267108 doesn't mention any existing getSubject/doAs methods will be changed to throw UnsupportedOperationException, would be great if that can be documented as well, if I hadn't seen the change in the mailing list I would have missed it.?? Just helps track related changes.

The warning at compile time that the API is deprecated for removal should help when compiling with recent JDK releases.

When JDK-8267108 introduced new/replacement APIs then the details/time-line for when the old/problematic APIs would be degraded was not known.

JDK 23 (or whatever release the current change goes into) will have documentation on the change in the release.

-Alan

mlbridge[bot] avatar Mar 08 '24 14:03 mlbridge[bot]

@wangweij This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs

Reviewed-by: mullan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 13 new commits pushed to the master branch:

  • 000f4d8d156a48939fd29f8b4dd84b3dfd7d9d95: 8319251: [REDO] Change LockingMode default from LM_LEGACY to LM_LIGHTWEIGHT
  • fbeac98c84078a566c572abeba07c49e94bbf26b: 8328379: Convert URLDragTest.html applet test to main
  • 96530bcc07514c3eda40fd6ffa74f197fe541dea: 8326521: JFR: CompilerPhase event test fails on windows 32 bit
  • a112fc8bac8ddee87c8555b156519a2785d3223b: 8328225: Convert ImageDecoratedDnD.html applet test to main
  • 4e83f4cfc779e39cca0070b5729a508aeaa74654: 8328185: Convert java/awt/image/MemoryLeakTest/MemoryLeakTest.java applet test to main
  • d3f3011d56267360d65841da3550eca79cf1575b: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream
  • e5e7cd20eca0e5a5f0811d304a9659961dcf11c0: 8328386: Convert java/awt/FileDialog/FileNameOverrideTest test to main
  • 1b68c731f2461a2f26e6d967dd3f94e217b4d1f4: 8328377: Convert java/awt/Cursor/MultiResolutionCursorTest test to main
  • e0373e01fece310d12859207cc5e233f68b7607f: 8328378: Convert java/awt/FileDialog/FileDialogForDirectories test to main
  • 03c25b15eb73e594d1329be397cd34e206d2682b: 8328367: Convert java/awt/Component/UpdatingBootTime test to main
  • ... and 3 more: https://git.openjdk.org/jdk/compare/7231fd78aada80946b680e10227a356714d5c34b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk[bot] avatar Mar 13 '24 20:03 openjdk[bot]

There is no source code change to java.management anymore inside this PR. They will be resolved with new bugs at JDK-8327618 and JDK-8328263. There are test changes in these areas in this PR to force them running with SM allowed. Ideally, these changes can be reverted when the 2 new bugs are resolved.

wangweij avatar Mar 20 '24 14:03 wangweij

/integrate

wangweij avatar Mar 20 '24 21:03 wangweij

Going to push as commit d32746ef4a0ce6fec558274244321991be141698. Since your change was applied there have been 13 commits pushed to the master branch:

  • 000f4d8d156a48939fd29f8b4dd84b3dfd7d9d95: 8319251: [REDO] Change LockingMode default from LM_LEGACY to LM_LIGHTWEIGHT
  • fbeac98c84078a566c572abeba07c49e94bbf26b: 8328379: Convert URLDragTest.html applet test to main
  • 96530bcc07514c3eda40fd6ffa74f197fe541dea: 8326521: JFR: CompilerPhase event test fails on windows 32 bit
  • a112fc8bac8ddee87c8555b156519a2785d3223b: 8328225: Convert ImageDecoratedDnD.html applet test to main
  • 4e83f4cfc779e39cca0070b5729a508aeaa74654: 8328185: Convert java/awt/image/MemoryLeakTest/MemoryLeakTest.java applet test to main
  • d3f3011d56267360d65841da3550eca79cf1575b: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream
  • e5e7cd20eca0e5a5f0811d304a9659961dcf11c0: 8328386: Convert java/awt/FileDialog/FileNameOverrideTest test to main
  • 1b68c731f2461a2f26e6d967dd3f94e217b4d1f4: 8328377: Convert java/awt/Cursor/MultiResolutionCursorTest test to main
  • e0373e01fece310d12859207cc5e233f68b7607f: 8328378: Convert java/awt/FileDialog/FileDialogForDirectories test to main
  • 03c25b15eb73e594d1329be397cd34e206d2682b: 8328367: Convert java/awt/Component/UpdatingBootTime test to main
  • ... and 3 more: https://git.openjdk.org/jdk/compare/7231fd78aada80946b680e10227a356714d5c34b...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Mar 20 '24 21:03 openjdk[bot]

@wangweij Pushed as commit d32746ef4a0ce6fec558274244321991be141698.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk[bot] avatar Mar 20 '24 21:03 openjdk[bot]