jmx-scraper add ssl support (#1710)

Author: SylvainJuge

README.md

@@ -15,11 +15,11 @@ feature or via instrumentation, this project is hopefully for you.
 ## Provided Libraries
 
 | Status* | Library                                                           |
-| ------- |-------------------------------------------------------------------|
+|---------|-------------------------------------------------------------------|
 | beta    | [AWS Resources](./aws-resources/README.md)                        |
 | stable  | [AWS X-Ray SDK Support](./aws-xray/README.md)                     |
 | alpha   | [AWS X-Ray Propagator](./aws-xray-propagator/README.md)           |
-| alpha   | [Baggage Processors](./baggage-processor/README.md) |
+| alpha   | [Baggage Processors](./baggage-processor/README.md)               |
 | alpha   | [zstd Compressor](./compressors/compressor-zstd/README.md)        |
 | alpha   | [Consistent Sampling](./consistent-sampling/README.md)            |
 | alpha   | [Disk Buffering](./disk-buffering/README.md)                      |
@@ -29,6 +29,7 @@ feature or via instrumentation, this project is hopefully for you.
 | alpha   | [JFR Connection](./jfr-connection/README.md)                      |
 | alpha   | [JFR Events](./jfr-events/README.md)                              |
 | alpha   | [JMX Metric Gatherer](./jmx-metrics/README.md)                    |
+| alpha   | [JMX Metric Scraper](./jmx-scraper/README.md)                     |
 | alpha   | [Kafka Support](./kafka-exporter/README.md)                       |
 | alpha   | [OpenTelemetry Maven Extension](./maven-extension/README.md)      |
 | alpha   | [Micrometer MeterProvider](./micrometer-meter-provider/README.md) |

jmx-scraper/README.md

@@ -29,20 +29,29 @@ Configuration can be provided through:
   `otel.jmx.service.url=service:jmx:rmi:///jndi/rmi://tomcat:9010/jmxrmi` is written to stdin.
 - environment variables: `OTEL_JMX_TARGET_SYSTEM=tomcat OTEL_JMX_SERVICE_URL=service:jmx:rmi:///jndi/rmi://tomcat:9010/jmxrmi java -jar scraper.jar`
 
-SDK auto-configuration is being used, so all the configuration options can be set using the java
+SDK autoconfiguration is being used, so all the configuration options can be set using the java
 properties syntax or the corresponding environment variables.
 
 For example the `otel.jmx.service.url` option can be set with the `OTEL_JMX_SERVICE_URL` environment variable.
 
 ## Configuration reference
 
-| config option            | description                                                                                                         |
-|--------------------------|---------------------------------------------------------------------------------------------------------------------|
-| `otel.jmx.service.url`   | mandatory JMX URL to connect to the remote JVM                                                                      |
-| `otel.jmx.target.system` | comma-separated list of systems to monitor, mandatory unless a custom configuration is used                         |
-| `otel.jmx.config`        | comma-separated list of paths to custom YAML metrics definition, mandatory when `otel.jmx.target.system` is not set |
-| `otel.jmx.username`      | user name for JMX connection, mandatory when JMX authentication is enabled on target JVM                            |
-| `otel.jmx.password`      | password for JMX connection, mandatory when JMX authentication is enabled on target JVM                             |
+| config option                  | default value | description                                                                                                                               |
+|--------------------------------|---------------|-------------------------------------------------------------------------------------------------------------------------------------------|
+| `otel.jmx.service.url`         | -             | mandatory JMX URL to connect to the remote JVM                                                                                            |
+| `otel.jmx.target.system`       | -             | comma-separated list of systems to monitor, mandatory unless `otel.jmx.config` is set                                                     |
+| `otel.jmx.config`              | empty         | comma-separated list of paths to custom YAML metrics definition, mandatory when `otel.jmx.target.system` is not set                       |
+| `otel.jmx.username`            | -             | user name for JMX connection, mandatory when JMX authentication is set on target JVM with`com.sun.management.jmxremote.authenticate=true` |
+| `otel.jmx.password`            | -             | password for JMX connection, mandatory when JMX authentication is set on target JVM with `com.sun.management.jmxremote.authenticate=true` |
+| `otel.jmx.remote.registry.ssl` | `false`       | connect to an SSL-protected registry when enabled on target JVM with `com.sun.management.jmxremote.registry.ssl=true`                     |
+
+When both `otel.jmx.target.system` and `otel.jmx.config` configuration options are used at the same time:
+
+- `otel.jmx.target.system` provides ready-to-use metrics and `otel.jmx.config` allows to add custom definitions.
+- The metrics definitions will be the aggregation of both.
+- There is no guarantee on the priority or any ability to override the definitions.
+
+If there is a need to override existing ready-to-use metrics or to keep control on the metrics definitions, using a custom YAML definition with `otel.jmx.config` is the recommended option.
 
 Supported values for `otel.jmx.target.system`:
 

jmx-scraper/build.gradle.kts

@@ -37,6 +37,8 @@ testing {
         implementation("com.linecorp.armeria:armeria-junit5")
         implementation("com.linecorp.armeria:armeria-grpc")
         implementation("io.opentelemetry.proto:opentelemetry-proto:1.5.0-alpha")
+        implementation("org.bouncycastle:bcprov-jdk18on:1.80")
+        implementation("org.bouncycastle:bcpkix-jdk18on:1.80")
       }
     }
   }

jmx-scraper/src/integrationTest/java/io/opentelemetry/contrib/jmxscraper/JmxConnectionTest.java

@@ -7,10 +7,13 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.nio.file.Path;
+import java.security.cert.X509Certificate;
 import java.util.function.Function;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.testcontainers.containers.GenericContainer;
@@ -31,6 +34,10 @@ public class JmxConnectionTest {
   private static final int JMX_PORT = 9999;
   private static final String APP_HOST = "app";
 
+  // key/trust stores passwords
+  private static final String CLIENT_PASSWORD = "client";
+  private static final String SERVER_PASSWORD = "server";
+
   private static final Logger jmxScraperLogger = LoggerFactory.getLogger("JmxScraperContainer");
   private static final Logger appLogger = LoggerFactory.getLogger("TestAppContainer");
 
@@ -70,6 +77,84 @@ void userPassword() {
         scraper -> scraper.withRmiServiceUrl(APP_HOST, JMX_PORT).withUser(login).withPassword(pwd));
   }
 
+  @Test
+  void serverSsl(@TempDir Path tempDir) {
+    testServerSsl(tempDir, /* sslRmiRegistry= */ false);
+  }
+
+  @Test
+  void serverSslWithSslRmiRegistry(@TempDir Path tempDir) {
+    testServerSsl(tempDir, /* sslRmiRegistry= */ true);
+  }
+
+  private static void testServerSsl(Path tempDir, boolean sslRmiRegistry) {
+    // two keystores:
+    // server keystore with public/private key pair
+    // client trust store with certificate from server
+
+    TestKeyStore serverKeyStore =
+        TestKeyStore.newKeyStore(tempDir.resolve("server.jks"), SERVER_PASSWORD);
+    TestKeyStore clientTrustStore =
+        TestKeyStore.newKeyStore(tempDir.resolve("client.jks"), CLIENT_PASSWORD);
+
+    X509Certificate serverCertificate = serverKeyStore.addKeyPair();
+    clientTrustStore.addTrustedCertificate(serverCertificate);
+
+    connectionTest(
+        app ->
+            (sslRmiRegistry ? app.withSslRmiRegistry(4242) : app)
+                .withJmxPort(JMX_PORT)
+                .withJmxSsl()
+                .withKeyStore(serverKeyStore),
+        scraper ->
+            (sslRmiRegistry ? scraper.withSslRmiRegistry() : scraper)
+                .withRmiServiceUrl(APP_HOST, JMX_PORT)
+                .withTrustStore(clientTrustStore));
+  }
+
+  @Test
+  void serverSslClientSsl(@TempDir Path tempDir) {
+    // Note: this could have been made simpler by relying on the fact that keystore could be used
+    // as a trust store, but having clear split provides also some extra clarity
+    //
+    // 4 keystores:
+    // server keystore with public/private key pair
+    // server truststore with client certificate
+    // client key store with public/private key pair
+    // client trust store with certificate from server
+
+    TestKeyStore serverKeyStore =
+        TestKeyStore.newKeyStore(tempDir.resolve("server-keystore.jks"), SERVER_PASSWORD);
+    TestKeyStore serverTrustStore =
+        TestKeyStore.newKeyStore(tempDir.resolve("server-truststore.jks"), SERVER_PASSWORD);
+
+    X509Certificate serverCertificate = serverKeyStore.addKeyPair();
+
+    TestKeyStore clientKeyStore =
+        TestKeyStore.newKeyStore(tempDir.resolve("client-keystore.jks"), CLIENT_PASSWORD);
+    TestKeyStore clientTrustStore =
+        TestKeyStore.newKeyStore(tempDir.resolve("client-truststore.jks"), CLIENT_PASSWORD);
+
+    X509Certificate clientCertificate = clientKeyStore.addKeyPair();
+
+    // adding certificates in trust stores
+    clientTrustStore.addTrustedCertificate(serverCertificate);
+    serverTrustStore.addTrustedCertificate(clientCertificate);
+
+    connectionTest(
+        app ->
+            app.withJmxPort(JMX_PORT)
+                .withJmxSsl()
+                .withClientSslCertificate()
+                .withKeyStore(serverKeyStore)
+                .withTrustStore(serverTrustStore),
+        scraper ->
+            scraper
+                .withRmiServiceUrl(APP_HOST, JMX_PORT)
+                .withKeyStore(clientKeyStore)
+                .withTrustStore(clientTrustStore));
+  }
+
   private static void connectionTest(
       Function<TestAppContainer, TestAppContainer> customizeApp,
       Function<JmxScraperContainer, JmxScraperContainer> customizeScraper) {
@@ -86,17 +171,23 @@ private static void connectionTest(
   private static void checkConnectionLogs(JmxScraperContainer scraper, boolean expectedOk) {
 
     String[] logLines = scraper.getLogs().split("\n");
-    String lastLine = logLines[logLines.length - 1];
-
-    if (expectedOk) {
-      assertThat(lastLine)
-          .describedAs("should log connection success")
-          .endsWith("JMX connection test OK");
-    } else {
-      assertThat(lastLine)
-          .describedAs("should log connection failure")
-          .endsWith("JMX connection test ERROR");
-    }
+
+    // usually only the last line can be checked, however when it fails with an exception
+    // the stack trace is last in the output, so it's simpler to check all lines of log output
+
+    assertThat(logLines)
+        .anySatisfy(
+            line -> {
+              if (expectedOk) {
+                assertThat(line)
+                    .describedAs("should log connection success")
+                    .contains("JMX connection test OK");
+              } else {
+                assertThat(line)
+                    .describedAs("should log connection failure")
+                    .contains("JMX connection test ERROR");
+              }
+            });
   }
 
   private static void waitTerminated(GenericContainer<?> container) {

jmx-scraper/src/integrationTest/java/io/opentelemetry/contrib/jmxscraper/JmxScraperContainer.java

@@ -8,8 +8,11 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import java.nio.file.Path;
 import java.time.Duration;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
@@ -29,6 +32,9 @@ public class JmxScraperContainer extends GenericContainer<JmxScraperContainer> {
   private String password;
   private final List<String> extraJars;
   private boolean testJmx;
+  private TestKeyStore keyStore;
+  private TestKeyStore trustStore;
+  private boolean sslRmiRegistry;
 
   public JmxScraperContainer(String otlpEndpoint, String baseImage) {
     super(baseImage);
@@ -44,20 +50,38 @@ public JmxScraperContainer(String otlpEndpoint, String baseImage) {
     this.extraJars = new ArrayList<>();
   }
 
+  /**
+   * Adds a target system
+   *
+   * @param targetSystem target system
+   * @return this
+   */
   @CanIgnoreReturnValue
   public JmxScraperContainer withTargetSystem(String targetSystem) {
     targetSystems.add(targetSystem);
     return this;
   }
 
+  /**
+   * Set connection to a standard JMX service URL
+   *
+   * @param host JMX host
+   * @param port JMX port
+   * @return this
+   */
   @CanIgnoreReturnValue
   public JmxScraperContainer withRmiServiceUrl(String host, int port) {
-    // TODO: adding a way to provide 'host:port' syntax would make this easier for end users
     return withServiceUrl(
         String.format(
             Locale.getDefault(), "service:jmx:rmi:///jndi/rmi://%s:%d/jmxrmi", host, port));
   }
 
+  /**
+   * Set connection to a JMX service URL
+   *
+   * @param serviceUrl service URL
+   * @return this
+   */
   @CanIgnoreReturnValue
   public JmxScraperContainer withServiceUrl(String serviceUrl) {
     this.serviceUrl = serviceUrl;
@@ -112,12 +136,52 @@ public JmxScraperContainer withCustomYaml(String yamlPath) {
     return this;
   }
 
+  /**
+   * Configure the scraper JVM to only test connection with the JMX endpoint
+   *
+   * @return this
+   */
   @CanIgnoreReturnValue
   public JmxScraperContainer withTestJmx() {
     this.testJmx = true;
     return this;
   }
 
+  /**
+   * Configure key store for the scraper JVM
+   *
+   * @param keyStore key store
+   * @return this
+   */
+  @CanIgnoreReturnValue
+  public JmxScraperContainer withKeyStore(TestKeyStore keyStore) {
+    this.keyStore = keyStore;
+    return this;
+  }
+
+  /**
+   * Configure trust store for the scraper JVM
+   *
+   * @param trustStore trust store
+   * @return this
+   */
+  @CanIgnoreReturnValue
+  public JmxScraperContainer withTrustStore(TestKeyStore trustStore) {
+    this.trustStore = trustStore;
+    return this;
+  }
+
+  /**
+   * Enables connection to an SSL-protected RMI registry
+   *
+   * @return this
+   */
+  @CanIgnoreReturnValue
+  public JmxScraperContainer withSslRmiRegistry() {
+    this.sslRmiRegistry = true;
+    return this;
+  }
+
   @Override
   public void start() {
     // for now only configure through JVM args
@@ -144,6 +208,13 @@ public void start() {
       arguments.add("-Dotel.jmx.password=" + password);
     }
 
+    arguments.addAll(addSecureStore(keyStore, /* isKeyStore= */ true));
+    arguments.addAll(addSecureStore(trustStore, /* isKeyStore= */ false));
+
+    if (sslRmiRegistry) {
+      arguments.add("-Dotel.jmx.remote.registry.ssl=true");
+    }
+
     if (!customYamlFiles.isEmpty()) {
       for (String yaml : customYamlFiles) {
         this.withCopyFileToContainer(MountableFile.forClasspathResource(yaml), yaml);
@@ -177,4 +248,17 @@ public void start() {
 
     super.start();
   }
+
+  private List<String> addSecureStore(TestKeyStore keyStore, boolean isKeyStore) {
+    if (keyStore == null) {
+      return Collections.emptyList();
+    }
+    Path path = keyStore.getPath();
+    String containerPath = "/" + path.getFileName().toString();
+    this.withCopyFileToContainer(MountableFile.forHostPath(path), containerPath);
+
+    String prefix = String.format("-Djavax.net.ssl.%sStore", isKeyStore ? "key" : "trust");
+    return Arrays.asList(
+        prefix + "=" + containerPath, prefix + "Password=" + keyStore.getPassword());
+  }
 }