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 SecurityManager #248

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove SecurityManager #248

wants to merge 1 commit into from

Conversation

keeganwitt
Copy link
Member

@keeganwitt keeganwitt commented May 23, 2023

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.

@keeganwitt
Copy link
Member Author

keeganwitt commented May 23, 2023

An example warning is

[ERROR] WARNING: A terminally deprecated method in java.lang.System has been called
[ERROR] WARNING: System::setSecurityManager has been called by org.codehaus.gmavenplus.mojo.ExecuteMojo (file:/C:/Users/keega/Documents/GitHub/GMavenPlus/target/classes/)
[ERROR] WARNING: Please consider reporting this to the maintainers of org.codehaus.gmavenplus.mojo.ExecuteMojo
[ERROR] WARNING: System::setSecurityManager will be removed in a future release

@keeganwitt
Copy link
Member Author

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

Comment on lines -73 to -81
/**
* 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;

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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).

Copy link
Contributor

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.

Copy link
Contributor

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:

  1. You use reflection. A lot.
  2. Create a multi release jar
  3. Drop it all together and print a warning

Copy link
Member Author

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

Copy link
Contributor

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.

Comment on lines -91 to -93
if (!allowSystemExits) {
System.setSecurityManager(new NoExitSecurityManager());
}
Copy link
Contributor

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.

Comment on lines -142 to -144
if (!allowSystemExits) {
System.setSecurityManager(new NoExitSecurityManager());
}
Copy link
Contributor

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…)

Comment on lines -96 to -98
if (!allowSystemExits) {
System.setSecurityManager(new NoExitSecurityManager());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

@bmarwell
Copy link
Contributor

Kind of fixes #286 but let me make another suggestion

@keeganwitt
Copy link
Member Author

Created #289 for printing a warning about the SystemManager deprecation

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

Successfully merging this pull request may close these issues.

None yet

2 participants