-
Notifications
You must be signed in to change notification settings - Fork 36
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 SecurityManager #248
base: master
Are you sure you want to change the base?
Conversation
523ff10
to
0f801bc
Compare
An example warning is
|
Currently, there's only one open source example of this feature being used: https://github.com/search?q=%3CallowSystemExits%3Efalse%3C%2FallowSystemExits%3E+language%3A%22Maven+POM%22&type=code&l=Maven+POM |
/** | ||
* Whether to allow System.exit() to be used. Should not be set to <code>false</code> when using parallel | ||
* execution, as it isn't thread-safe. | ||
* | ||
* @since 1.2 | ||
*/ | ||
@Parameter(defaultValue = "true") | ||
protected boolean allowSystemExits; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this would be a breaking change, keep it for a while and just print a warning for now. This way, users can fix their scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that's why I hadn't merged this -- Java isn't forcing me to yet.
Another option here might be to wrap the usage in if (Version.parseFromString(System.getProperty("java.version")).compareTo(new Version(24)) >=1)
and print a warning if using a newer Java version that won't support it and ignore the option in that case. And in the meantime, print a warning and deprecate the parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure when they're planning on removing this (I used 24 as an example). I don't think they've decided when that will be (or at least I can't find any documentation about it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it gets removed. The warning is there since 17 or 18.
Docs: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/SecurityManager.html
@Deprecated(since="17",
forRemoval=true)
public class SecurityManager
extends Object
Deprecated, for removal: This API element is subject to removal in a future version.
The Security Manager is deprecated and subject to removal in a future release. There is no replacement for the Security Manager. See JEP 411 for discussion and alternatives.
There's more
https://openjdk.org/jeps/411
Description
In Java 17, we will:
Deprecate, for removal, most Security Manager related classes and methods.
Issue a warning message at startup if the Security Manager is enabled on the command line.
Issue a warning message at run time if a Java application or library installs a Security Manager dynamically.
In Java 18, we will prevent a Java application or library from dynamically installing a Security Manager unless the end user has explicitly opted to allow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option here might be to wrap the usage in if (Version.parseFromString(System.getProperty("java.version")).compareTo(new Version(24)) >=1)
But that would still print a ClassNotFoundException on 24 or so. Unless:
- You use reflection. A lot.
- Create a multi release jar
- Drop it all together and print a warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of using reflection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather put that code into its own class.
Then, for Java 21+, create a Multi Release Jar which implements this as a no-op.
Easier to maintain, faster than reflection.
But still, why not start with a warning? 🤷🏻♂️ Ship early, ship often.
if (!allowSystemExits) { | ||
System.setSecurityManager(new NoExitSecurityManager()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print a warning in case this is false
.
if (!allowSystemExits) { | ||
System.setSecurityManager(new NoExitSecurityManager()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, keep var and print warning (to be removed in…)
if (!allowSystemExits) { | ||
System.setSecurityManager(new NoExitSecurityManager()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
Kind of fixes #286 but let me make another suggestion |
Created #289 for printing a warning about the SystemManager deprecation |
JEP 411 deprecated SecurityManager in Java 17, for future removal. It is unclear what it will be replaced with for the use case of preventing
System.exit
usages. JDK-8199704 is one possibility, but it's unclear how likely that is to be implemented.We should prepare for it's removal by removing our dependency on it. This is a breaking change.