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

[Build] Disallow Java security-manager in all I-/Y-/Smoke-tests #2659

Merged

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell commented Dec 14, 2024

Part of #2623

@merks do you think we should explicitly disallow the security-manager or just remove the argument to explicitly enable it?
Since Java-18 the default value of the java.security.manager property is disallow as part of JEP-411:

In order to make the builds a bit simpler I would be in favor to just remove the java.security.manager argument. With that only the Java-17 tests would run with enabled security-manager and all others would have it disabled. And Java-17 will probably not last long any-ways.

Btw. I have replayed a I-build test with explicit disallowed security-manager in order to test in advance if everything falls apart or not (you can see it in the diff):

@akurtakov
Copy link
Member

IMO drop the argument. For #2654 builds will be moved to Java 21 soon as this is a prereq to actually do anything.

@merks
Copy link
Contributor

merks commented Dec 14, 2024

Setting it to disallow now will let us detected problems sooner rather than later...

@HannesWell HannesWell force-pushed the no-security-manager-in-tests branch from c0a15b4 to c63acd4 Compare December 14, 2024 09:22
@HannesWell
Copy link
Member Author

HannesWell commented Dec 14, 2024

Setting it to disallow now will let us detected problems sooner rather than later...

Since Java-18 the default is already disallow. So by removing it we would already test this, but only in two of our six I-build configs. I assume that's sufficient, hoping that the security-manager will behave the same everywhere.

Btw. I have replayed a I-build test with explicit disallowed security-manager in order to test in advance if everything falls apart or not (you can see it in the diff):

* https://ci.eclipse.org/releng/job/AutomatedTests/job/ep435I-unit-linux-x86_64-java21/25/

This resulted in the following additional failures. If we not disable it for all builds we will slightly reduce the flood of new failures.
But I'll try to have a look at them tomorrow.

org.eclipse.ant.tests.core.tests.FrameworkTests.testClasspathOrdering()
org.eclipse.ant.tests.core.tests.FrameworkTests.testClasspathOrderingDeprecated()
org.eclipse.ant.tests.core.tests.OptionTests.testInputHandler()
org.eclipse.ant.tests.core.tests.OptionTests.testMinusK()
org.eclipse.ant.tests.core.tests.OptionTests.testMinusKeepGoing()
org.eclipse.ant.tests.core.tests.OptionTests.testSpecifyBothBadAndGoodTargetsAsArg()
org.eclipse.ant.tests.core.tests.OptionTests.testSpecifyTargetAsArg()
org.eclipse.ant.tests.core.tests.OptionTests.testSpecifyTargetAsArgWithOtherOptions()
org.eclipse.ant.tests.core.tests.OptionTests.testSpecifyTargetsAsArgWithOtherOptions()
org.eclipse.ant.tests.core.tests.OptionTests.testUnknownArg()
org.eclipse.ant.tests.core.tests.TaskTests.testAddTaskFromFolder()
org.eclipse.ant.tests.core.tests.TaskTests.testAddTaskSettingLibrary()
org.eclipse.ant.tests.core.tests.TaskTests.testAddTaskSettingLibraryEntry()
org.eclipse.ant.tests.core.tests.TaskTests.testTaskDefinedInExtensionPoint()
org.eclipse.ant.tests.core.tests.TaskTests.testTaskDefinedInExtensionPointWithURI()
org.eclipse.ant.tests.core.tests.TypeTests.testAddType()
org.eclipse.ant.tests.core.tests.TypeTests.testTypeDefinedInExtensionPoint()
org.eclipse.jdt.core.tests.model.ClasspathTests.testExtraAttributes4
org.eclipse.jdt.ui.tests.views.SmokeViewsTest.testOpenSourceView
org.eclipse.osgi.tests.bundles.EquinoxBundleAdaptTests.testAdapt_ProtectionDomain
org.eclipse.osgi.tests.bundles.SystemBundleTests.testDeleteBundleFile
org.eclipse.osgi.tests.bundles.SystemBundleTests.testDynamicSecurityManager
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testAccessControlContext01
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testAccessControlContext01a
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testAccessControlContext02
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testAccessControlContext03
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testAccessControlContext04
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testAccessControlContext05
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testAccessControlContext06
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testAccessControlContext07
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testBug286307
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testCreateDomain
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testDefaultPermissions01
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testDefaultPermissions02
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testEncodingInfos01
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testEncodingInfos02
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testLocationPermission01
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testLocationPermission02
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testLocationPermission03
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testMultipleLocationConditions01
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testMultipleLocationConditions02
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testMutableConditions
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testNotLocationCondition01
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testNotLocationCondition02
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testPermissionCheckCache
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testPostponedConditions01
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testPostponedConditions02
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testPostponedConditions03
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testPostponedConditions04
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testPostponedConditions05
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testRelativeFilePermission
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testSecurityManager01
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testUpdate01
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testUpdate02
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testUpdate03
org.eclipse.osgi.tests.securityadmin.SecurityAdminUnitTests.testUpdate04
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testBug254600
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testBug287750
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testBug367614
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testDynamicImportWithSecurity
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testEnableSecurityManager01
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testEnableSecurityManager02
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testEnableSecurityManager03
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testEnableSecurityManager04
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testEnableSecurityManager05
org.eclipse.osgi.tests.securityadmin.SecurityManagerTests.testLocalization01

@HannesWell
Copy link
Member Author

This resulted in the following additional failures. If we not disable it for all builds we will slightly reduce the flood of new failures.
But I'll try to have a look at them tomorrow.

The failing tests in Equinox are fixed respectively disabled if setting a security-manager is not permitted with

Another reason to not explicitly disallow setting a SecurityManager are the failures of the org.eclipse.ant.tests.core.tests.
Currently ANT determines if a SecurityManager may be set only based on the version of the JVM. Therefore if one sets -Djava.security.manager=disallow for a Java-17 VM, it will lead to problems when executing the Java task because then ANT tries to set default permissions in:
https://github.com/apache/ant/blob/375b5132adfb66953cc698aedebb9b356c1ca180/src/main/org/apache/tools/ant/taskdefs/Java.java#L206-L207

I have created a PR to refine ANT in this regard but that's of course something we can't use immediately:

Therefore we should just stick to the defaults or we'll end up with failing ANT tests for all configurations running on Java-17.

There are still a few more ant.tests failing with a disabled security-manger and I'm currently looking into them.
Will report back when they are fixed.

@HannesWell
Copy link
Member Author

There are still a few more ant.tests failing with a disabled security-manger and I'm currently looking into them.
Will report back when they are fixed.

This should now be fixed with:

and together with

disabling the security-manager in all tests shouldn't cause regressions anymore.
@merks are you fine with this, once all the mentioned PRs are submitted?

@merks
Copy link
Contributor

merks commented Dec 15, 2024

Yes, this looks good and is a good step forward.

@HannesWell
Copy link
Member Author

Great. Then lets submit this and see if really all I-build tests can handle a disallowed security-manager.

@HannesWell HannesWell merged commit dafdf45 into eclipse-platform:master Dec 15, 2024
4 checks passed
@HannesWell HannesWell deleted the no-security-manager-in-tests branch December 15, 2024 20:29
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.

3 participants