[WFCORE-4296][WFCORE-5406] Add required --add-opens for java.base/com.sun.net.ssl.internal.ssl when HTTPS is configured #4966

ropalka · 2022-02-09T18:43:07Z

https://issues.redhat.com/browse/WFCORE-4296
https://issues.redhat.com/browse/WFCORE-5406

This PR follows up on:

[WFCORE-5406] Add --add-opens to the SE 9+ VM launch for expected reflective access by the server (round 2) #4728
[WFCORE-5406] Add --add-opens to the SE 9+ VM launch for expected reflective access by the server (round 2) #4728

This PR replaces:

[WFCORE-4296] Illegal Reflective Access wildfly#15208

wildfly-ci · 2022-02-09T22:17:51Z

Core - Full Integration Build 11374 outcome was FAILURE using a merge of 3e02eaf
Summary: Tests failed: 1 (1 new), passed: 7165, ignored: 144 Build time: 03:33:57

Failed tests

org.wildfly.extension.undertow.ServerServiceTestCase.testDefinedAttributes: java.lang.AssertionError: expected:<1> but was:<0>
	at org.wildfly.extension.undertow.ServerServiceTestCase.testDefinedAttributes(ServerServiceTestCase.java:141)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

soul2zimate · 2022-02-10T01:49:19Z

The warning on the illegal reflective access is gone, but there is going to be a new WARNING: package com.sun.net.ssl.internal.ssl not in java.base with Java 13 or later (server boot up and jboss-cli.sh launching), Because the internal package com.sun.net.ssl has been removed since Java 13 (https://bugs.openjdk.java.net/browse/JDK-8215430). Is that OK ?

ropalka · 2022-02-10T12:55:57Z

This is interesting problem @bstansberry . Package to be opened exists on JDK11 but doesn't exist on JDK17. I think JDK17 represent higher priority than JDK11, doesn't it? I am thinking whether to not drop this PR. WDYT?

bstansberry

@ropalka Please update this file as well:

https://github.com/wildfly/wildfly-core/blob/main/bootable-jar/boot/pom.xml#L61

fjuma · 2022-02-10T16:53:29Z

@ropalka Just FYI, when that method doesn't exist, a different approach is used to detect if FIPS is enabled, see:

https://issues.redhat.com/browse/WFCORE-5566

and

wildfly-core/elytron/src/main/java/org/wildfly/extension/elytron/SSLDefinitions.java

Line 1609 in a19937e

    
           return () -> new SecureRandom().getProvider().getName().toLowerCase().contains("fips");

ropalka · 2022-02-10T19:45:06Z

@ropalka Please update this file as well:

https://github.com/wildfly/wildfly-core/blob/main/bootable-jar/boot/pom.xml#L61

Fixed @bstansberry

bstansberry · 2022-02-10T20:29:59Z

@ropalka Good question, and thank you @soul2zimate for raising the point. I agree that SE 17 should get higher priority. JPMS warns in SE 11 are partly (mostly?) meant to guide people toward the right code for later releases when JPMS would be more strict. And we've been guided -- WFCORE-5566 is evidence of that.

But please leave this open as it's a good place to discuss the tradeoffs.

ropalka · 2022-02-10T20:35:34Z

But please leave this open as it's a good place to discuss the tradeoffs.

The main problem is there is currently not available any way to detect the JVM version we are running on. Neither in shell scripts nor in Java source code. This kind of problems would be nicely solvable if we would have such detection possibility.

github-actions · 2022-05-03T00:52:38Z

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

soul2zimate · 2022-06-08T01:42:12Z

Is there any decision made on this ?

github-actions · 2022-07-24T01:00:26Z

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

github-actions · 2022-10-22T01:13:33Z

There has been no activity on this PR for 90 days and it has been closed automatically.

soul2zimate · 2022-11-01T03:05:37Z

reopen this since the WFCORE issue is still open.

ropalka · 2022-12-13T20:17:24Z

Is there any decision made on this ?

No @soul2zimate . The problem still persists - no possibility to detect exact running JVM version in shell scripts.

github-actions · 2023-01-28T00:47:59Z

There has been no activity on this PR for 45 days. It will be auto-closed after 90 days.

jbliznak · 2023-04-21T22:28:23Z

no possibility to detect exact running JVM version in shell scripts

@ropalka why do we need exact version?

Aother idea, can't we just use the exact same trick as here (simply try the problematic argument first and act accordingly later)? https://github.com/wildfly/wildfly-core/blob/main/core-feature-pack/common/src/main/resources/content/bin/common.sh#L27-L29

gaol · 2023-07-04T06:55:34Z

@ropalka does what @jbliznak's suggestion make sense to you ? and may I ask you to rebase ? Thanks :)

wildfly-ci · 2023-08-17T16:25:13Z

Core -> WildFly Preview Integration Build 12795 outcome was FAILURE using a merge of 1914cd3
Summary: Tests failed: 1 (1 new), passed: 2651, ignored: 44 Build time: 01:52:25

Failed tests

org.wildfly.test.scripts.AppClientScriptTestCase.testBashScript: java.lang.AssertionError: Expected 2 lines.
Command Executed:
/opt/buildAgent/work/e8e0dd9c7c4ba60/full/testsuite/scripts/target/wildfly/bin/appclient.sh -v
Environment:
Output:
WARNING: package com.sun.net.ssl.internal.ssl not in java.base
&#27;[0m16:24:59,545 INFO  [org.jboss.modules] (main) JBoss Modules version 2.1.2.Final
&#27;[0mWildFly Core 22.0.0.Beta2-SNAPSHOT
 expected:<2> but was:<3>
java.lang.AssertionError: 
Expected 2 lines.
Command Executed:
/opt/buildAgent/work/e8e0dd9c7c4ba60/full/testsuite/scripts/target/wildfly/bin/appclient.sh -v
Environment:
Output:
WARNING: package com.sun.net.ssl.internal.ssl not in java.base
 [0m16:24:59,545 INFO  [org.jboss.modules] (main) JBoss Modules version 2.1.2.Final
 [0mWildFly Core 22.0.0.Beta2-SNAPSHOT
 expected:<2> but was:<3>
	at org.wildfly.test.scripts.AppClientScriptTestCase.testScript(AppClientScriptTestCase.java:52)
	at org.wildfly.test.scripts.ScriptTestCase.executeTests(ScriptTestCase.java:153)
	at org.wildfly.test.scripts.ScriptTestCase.testBashScript(ScriptTestCase.java:102)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

ropalka · 2023-09-12T07:06:54Z

@bstansberry please review

bstansberry

Hi @ropalka

I have a few comments; only substantial one is the timeout.

When this is ready please ping me in chat so I don't forget about it.

host-controller/src/main/java/org/jboss/as/host/controller/jvm/JvmType.java

launcher/src/main/java/org/wildfly/core/launcher/AbstractCommandBuilder.java

…internal.ssl when HTTPS is configured

…onditional because it is not available on all JDK versions

… method javadoc plus refactoring system property parsing

ropalka · 2023-09-14T14:33:27Z

Fixed @bstansberry . Thanks for review!

bstansberry · 2023-09-15T15:15:37Z

pom.xml

@@ -164,7 +164,7 @@
             NB: In case an update is made to these exports and opens, make sure that the common.sh script is in sync.
        -->
        <embedding.jar.jpms.exports>java.desktop/sun.awt java.naming/com.sun.jndi.ldap java.naming/com.sun.jndi.url.ldap java.naming/com.sun.jndi.url.ldaps jdk.naming.dns/com.sun.jndi.dns</embedding.jar.jpms.exports>
-        <embedding.jar.jpms.opens>java.base/java.lang java.base/java.lang.invoke java.base/java.lang.reflect java.base/java.io java.base/java.net java.base/java.security java.base/java.util java.base/java.util.concurrent java.management/javax.management java.naming/javax.naming</embedding.jar.jpms.opens>
+        <embedding.jar.jpms.opens>java.base/com.sun.net.ssl.internal.ssl java.base/java.lang java.base/java.lang.invoke java.base/java.lang.reflect java.base/java.io java.base/java.net java.base/java.security java.base/java.util java.base/java.util.concurrent java.management/javax.management java.naming/javax.naming</embedding.jar.jpms.opens>


@ropalka @jfdenise AIUI this will result in a warning if the CLI or a bootable jar is used on SE 17. So in this case it seems we are forced to make a choice, unless there's some fancy MANIFEST.mf, etc thing in SE that allows some kind of conditional behavior.

If not, my inclination is to not add this since

Getting the warn with SE 11 is the existing state, whereas introducing a warn for SE 17 would be a breaking change. All things equal, annoying a fresh set of users in order to stop annoying ones who may already be used to the warn seems a bad tradeoff.

We'll be moving on from SE 11 soon enough.

unless there's some fancy MANIFEST.mf, etc thing in SE that allows some kind of conditional behavior.

there isn't anything conditional, but when MANIFEST.MF has add-opens/add-exports on non-existing module, it silently ignores it, so IIUC this could be fine then.

https://openjdk.org/jeps/261#Breaking-encapsulation

Two new JDK-specific JAR-file manifest attributes are defined to correspond to the --add-exports and --add-opens command-line options: Add-Exports: <module>/<package>( <module>/<package>)* Add-Opens: <module>/<package>( <module>/<package>)* The value of each attribute is a space-separated list of slash-separated module-name/package-name pairs. A <module>/<package> pair in the value of an Add-Exports attribute has the same meaning as the command-line option --add-exports <module>/<package>=ALL-UNNAMED. A <module>/<package> pair in the value of an Add-Opens attribute has the same meaning as the command-line option --add-opens <module>/<package>=ALL-UNNAMED. Each attribute can occur at most once, in the main section of a MANIFEST.MF file. A particular pair can be listed more than once. If a specified module was not resolved, or if a specified package does not exist, then the corresponding pair is ignored. These attributes are interpreted only in the main executable JAR file of an application, i.e., in the JAR file specified to the -jar option of the Java run-time launcher; they are ignored in all other JAR files.

Thanks, a lot @jbliznak -- that's very helpful.

bstansberry · 2023-09-16T18:34:08Z

Thanks @ropalka and @jbliznak

ropalka · 2023-09-18T08:15:46Z

@jbliznak is right. Not existenting values of add-opens or add-exports attributes in MANIFEST.MF are silently ignored for executable jar files.

ropalka · 2023-09-18T08:16:01Z

Thanks for review and merge of this PR @bstansberry

ropalka · 2023-09-18T08:30:00Z

I provided a reproducer (for the claim about executable jars) and attached it to the JIRA @bstansberry

ropalka mentioned this pull request Feb 9, 2022

[WFCORE-4296] Illegal Reflective Access wildfly/wildfly#15208

Closed

github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Feb 9, 2022

bstansberry requested changes Feb 10, 2022

View reviewed changes

ropalka force-pushed the WFCORE-4296 branch from 3e02eaf to 0f420c9 Compare February 10, 2022 19:44

yersan added the rebase-this This PR must be rebased before it is merged label Mar 10, 2022

jfdenise mentioned this pull request Mar 14, 2022

[HOLD] Fix for WFCORE-5835, Missing modular export when enabling dns.ping protocol #5005

Closed

bstansberry removed the rebase-this This PR must be rebased before it is merged label Mar 18, 2022

github-actions bot added the Stale label May 3, 2022

github-actions bot removed the Stale label Jun 9, 2022

github-actions bot added the Stale label Jul 24, 2022

github-actions bot closed this Oct 22, 2022

soul2zimate reopened this Nov 1, 2022

github-actions bot removed the Stale label Nov 2, 2022

github-actions bot added the Stale label Jan 28, 2023

github-actions bot removed the Stale label Apr 22, 2023

darranl added the rebase-this This PR must be rebased before it is merged label May 25, 2023

ropalka force-pushed the WFCORE-4296 branch from 0f420c9 to 1914cd3 Compare August 17, 2023 13:35

ropalka force-pushed the WFCORE-4296 branch 2 times, most recently from d559874 to 33624f8 Compare September 8, 2023 07:50

ropalka removed the rebase-this This PR must be rebased before it is merged label Sep 12, 2023

bstansberry requested changes Sep 12, 2023

View reviewed changes

ropalka and others added 4 commits September 14, 2023 16:27

[WFCORE-4296] Add required --add-opens for java.base/com.sun.net.ssl.…

4e705bf

…internal.ssl when HTTPS is configured

[WFCORE-4296] Make adding of java.base/com.sun.net.ssl.internal.ssl c…

853eaea

…onditional because it is not available on all JDK versions

[WFCORE-4296] Tweaks for Windows scripts

613f505

[WFCORE-4296] Increasing forked process timeout to 30 seconds, adding…

3b6b41f

… method javadoc plus refactoring system property parsing

ropalka force-pushed the WFCORE-4296 branch from f980034 to 3b6b41f Compare September 14, 2023 14:30

bstansberry reviewed Sep 15, 2023

View reviewed changes

bstansberry approved these changes Sep 16, 2023

View reviewed changes

bstansberry merged commit 2d8f15c into wildfly:main Sep 16, 2023
12 checks passed

ropalka deleted the WFCORE-4296 branch September 18, 2023 08:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFCORE-4296][WFCORE-5406] Add required --add-opens for java.base/com.sun.net.ssl.internal.ssl when HTTPS is configured #4966

[WFCORE-4296][WFCORE-5406] Add required --add-opens for java.base/com.sun.net.ssl.internal.ssl when HTTPS is configured #4966

ropalka commented Feb 9, 2022 •

edited

wildfly-ci commented Feb 9, 2022

soul2zimate commented Feb 10, 2022 •

edited

ropalka commented Feb 10, 2022

bstansberry left a comment

fjuma commented Feb 10, 2022

ropalka commented Feb 10, 2022

bstansberry commented Feb 10, 2022

ropalka commented Feb 10, 2022 •

edited

github-actions bot commented May 3, 2022

soul2zimate commented Jun 8, 2022

github-actions bot commented Jul 24, 2022

github-actions bot commented Oct 22, 2022

soul2zimate commented Nov 1, 2022

ropalka commented Dec 13, 2022

github-actions bot commented Jan 28, 2023

jbliznak commented Apr 21, 2023

gaol commented Jul 4, 2023

wildfly-ci commented Aug 17, 2023

ropalka commented Sep 12, 2023

bstansberry left a comment

ropalka commented Sep 14, 2023

bstansberry Sep 15, 2023

jbliznak Sep 15, 2023 •

edited

bstansberry Sep 16, 2023

bstansberry commented Sep 16, 2023

ropalka commented Sep 18, 2023

ropalka commented Sep 18, 2023

ropalka commented Sep 18, 2023

[WFCORE-4296][WFCORE-5406] Add required --add-opens for java.base/com.sun.net.ssl.internal.ssl when HTTPS is configured #4966

[WFCORE-4296][WFCORE-5406] Add required --add-opens for java.base/com.sun.net.ssl.internal.ssl when HTTPS is configured #4966

Conversation

ropalka commented Feb 9, 2022 • edited

wildfly-ci commented Feb 9, 2022

Failed tests

soul2zimate commented Feb 10, 2022 • edited

ropalka commented Feb 10, 2022

bstansberry left a comment

Choose a reason for hiding this comment

fjuma commented Feb 10, 2022

ropalka commented Feb 10, 2022

bstansberry commented Feb 10, 2022

ropalka commented Feb 10, 2022 • edited

github-actions bot commented May 3, 2022

soul2zimate commented Jun 8, 2022

github-actions bot commented Jul 24, 2022

github-actions bot commented Oct 22, 2022

soul2zimate commented Nov 1, 2022

ropalka commented Dec 13, 2022

github-actions bot commented Jan 28, 2023

jbliznak commented Apr 21, 2023

gaol commented Jul 4, 2023

wildfly-ci commented Aug 17, 2023

Failed tests

ropalka commented Sep 12, 2023

bstansberry left a comment

Choose a reason for hiding this comment

ropalka commented Sep 14, 2023

bstansberry Sep 15, 2023

Choose a reason for hiding this comment

jbliznak Sep 15, 2023 • edited

Choose a reason for hiding this comment

bstansberry Sep 16, 2023

Choose a reason for hiding this comment

bstansberry commented Sep 16, 2023

ropalka commented Sep 18, 2023

ropalka commented Sep 18, 2023

ropalka commented Sep 18, 2023

ropalka commented Feb 9, 2022 •

edited

soul2zimate commented Feb 10, 2022 •

edited

ropalka commented Feb 10, 2022 •

edited

jbliznak Sep 15, 2023 •

edited