Re: [PR] feat: support open search client cert auth [skywalking]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #13641:
URL: https://github.com/apache/skywalking/pull/13641#discussion_r2659696838


##########
oap-server/server-starter/src/main/resources/application.yml:
##########
@@ -160,6 +160,8 @@ storage:
     password: ${SW_ES_PASSWORD:""}
     trustStorePath: ${SW_STORAGE_ES_SSL_JKS_PATH:""}
     trustStorePass: ${SW_STORAGE_ES_SSL_JKS_PASS:""}
+    keyStorePath: ${SW_STORAGE_ES_SSL_KEY_STORE_PATH:""} # Path to client certificate keystore for mutual TLS (OpenSearch/Elasticsearch client cert auth). Supports PKCS12 (.p12, .pfx) and JKS (.jks) formats.
+    keyStorePass: ${SW_STORAGE_ES_SSL_KEY_STORE_PASS:""} # Password for the client certificate keystore. Can be managed via secretsManagementFile.
     secretsManagementFile: ${SW_ES_SECRETS_MANAGEMENT_FILE:""} # Secrets management file in the properties format includes the username, password, which are managed by 3rd party tool.

Review Comment:
   The new keyStorePath and keyStorePass configuration options for client certificate authentication are not documented. The existing elasticsearch.md documentation covers trustStorePath but does not include the new mutual TLS configuration options. Users need documentation explaining how to configure client certificate authentication for OpenSearch/Elasticsearch.
   ```suggestion
       trustStorePath: ${SW_STORAGE_ES_SSL_JKS_PATH:""} # Path to the truststore that contains CA certificates used to verify the OpenSearch/Elasticsearch server certificate.
       trustStorePass: ${SW_STORAGE_ES_SSL_JKS_PASS:""} # Password for the truststore defined in trustStorePath.
       keyStorePath: ${SW_STORAGE_ES_SSL_KEY_STORE_PATH:""} # Path to the client certificate keystore for mutual TLS (OpenSearch/Elasticsearch client certificate authentication). Supports PKCS12 (.p12, .pfx) and JKS (.jks) formats.
       keyStorePass: ${SW_STORAGE_ES_SSL_KEY_STORE_PASS:""} # Password for the client certificate keystore defined in keyStorePath. This can be externalized via secretsManagementFile.
       secretsManagementFile: ${SW_ES_SECRETS_MANAGEMENT_FILE:""} # Secrets management file in properties format (e.g. username, password, keystore passwords) managed by a 3rd party tool.
   ```



##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/ElasticSearchBuilder.java:
##########
@@ -116,6 +126,18 @@ public ElasticSearchBuilder trustStorePass(String trustStorePass) {
         return this;
     }
 
+    public ElasticSearchBuilder keyStorePath(String keyStorePath) {
+        requireNonNull(keyStorePath, "keyStorePath");
+        this.keyStorePath = keyStorePath;
+        return this;
+    }
+
+    public ElasticSearchBuilder keyStorePass(String keyStorePass) {
+        requireNonNull(keyStorePass, "keyStorePass");
+        this.keyStorePass = keyStorePass;
+        return this;
+    }

Review Comment:
   The keyStorePass and trustStorePass builder methods use requireNonNull which prevents passing null values. However, in ElasticSearchClient.connect(), the code checks if these values are null before calling the builder methods. This inconsistency could cause issues if someone tries to set an empty password. The builder should allow null values and handle password-less keystores, or the validation logic should be consistent throughout.



##########
oap-server/server-library/library-elasticsearch-client/src/main/java/org/apache/skywalking/library/elasticsearch/ElasticSearchBuilder.java:
##########
@@ -157,19 +179,62 @@ public ElasticSearch build() {
                          .connectTimeout(connectTimeout)
                          .idleTimeout(socketTimeout)
                          .useHttp2Preface(false)
-                         .workerGroup(numHttpClientThread > 0 ? numHttpClientThread : NUM_PROC);
-
-        if (StringUtil.isNotBlank(trustStorePath)) {
-            final TrustManagerFactory trustManagerFactory =
-                TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
-            final KeyStore truststore = KeyStore.getInstance("jks");
-            try (final InputStream is = Files.newInputStream(Paths.get(trustStorePath))) {
-                truststore.load(is, trustStorePass.toCharArray());
-            }
-            trustManagerFactory.init(truststore);
-
-            factoryBuilder.tlsCustomizer(
-                sslContextBuilder -> sslContextBuilder.trustManager(trustManagerFactory));
+                         .workerGroup(new NioEventLoopGroup(numHttpClientThread > 0 ? numHttpClientThread : NUM_PROC), true);

Review Comment:
   The workerGroup is now instantiated with a new NioEventLoopGroup and set to shutdown on close (second parameter true). However, the previous implementation used the integer value directly. This change means that every ElasticSearch client instance will create its own event loop group, which could lead to resource exhaustion if multiple clients are created. Consider whether this EventLoopGroup should be shared across clients or if there's proper lifecycle management to prevent resource leaks.



##########
test/e2e-v2/cases/storage/opensearch/docker-compose.yml:
##########
@@ -42,15 +107,24 @@ services:
       SW_STORAGE: elasticsearch
       SW_STORAGE_ES_CLUSTER_NODES: opensearch:9200
       SW_ES_USER: admin
-      SW_ES_PASSWORD: admin
+      SW_ES_PASSWORD: SecurePass@2024!
+      SW_STORAGE_ES_HTTP_PROTOCOL: https
+      SW_STORAGE_ES_SSL_JKS_PATH: /etc/skywalking/certs/truststore.jks
+      SW_STORAGE_ES_SSL_JKS_PASS: changeit
+      SW_STORAGE_ES_SSL_KEY_STORE_PATH: /etc/skywalking/certs/client.p12
+      SW_STORAGE_ES_SSL_KEY_STORE_PASS: changeit
+      JAVA_OPTS: -Dcom.linecorp.armeria.useEpoll=false # TODO: fix this

Review Comment:
   This TODO comment indicates an incomplete fix for an Armeria epoll issue. While TODOs in test configuration are acceptable if tracked, this should reference a corresponding issue number or have a plan for resolution to ensure it doesn't remain indefinitely.
   ```suggestion
         JAVA_OPTS: -Dcom.linecorp.armeria.useEpoll=false # Epoll disabled in e2e tests due to known instability on some hosts
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org