-
Notifications
You must be signed in to change notification settings - Fork 725
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
JDK24: Permanently Disable the Security Manager #20625
base: master
Are you sure you want to change the base?
Conversation
<disables> | ||
<disable> | ||
<comment>https://github.com/eclipse-openj9/openj9/issues/20563</comment> | ||
<version>24+</version> |
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.
@llxia is this the preferred way to disable tests from running in future versions? The security tests should run for JDK 11-23 only. I've mostly seen the block used in temporarily disabled tests so I wanted to check since this would be a permanent change.
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.
disable
is for temporary excludes. In this case, we should set <version>[11, 23]</version>
. Example code: https://github.com/adoptium/TKG/blob/master/examples/jdkVersion/playlist.xml#L39
888855d
to
7ab4eda
Compare
@JasonFengJ9 Please review these changes |
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.
The natives related to checkPermission()
can be ifdef
out for JDK24+ such as
openj9/runtime/jcl/common/java_lang_Class.cpp
Line 1406 in 48709bf
Java_java_security_AccessController_getAccSnapshot(JNIEnv* env, jclass jsAccessController, jint startingFrame, jboolean forDoPrivilegedWithCombiner) |
@@ -1265,6 +1265,10 @@ static void checkTmpDir() { | |||
|
|||
/*[IF JAVA_SPEC_VERSION >= 9]*/ | |||
static void initSecurityManager(ClassLoader applicationClassLoader) { |
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.
initSecurityManager()
can be removed at
System.initSecurityManager(applicationClassLoader); |
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.
This is still needed since initSecurityManager
is used to detect settings of the java.security.manager
property.
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.
initSecurityManager()
reads the system property java.security.manager
, and sets throwUOEFromSetSM
which can be skipped within setSecurityManager()
.
System.initSecurityManager(applicationClassLoader)
seems not needed for JDK24+.
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.
It will still be needed to throw an exception on startup for illegal java.security.manager manager settings triggered by throwErrorOnInit
.
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.
static void initSecurityManager(ClassLoader applicationClassLoader) { | |
static void initSecurityManager(ClassLoader applicationClassLoader) { | |
String javaSecurityManager = internalGetProperties().getProperty("java.security.manager"); //$NON-NLS-1$ | |
if (null == javaSecurityManager) { | |
/*[IF JAVA_SPEC_VERSION >= 18]*/ | |
throwUOEFromSetSM = true; | |
/*[ELSE] JAVA_SPEC_VERSION >= 18 */ | |
/* Do nothing. */ | |
/*[ENDIF] JAVA_SPEC_VERSION >= 18 */ | |
} else if ("disallow".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
/*[IF JAVA_SPEC_VERSION > 11]*/ | |
throwUOEFromSetSM = true; | |
/*[ELSE] JAVA_SPEC_VERSION > 11 */ | |
/* Do nothing. */ | |
/*[ENDIF] JAVA_SPEC_VERSION > 11 */ | |
} else { | |
/*[IF JAVA_SPEC_VERSION >= 24]*/ | |
/*[MSG "K0B04", "A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported."]*/ | |
throw new Error(Msg.getString("K0B04")); //$NON-NLS-1$ | |
/*[ELSE] JAVA_SPEC_VERSION >= 24 */ | |
if ("allow".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
/* Do nothing. */ | |
} else { | |
/*[IF JAVA_SPEC_VERSION >= 17]*/ | |
initialErr.println("WARNING: A command line option has enabled the Security Manager"); //$NON-NLS-1$ | |
initialErr.println("WARNING: The Security Manager is deprecated and will be removed in a future release"); //$NON-NLS-1$ | |
/*[ENDIF] JAVA_SPEC_VERSION >= 17 */ | |
if (javaSecurityManager.isEmpty() || "default".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
setSecurityManager(new SecurityManager()); | |
} else { | |
try { | |
Constructor<?> constructor = Class.forName(javaSecurityManager, true, applicationClassLoader).getConstructor(); | |
constructor.setAccessible(true); | |
setSecurityManager((SecurityManager)constructor.newInstance()); | |
} catch (Throwable e) { | |
/*[MSG "K0631", "JVM can't set custom SecurityManager due to {0}"]*/ | |
throw new Error(Msg.getString("K0631", e.toString()), e); //$NON-NLS-1$ | |
} | |
} | |
} | |
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */ | |
} | |
} |
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.
Can you clarify the benefits of this approach over the existing change?
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.
For readability, the proposed change has 4 pairs of /*[IF JAVA_SPEC_VERSION >= 24]*/
and an unnecessary local variable throwErrorOnInit
.
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 think what we have is correct. This could be refactored to eliminate that local variable, but I think that should be done separately (if at all), and I can see ways forward that are even better.
jcl/src/java.base/share/classes/java/security/AccessControlContext.java
Outdated
Show resolved
Hide resolved
jcl/src/java.base/share/classes/java/security/AccessController.java
Outdated
Show resolved
Hide resolved
.../functional/cmdLineTests/shareClassTests/DataHelperTests/DataHelperTests_SecurityManager.xml
Outdated
Show resolved
Hide resolved
test/functional/cmdLineTests/shareClassTests/DataHelperTests/playlist.xml
Outdated
Show resolved
Hide resolved
test/functional/Java8andUp/src/org/openj9/test/attachAPI/TestAttachAPI.java
Outdated
Show resolved
Hide resolved
FYI #20655 |
fce9383
to
fc9e6eb
Compare
jcl/src/java.base/share/classes/com/ibm/oti/util/ExternalMessages-MasterIndex.properties
Outdated
Show resolved
Hide resolved
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.
In addition, all AccessController.doPrivileged_XXX
usages can be removed for JDK24+ such as
static final boolean ENABLED = AccessController.doPrivileged(new PrivilegedAction<Boolean>() { |
Yeah, there are lots of them.
@@ -49,25 +51,25 @@ public final class AccessController { | |||
initializeInternal(); | |||
} | |||
|
|||
private static native void initializeInternal(); |
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.
This native can be removed for JDK24+, I expect J9JavaVM->doPrivilegedMethodID_XXX
are not needed along with
openj9/runtime/jcl/common/acccont.c
Line 28 in 3da9d2f
jboolean JNICALL Java_java_security_AccessController_initializeInternal(JNIEnv *env, jclass thisClz) |
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 have excluded these and am running a personal build to see if there are any impacted tests.
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 have excluded and opened an issue for the one failure I found #20702.
72168cc
to
c9cabc6
Compare
Do you mind if I do this in a second pull request? This change set is already getting large. |
Sounds good. |
@@ -1265,6 +1265,10 @@ static void checkTmpDir() { | |||
|
|||
/*[IF JAVA_SPEC_VERSION >= 9]*/ | |||
static void initSecurityManager(ClassLoader applicationClassLoader) { |
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.
static void initSecurityManager(ClassLoader applicationClassLoader) { | |
static void initSecurityManager(ClassLoader applicationClassLoader) { | |
String javaSecurityManager = internalGetProperties().getProperty("java.security.manager"); //$NON-NLS-1$ | |
if (null == javaSecurityManager) { | |
/*[IF JAVA_SPEC_VERSION >= 18]*/ | |
throwUOEFromSetSM = true; | |
/*[ELSE] JAVA_SPEC_VERSION >= 18 */ | |
/* Do nothing. */ | |
/*[ENDIF] JAVA_SPEC_VERSION >= 18 */ | |
} else if ("disallow".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
/*[IF JAVA_SPEC_VERSION > 11]*/ | |
throwUOEFromSetSM = true; | |
/*[ELSE] JAVA_SPEC_VERSION > 11 */ | |
/* Do nothing. */ | |
/*[ENDIF] JAVA_SPEC_VERSION > 11 */ | |
} else { | |
/*[IF JAVA_SPEC_VERSION >= 24]*/ | |
/*[MSG "K0B04", "A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported."]*/ | |
throw new Error(Msg.getString("K0B04")); //$NON-NLS-1$ | |
/*[ELSE] JAVA_SPEC_VERSION >= 24 */ | |
if ("allow".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
/* Do nothing. */ | |
} else { | |
/*[IF JAVA_SPEC_VERSION >= 17]*/ | |
initialErr.println("WARNING: A command line option has enabled the Security Manager"); //$NON-NLS-1$ | |
initialErr.println("WARNING: The Security Manager is deprecated and will be removed in a future release"); //$NON-NLS-1$ | |
/*[ENDIF] JAVA_SPEC_VERSION >= 17 */ | |
if (javaSecurityManager.isEmpty() || "default".equals(javaSecurityManager)) { //$NON-NLS-1$ | |
setSecurityManager(new SecurityManager()); | |
} else { | |
try { | |
Constructor<?> constructor = Class.forName(javaSecurityManager, true, applicationClassLoader).getConstructor(); | |
constructor.setAccessible(true); | |
setSecurityManager((SecurityManager)constructor.newInstance()); | |
} catch (Throwable e) { | |
/*[MSG "K0631", "JVM can't set custom SecurityManager due to {0}"]*/ | |
throw new Error(Msg.getString("K0631", e.toString()), e); //$NON-NLS-1$ | |
} | |
} | |
} | |
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */ | |
} | |
} |
@@ -637,6 +651,7 @@ private static int getNewAuthorizedState(AccessControlContext acc, ProtectionDom | |||
} | |||
return newAuthorizedState; | |||
} | |||
/*[ENDIF] JAVA_SPEC_VERSION < 24 */ |
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.
This can be moved to L722 to include toArrayOfProtectionDomains()
.
checkPermsNPE(perms); | ||
/*[ENDIF] JAVA_SPEC_VERSION < 24 */ |
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.
keepalive()
can be removed for JDK24+.
@@ -55,10 +55,12 @@ typedef enum { | |||
#define STACK_WALK_STATE_LIMITED_DOPRIVILEGED (void *)2 | |||
#define STACK_WALK_STATE_FULL_DOPRIVILEGED (void *)3 | |||
|
|||
#if JAVA_SPEC_VERSION < 24 |
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.
ObjsArraySizeNindex
, STACK_WALK_STATE_LIMITED_DOPRIVILEGED
and STACK_WALK_STATE_FULL_DOPRIVILEGED
can be removed for JDK24+.
ac1f215
to
4f82040
Compare
jcl/src/java.base/share/classes/java/security/AccessController.java
Outdated
Show resolved
Hide resolved
jcl/src/java.base/share/classes/java/security/AccessController.java
Outdated
Show resolved
Hide resolved
jcl/src/java.base/share/classes/java/security/AccessController.java
Outdated
Show resolved
Hide resolved
.../functional/cmdLineTests/shareClassTests/DataHelperTests/DataHelperTests_SecurityManager.xml
Outdated
Show resolved
Hide resolved
.../functional/cmdLineTests/shareClassTests/DataHelperTests/DataHelperTests_SecurityManager.xml
Outdated
Show resolved
Hide resolved
25b945a
to
f876cb1
Compare
Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
to a different job and exclude from 24+. Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
Signed-off-by: Theresa Mammarella <[email protected]>
- Throw an error on initialization if java.security.manager attempts to add a security manager - configure setSecurityManager to always throw an UnsupportedOperationException - getSecurityManager will always return null since a security manager can't be set Signed-off-by: Theresa Mammarella <[email protected]>
... for further investigation. Signed-off-by: Theresa Mammarella <[email protected]>
Exclude helper methods related to removing the security manager including: - AccessController.initializeInternal native in acccont.c - native helper methods in java_lang_class - doPriviled*id fields from J9JavaVM struct - unused helper methods in java.security.AccessController - unused helper methods in java.security.AccessControlContext - unused helper methods in java.lang.Class java.security comment update and exclude helper methods Signed-off-by: Theresa Mammarella <[email protected]>
Jenkins test sanity amac jdk21,jdknext |
This test is failing for the head stream:
|
The sanity.openjdk failure needs to be addressed:
Either OpenJ9 needs to change so the error is thrown from |
Perhaps it's better to update /*[IF JAVA_SPEC_VERSION >= 24]*/
initPhase3(throwErrorOnInit);
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */
}
/*[IF JAVA_SPEC_VERSION >= 24]*/
private static void initPhase3(boolean throwError) {
if (throwError) {
/*[MSG "K0B04", "A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported."]*/
throw new Error(Msg.getString("K0B04")); //$NON-NLS-1$
}
}
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */ @pshipton Do you have an opinion? |
Or the method |
For this one the System.security field has been fully removed in OpenJDK's implementation so it has been removed from Reflection.fieldFilterMap. Should I also try to remove the field entirely or update fieldFilterMap to disallow accessing the field? |
No, otherwise if (VersionCheck.major() >= 24) {
// The remaining tests don't apply to Java 24+.
return;
} |
It does throw NoSuchFieldException in the ri. Shouldn't j9 match that behavior? |
I think OpenJ9 |
Agreed. |
If that were true, it wouldn't execute the next line: Assert.fail("java.lang.System.security shoud NOT be accessible via reflection"); but we do see that failure message in the log. |
I just chatted with Keith and we were in agreement on this approach. For now I will disable the test and do this another time since it will add quite a bit to this pull request. I've started a list of follow up changes #20563 (comment) |
... from Java8andUp test_getDeclaredFieldLjava_lang_String Signed-off-by: Theresa Mammarella <[email protected]>
The suggestion is fine with me. We could also modify the test to look for System.initSecurityManager. I'd lean towards modifying the test, but don't have a strong opinion.
It makes the code more cryptic, although a comment would help with that. |
I excluded the failing test in test_getDeclaredFieldLjava_lang_String. java/lang/System/SecurityManagerWarnings.java.SecurityManagerWarnings still needs to be resolved. |
... of openjdk java/lang/System/SecurityManagerWarnings.java.SecurityManagerWarnings Signed-off-by: Theresa Mammarella <[email protected]>
/*[IF JAVA_SPEC_VERSION >= 24]*/ | ||
private static void initPhase3(boolean throwError) { | ||
if (throwError) { | ||
/*[MSG "K0B04", "A command line option has attempted to allow or enable the Security Manager. Enabling a Security Manager is not supported."]*/ | ||
throw new Error(Msg.getString("K0B04")); //$NON-NLS-1$ | ||
} | ||
} | ||
/*[ENDIF] JAVA_SPEC_VERSION >= 24 */ |
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.
Would it cause any problems to simply rename initSecurityManager
to initPhase3
?
Related: #20563