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

Set javatest.security.noSecurityManager=true if Java 18+ #894

Merged
merged 1 commit into from Mar 28, 2022

Conversation

brideck
Copy link
Contributor

@brideck brideck commented Mar 14, 2022

Fixes Issue
Allows JavaTest to be run with Java 18+

Related Issue(s)
#743

Describe the change
This updates all invocations of JavaTest in the Platform TCK to use noSecurityManager=true when running with Java 18+. This has no impact on actual test outcomes because the VI's processes and the execution commands defined in ts.jte will still have the default security behavior for the level of JDK being tested.

Note: This implementation requires use of the condition introduced in Ant 1.10.2. To support older levels of Ant, a more elaborate statement using Ant's builtin ant.java.version would be required instead.

CC @alwin-joseph @anajosep @arjantijms @cesarhernandezgt @dblevins @m0mus @edbratt @gurunrao @jansupol @jgallimore @kazumura @kwsutter @LanceAndersen @bhatpmk @RohitKumarJain @shighbar @gthoman @brideck @scottmarlow

@brideck brideck self-assigned this Mar 14, 2022
@scottmarlow
Copy link
Contributor

scottmarlow commented Mar 14, 2022

Nice to see this solution for #743 (comment)

Looks like Java Test Harness 6.0 will default to not install Java SecurityManager via openjdk/jtharness@1258355

@@ -34,6 +34,11 @@
<import file="./ts.vehicles.xml"/>

<!-- PROPERTIES -->
<condition property="noSecurityManager" value="true">
<javaversion atleast="18"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Automatically disabling the Security Manager on SE 18+ now sounds wrong, as the Java Security Manager still can work in Java SE 18.

Could users set -DnoSecurityManager=true instead?

I would expect that some users will want to also enable specify -Djava.security.manager=allowed to let Java SE 18 know that the SM should be allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason that anyone would want to run with a Security Manager here? Remember that this is only changing the JavaTest harness process itself, and has zero impact on how the prospective CI is configured and behaves. This could only affect sameJVM testing, which I don't believe to be valid for our TCK, so I think we could all literally run with the noSecurityManager property set in all of our runs (with any JDK) and see no difference in our ability to run the tests successfully.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that anyone would want to run with a Security Manager here?

I haven't experimented with sameJVM for many years (via native code wrappers that launch the JVM) so it does seem unlikely that I would experience the same problem but others could.

I didn't realize that this pr is for a non-general case but still, I don't understand why you wouldn't want the flexibility to enable the SecurityManager with SE 18 when sameJVM is being used. That sounds likely to vary between different EE implementations.

Copy link
Contributor Author

@brideck brideck Mar 25, 2022

Choose a reason for hiding this comment

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

The PR is for a general case. Everyone will hit the exception when running with Java 18 when trying to use JavaTest, but the SecurityManager currently used here is only actually useful in the sameJVM case per the comments in the JavaTest class.

My understanding was that the sameJVM mode is not a valid way to be running the Platform TCK. From comments in the ts.jte: "NOTE: This option is only to be used for sanity checking and not when running CTS for compatibility."

It's not a matter of not wanting flexibility, it's a matter of whether said flexibility should be a requirement if no one is ever going to use it. There's useful flexibility, and then there's flexibility for flexibility's sake.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense so we only impacting the case of the Java Test Harness Main entry point use of Java Security Manager.

@scottmarlow
Copy link
Contributor

This pr as currently written should resolve #904

@scottmarlow scottmarlow merged commit bb3a7b7 into jakartaee:master Mar 28, 2022
scottmarlow added a commit to scottmarlow/jakartaee-tck that referenced this pull request Mar 14, 2024
scottmarlow added a commit that referenced this pull request Mar 14, 2024
Update jaxws TCK version to 4.0.1 to prepare for staging jakarta-xml-ws-tck-4.0.1.zip to see if #894 change is included
@scottmarlow
Copy link
Contributor

[http://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-xml-ws-tck-4.0.1.zip] was built today and seems to contain the expected ts.top.import.xml change made by this pull request:

{quote}

<condition property="noSecurityManager" value="true">
    <javaversion atleast="18"/>
</condition>
<property name="noSecurityManager" value="false"/>

{quote}

@brideck I'm not sure of why https://download.eclipse.org/jakartaee/xml-web-services/4.0/jakarta-xml-ws-tck-4.0.0.zip didn't contain the ^ noSecurityManager change. I suggest that we create a https://github.com/jakartaee/specifications pull request to promote the staged http://download.eclipse.org/ee4j/jakartaee-tck/jakartaee10/staged/eftl/jakarta-xml-ws-tck-4.0.1.zip.

CC @gurunrao I think ^ is needed for releasing a new JAX-WS/jax-ws-api TCK that can run on Java 21.

@scottmarlow
Copy link
Contributor

I think that we need a tracking https://github.com/jakartaee/jax-ws-api/issues issue for addressing ^ so we can promote the staged TCK.

scottmarlow added a commit to scottmarlow/specifications that referenced this pull request Mar 19, 2024
…m-tck#894 change to address jakartaee/platform-tck#743 which didn't get released with jakarta-xml-ws-tck-4.0.0.zip

Signed-off-by: Scott Marlow <smarlow@redhat.com>
ivargrimstad pushed a commit to jakartaee/specifications that referenced this pull request Mar 19, 2024
…m-tck#894 change to address jakartaee/platform-tck#743 which didn't get released with jakarta-xml-ws-tck-4.0.0.zip (#708)

Signed-off-by: Scott Marlow <smarlow@redhat.com>
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