SecurityUtils should not elminate calls to existing methods

[stoty]

Jetty version(s)
12.1.x

Jetty Environment
any

Java version/vendor (use: java -version)
any

OS type/version
any

Description
When "org.eclipse.jetty.util.security.useSecurityManager" is set to false, or JDK21 or later is used, then SecurityUtils treats doAs() callAs() as NOOP.

This is wrong, as both methods also perform functions unrelated to the SecurityManager (i.e. setting the subject).

The doPrivileged elimiation also seems bad , I suspect that it will lead to leaks if the SecurityManager is enabled.

In fact, I think that SecurityUtils should not try to guess the SecurityManager setting at all, but just call existing APIs if the methods exist.

The JVM will figure out the rest, and neither Jetty, nor its users have to worry about handling all the JVM/securityManager setting combinations.

How to reproduce?

[stoty]

Please take a look, @gregw .

[joakime]

The doPrivileged elimiation also seems bad , I suspect that it will lead to leaks if the SecurityManager is enabled.

This wholesale removal of SecurityManager is ongoing everywhere, as the SecurityManager has been flagged as deprecated and is scheduled for removal from the JVM itself.
Most of the Jakarta EE specs have already done this. (this effort will be considered complete by ee12)
Most of our 3rd party libraries have already done this as well.

[joakime]

More information on the Jakarta EE side.

• Remove usage and references to SecurityManager jakartaee/security#301
• Remove references to SecurityManager as per #301 jakartaee/security#302
• Fix #443 Remove references to the SecurityManager and associated APIs jakartaee/servlet#495

etc ...

[stoty]

This is a simple correctness issue.

These methods are called from a few places, but there is a reason for each call, and at least the callAs() and doPrivileged() calls are NOT for using the removed SecurityManager functionality.

callAs() is NOT scheduled to be removed, and this new method was specifically added to Java to avoid the SecurityManager dependecy of doAs() , so not calling it simply breaks any code depending on the java security framework for handling users, including Kerberos/SPENGO support in Jetty.

Gating callAs() behind some arbitray parameter is a glaringly obvious copy-paste bug.

doPrivileged() is used in one place to avoid a resources leak in the code.
If the application pulling iun Jetty re-enables securityManager, and Jetty eliminates the doPrivilieged() call, Jetty will leak resources (at least I expect that re-enabling securityManager re-enables the resource leak that adding the call was originally meant to fix).

doPriviliged() is a but more subtle bug, because in this case Jetty was never using it for SecurityManager related functionality, but for avoiding leaks via Classloaders when forking Threads.

Frankly, I did not dig into the Java EE context stuff, but I would expect that there are cases where the getSecurityManager() elimination also breaks uses cases that currently work, especially since most of those Java EE releases were written with the assumption of SecurityManager funtionalty, and may still be used by code that really does use SecurityManager functionality.

[stoty]

Specifically, for JDK 17 the code won't find the new callAs() method, because it was added in 18, and then won't even try the new API if the jetty property is set to false, even though the doAs()/callAs() functionaly is only marginally related to SecurityManager.

The only saving grace in that case is that for JDK21 and earlier Jetty defaults to using the old API, so this won't break on JDK17, unless someone explicitly sets the property to the wrong value.

[joakime]

doPrivileged() is used in one place to avoid a resources leak in the code. If the application pulling iun Jetty re-enables securityManager, and Jetty eliminates the doPrivilieged() call, Jetty will leak resources (at least I expect that re-enabling securityManager re-enables the resource leak that adding the call was originally meant to fix).

doPriviliged() is a but more subtle bug, because in this case Jetty was never using it for SecurityManager related functionality, but for avoiding leaks via Classloaders when forking Threads.

This leak is JDK version specific.
OpenJDK 17, and OpenJDK 21 do not exhibit this leak.

If you were running JDK 11, yes, this bug is very important to address.
The JDK recommended fix for this was to use Executors.privilegedThreadFactory() btw, not a SecurityManager directly.

In the OpenJDK 17 codebase, the AccessControlContext changed with how it is associated with the Class and Thread (the context exists, but is null). (this is the source of the leak you referenced)
This was done at the same time as Executors.privilegedThreadFactory() was deprecated.

Our current codebase, in Jetty 12, uses reflection against deprecated java.security.AccessController to find a doPrivileged method to call when creating a new Thread if that class and method exists, otherwise it creates the thread directly. (still no direct SecurityManager class use for that in Jetty).

[joakime]

These methods are called from a few places, but there is a reason for each call, and at least the callAs() and doPrivileged() calls are NOT for using the removed SecurityManager functionality.

callAs() is NOT scheduled to be removed, and this new method was specifically added to Java to avoid the SecurityManager dependecy of doAs() , so not calling it simply breaks any code depending on the java security framework for handling users, including Kerberos/SPENGO support in Jetty.

The two methods you referenced, callAs() and doAs() are new to JDK 18.

• https://docs.oracle.com/en/java/javase/21/docs/api/java.base/javax/security/auth/Subject.html#callAs(javax.security.auth.Subject,java.util.concurrent.Callable)
• https://docs.oracle.com/en/java/javase/21/docs/api/java.base/javax/security/auth/Subject.html#doAs(javax.security.auth.Subject,java.security.PrivilegedAction)

I find it entertaining that all of the doAs methods were added in JDK18 and immediately deprecated for removal.

We should probably have a Jetty SecurityUtils.callAs() method option.
I wonder how hard it would be to move usages of SecurityUtils.doAs() to this new non-deprecated javax.security.auth.Subject.callAs(Subject, Callable) replacement?
It uses `

And deprecate the SecurityUtils.doAs() method direct call.

[stoty]

We should probably have a Jetty SecurityUtils.callAs() method option.
I wonder how hard it would be to move usages of SecurityUtils.doAs() to this new non-deprecated javax.security.auth.Subject.callAs(Subject, Callable) replacement?
It uses `

And deprecate the SecurityUtils.doAs() method direct call.

This has already been done in 12.1.x (and it was a good change)

[stoty]

However, Subject.doAs() is so old, that I gave up trying to figure when it was added.
It has certainly been present in Java 7 :

https://docs.oracle.com/javase/7/docs/api/javax/security/auth/Subject.html#doAs(javax.security.auth.Subject,%20java.security.PrivilegedAction)

[stoty]

Regarding doPriviliged()

Now that we are talking about it, I remember how it broke SPNEGO support in Avatica when Jetty added it.
I had to add a new ThreadFactory that saved and reset the Subject because doPrivileged() has reset it.

Coincidently, this is another good example of how eliminating the doPrivileged() call for JDK22 and later causes (another) incompatible change in Jetty.

before the hack, Jetty did not change Subject.
after the hack, Jetty did reset the Subject.
With this new change, Jetty would not reset the Subject again.

[stoty]

Because you referred 12.0 behaviour, I want to stress that this problem only exists in 12.1.x and the problematic code was added qute recently in:

12db285

What I propose is basically reverting to the 12.0 behaviour.