-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix jmx ssl not work issue #947
Conversation
Signed-off-by: unitsvc <[email protected]>
we need open ssl, can you check this feature in jdk17. |
@unitsvc What Java version are you testing? I created an integration test using RMI and SSL (not yet merged to The current code in
|
@dhoard Jmx ssl test jdk version:
java version "17.0.9" 2023-10-17 LTS
Java(TM) SE Runtime Environment (build 17.0.9+11-LTS-201)
Java HotSpot(TM) 64-Bit Server VM (build 17.0.9+11-LTS-201, mixed mode, sharing)
openjdk version "17.0.10" 2024-01-16 LTS
OpenJDK Runtime Environment (Red_Hat-17.0.10.0.7-1) (build 17.0.10+7-LTS)
OpenJDK 64-Bit Server VM (Red_Hat-17.0.10.0.7-1) (build 17.0.10+7-LTS, mixed mode, sharing) |
#1 keytool -genkeypair -alias jmx20240425 -keyalg RSA -keysize 2048 -validity 3650 -keystore jmx.jks -storepass changeit -keypass changeit
keytool -importkeystore -srckeystore jmx.jks -destkeystore jmx.p12 -srcstoretype jks -deststoretype pkcs12 -srcstorepass changeit -deststorepass changeit #2 package jmx;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.Locale;
public class JmxSSL {
private static final SimpleDateFormat SIMPLE_DATE_FORMAT = new SimpleDateFormat("yyyy-MM-dd | HH:mm:ss.SSS",
Locale.getDefault());
public static void main(String[] args) throws Exception {
System.out.println(
String.format(
"%s | %s | INFO | %s | %s",
SIMPLE_DATE_FORMAT.format(new Date()),
Thread.currentThread().getName(),
JmxSSL.class.getName(),
"Running"));
Thread.currentThread().join(); // wait forever
}
} #4 java \
-Djavax.net.debug=all \
-Dcom.sun.management.jmxremote=true \
-Dcom.sun.management.jmxremote.port=1234 \
-Dcom.sun.management.jmxremote.authenticate=true \
-Dcom.sun.management.jmxremote.ssl=true \
-Djavax.net.ssl.keyStore=jmx.p12 \
-Djavax.net.ssl.keyStorePassword=changeit \
-Dcom.sun.management.jmxremote.password.file=./jmxremote.password \
-Dcom.sun.management.jmxremote.access.file=./jmxremote.access \
JmxSSL.java #5 startDelaySeconds: 0
username: admin
password: "123456"
jmxUrl: "service:jmx:rmi:///jndi/rmi://127.0.0.1:1234/jmxrmi"
ssl: true
lowercaseOutputName: false
lowercaseOutputLabelNames: false #6 java \
-Djavax.net.debug=all \
-Djavax.net.ssl.trustStorePassword=changeit \
-Djavax.net.ssl.trustStore=jmx.p12 \
-XshowSettings:vm \
-jar ./jmx_prometheus_httpserver.jar 8081 ./config.yaml Then visit: http://127.0.0.1:8081/metrics @dhoard You can try it. if use main branch, it will not work, but this pr will work for ssl. |
@unitsvc When configuring RMI for SSL, the expectation is that the RMI registry is also configured for SSL. You are missing a Java system property in step #4 when starting the application:
If I remove this property, I can reproduce the |
@dhoard work , thx. java \
-Djavax.net.debug=all \
-Dcom.sun.management.jmxremote.registry.ssl=true \
-Dcom.sun.management.jmxremote=true \
-Dcom.sun.management.jmxremote.port=1234 \
-Dcom.sun.management.jmxremote.authenticate=true \
-Dcom.sun.management.jmxremote.ssl=true \
-Djavax.net.ssl.keyStore=jmx.p12 \
-Djavax.net.ssl.keyStorePassword=changeit \
-Dcom.sun.management.jmxremote.password.file=./jmxremote.password \
-Dcom.sun.management.jmxremote.access.file=./jmxremote.access \
JmxSSL.java |
@unitsvc thanks for the confirmation. I have merged the integration test... ... and associated integration test resources... |
Closing. Working correctly. |
@dhoard config.yaml can you add a toggle to control this line of code, which is turned on by default because this line of code is not compatible with collected. Projects cannot be smoothly migrated. |
@unitsvc collected? ... are you referring to collectd? My concern with allowing users to disable the RMI SSL registry requirement is that it sets up a half-secure configuration. I can entertain adding a Java system property or environment variable to control the behavior for a specific migration scenario, but I feel we shouldn't add a proper/documented configuration value to allow a configuration scenario that is only half-secure. |
@dhoard Yes, but the old system uses ssl, it is not allowed to disable ssl, if you add this parameter, collectd will not work, and this parameter is only valid for jdk5, adding a switch to let the user decide whether to enable or not, this does not break the original function, and greatly improve the system compatibility. |
While the documentation makes this statement, testing shows it's only partially correct and may be required depending on JVM system properties. Will adding an undocumented environment variable work for your migration?
|
It's too late today. I'll test it tomorrow. |
@dhoard if jmx server remove -Dcom.sun.management.jmxremote.registry.ssl=true , and jmx_prometheus_httpserver add export RMI_REGISTRY_SSL_DISABLED=true, will not work. JMX scrape failed: java.io.IOException: Failed to retrieve RMIServer stub: javax.naming.CommunicationException [Root exception is java.rmi.ConnectIOException: error during JRMP connection establishment; nested exception is:
javax.net.ssl.SSLHandshakeException: Remote host terminated the handshake]
at java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:370)
at java.management/javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java:270)
at io.prometheus.jmx.JmxScraper.doScrape(JmxScraper.java:126)
at io.prometheus.jmx.JmxCollector.collect(JmxCollector.java:771)
at io.prometheus.client.Collector.collect(Collector.java:45)
at io.prometheus.client.CollectorRegistry$MetricFamilySamplesEnumeration.findNextElement(CollectorRegistry.java:204)
at io.prometheus.client.CollectorRegistry$MetricFamilySamplesEnumeration.nextElement(CollectorRegistry.java:219)
at io.prometheus.client.CollectorRegistry$MetricFamilySamplesEnumeration.nextElement(CollectorRegistry.java:152)
at io.prometheus.client.exporter.common.TextFormat.write004(TextFormat.java:71)
at io.prometheus.client.exporter.common.TextFormat.writeFormat(TextFormat.java:53)
at io.prometheus.client.exporter.HTTPServer$HTTPMetricHandler.handle(HTTPServer.java:100)
at jdk.httpserver/com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:95)
at jdk.httpserver/sun.net.httpserver.AuthFilter.doFilter(AuthFilter.java:82)
at jdk.httpserver/com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:98)
at jdk.httpserver/sun.net.httpserver.ServerImpl$Exchange$LinkHandler.handle(ServerImpl.java:852)
at jdk.httpserver/com.sun.net.httpserver.Filter$Chain.doFilter(Filter.java:95)
at jdk.httpserver/sun.net.httpserver.ServerImpl$Exchange.run(ServerImpl.java:819)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
at java.base/java.lang.Thread.run(Thread.java:842)
Caused by: javax.naming.CommunicationException [Root exception is java.rmi.ConnectIOException: error during JRMP connection establishment; nested exception is:
javax.net.ssl.SSLHandshakeException: Remote host terminated the handshake]
at jdk.naming.rmi/com.sun.jndi.rmi.registry.RegistryContext.lookup(RegistryContext.java:138)
at java.naming/com.sun.jndi.toolkit.url.GenericURLContext.lookup(GenericURLContext.java:220)
at java.naming/javax.naming.InitialContext.lookup(InitialContext.java:409)
at java.management.rmi/javax.management.remote.rmi.RMIConnector.findRMIServerJNDI(RMIConnector.java:1839)
at java.management.rmi/javax.management.remote.rmi.RMIConnector.findRMIServer(RMIConnector.java:1813)
at java.management.rmi/javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java:302)
... 19 more
Caused by: java.rmi.ConnectIOException: error during JRMP connection establishment; nested exception is:
javax.net.ssl.SSLHandshakeException: Remote host terminated the handshake
at java.rmi/sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java:308)
at java.rmi/sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java:204)
at java.rmi/sun.rmi.server.UnicastRef.newCall(UnicastRef.java:344)
at java.rmi/sun.rmi.registry.RegistryImpl_Stub.lookup(RegistryImpl_Stub.java:116)
at jdk.naming.rmi/com.sun.jndi.rmi.registry.RegistryContext.lookup(RegistryContext.java:134) |
@unitsvc I am proposing adding the environment variable to resolve your migration scenario. The code has not been changed/merged into main. If this is a valid solution for your use case, I'll make the changes and merge it into for main for the upcoming release due very shortly. |
@dhoard That's great! |
@unitsvc I have merged the change into This change is to allow migration use cases where RMI SSL is only partially configured on an application, it is not meant for general usage. Partial RMI SSL is considered a security hole - this configuration change may be removed in future versions. Environment variable:
|
That's sound great.. |
The |
config.yaml
start jmx_prometheus_httpserver