Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove usages of classes associated with JEP-411 that deprecate/remove the SecurityManager from the JVM #6184

Closed
joakime opened this issue Apr 16, 2021 · 15 comments

Comments

@joakime
Copy link
Contributor

joakime commented Apr 16, 2021

Jetty version
All

Java version
All

OS type/version
All

Description
JEP-411 - https://openjdk.java.net/jeps/411

Will deprecate the SecurityManager in a specific way with an eye on removing it entirely in the future.
We should start to either alter our usages and expectations of the SecurityManager, or remove our usages entirely.

One of the techniques JEP-411 is considering is to have System.getSecurityManager() always return null.

That could impact our code when we use it like this ...

https://github.com/eclipse/jetty.project/blob/d825299da47a1d923648c52c80df11ac76fed77c/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java#L2529

Another aspect to consider is that AccessController.doPrivileged(...) will have no meaning anymore, the action will always be run in the JEP-411 deprecated mode.
Once JEP-411 enters removal mode, our code using AccessController will start to fail.

@sbordet
Copy link
Contributor

sbordet commented Jan 26, 2022

@janbartel please use this issue as an umbrella for other issues if you do any work related to the SecurityManager.

@janbartel
Copy link
Contributor

Even if we removed all calls to SecurityManager in jetty itself, it may still be present in 3rd party jars. For example, the JASPI api still refers to SecurityManager: https://github.com/jakartaee/authentication/blob/master/api/src/main/java/jakarta/security/auth/message/config/AuthConfigFactory.java#L157

@sbordet
Copy link
Contributor

sbordet commented Jan 27, 2022

@janbartel sure, but that's on them to fix JASPI.

@janbartel
Copy link
Contributor

@sbordet what I'm saying is that the JASPI api refers to SecurityExceptions that should be thrown if the caller does not have the correct permission. The api impl calls SecurityManager and expects that subclasses will do the same.

@joakime
Copy link
Contributor Author

joakime commented Jan 27, 2022

@janbartel I went ahead and opened jakartaee/authentication#138 about that specific case.

@joakime
Copy link
Contributor Author

joakime commented Jan 27, 2022

The Jakarta EE platform level is tracking this requirement as well.

jakartaee/platform#406

@janbartel
Copy link
Contributor

I think one of the biggest stumbling blocks for JEP-411 will be the fixes for #5859. The fix for the classloader pinning by the constructor for Thread requires the use of some java.security classes. We will need to support both pre-JEP-411 runtimes and post JEP-411 runtimes (possibly with reflection based off the runtime version?).

@joakime
Copy link
Contributor Author

joakime commented Feb 11, 2022

I opened PR #7562 for jetty-12.0.x branch.

The Classloader pinning issue needs to be solved without using the security classes.
Also, there are 3 levels of JEP-411 runtimes.

  1. pre JEP-411 - classes exist and function as expected.
  2. JEP-411 transition - classes exist, but are no-ops, and don't do anything.
  3. post JEP-411 - classes don't exist.

We cannot use reflection for level 2, so we need to solve the Classloader pinning issue a different way entirely.

@janbartel
Copy link
Contributor

@joakime that's easier said than done: we are at the mercy of the implementation of Thread as regards pinning of the classloader.

@joakime
Copy link
Contributor Author

joakime commented Feb 15, 2022

With the early-access releases of JDK 18 and JDK 19 now available.

We have a JVM runtime that's in stage 2 in my above list.

This means the SecurityManager and related classes are now a no-op.
You can't even start a JVM with a Security Manager.

Caused by: java.lang.UnsupportedOperationException: The Security Manager is deprecated and will be removed in a future release
	at java.base/java.lang.System.setSecurityManager(System.java:416)

The implementations of all of the JEP-411 specified classes now do nothing.
A future JEP will remove the classes.

@joakime
Copy link
Contributor Author

joakime commented Mar 23, 2022

Here's an example of how to use the JVM specific Privileged Thread Factory in our code.
Commit: 3c462c3

@janbartel
Copy link
Contributor

@joakime how can you be sure that the classloader is no longer pinned? Have you inspected the Thread class code? I'm fine to use the new jdk PrivilegedThreadFactory per se, however I want to be sure it is solving the problem this issue is about.

@joakime
Copy link
Contributor Author

joakime commented Jun 7, 2022

@janbartel doesn't matter if use the JDK PrivilegedThreadFactory either, as that is also covered by the removal in JEP-411.

@joakime
Copy link
Contributor Author

joakime commented Jun 7, 2022

The problem is that there's currently no solution that is resilient to changes in new JVMs.

The old Jetty PrivilegedThreadFactory will break because AccessController.doPrivileged will not exist.

And using the JDK PrivilegedThreadFactory will break because that wont be there either.

@joakime
Copy link
Contributor Author

joakime commented Jun 7, 2022

@joakime how can you be sure that the classloader is no longer pinned? Have you inspected the Thread class code?

I have inspected the code, with @sbordet too.
It's not obvious where this issue exists in newer JVMs. (we didn't look at older JVMs)

Currently we have no test that proves the pinning when not using the Jetty PrivilegedThreadFactory. (automatic, or manual)

sbordet added a commit that referenced this issue Apr 3, 2023
…the JVM.

Removed usages of `SecurityManager` and `AccessControlller.doPrivileged()`.
In places where they are still necessary, now using reflection.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet closed this as completed in 2c61011 Apr 6, 2023
@joakime joakime changed the title JEP-411 will deprecate/remove the SecurityManager from the JVM Remove classes associated with JEP-411 that deprecate/remove the SecurityManager from the JVM Apr 11, 2023
@joakime joakime changed the title Remove classes associated with JEP-411 that deprecate/remove the SecurityManager from the JVM Remove usages of classes associated with JEP-411 that deprecate/remove the SecurityManager from the JVM Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants