Jackie-Jiang commented on code in PR #19828:
URL: https://github.com/apache/pinot/pull/19828#discussion_r4275868251
##########
pinot-tools/src/main/java/org/apache/pinot/tools/admin/PinotAdministrator.java:
##########
@@ -94,6 +95,7 @@ public class PinotAdministrator {
private static final Map<String, Command> SUBCOMMAND_MAP = new HashMap<>();
static {
+ ZooKeeperSslContextSupplier.initialize();
Review Comment:
[P2, non-blocking] Initialize the separate tools launcher as well
This covers PinotAdministrator, but the separately packaged
PinotToolLauncher never calls the adapter. For example, `pinot-tools
ValidateTableRetention` calls `ValidateTableRetention.execute()`, which
constructs `TableRetentionValidator` and immediately creates a `ZKHelixAdmin`.
`UpdateSegmentState` and `AutoAddInvertedIndex` have the same gap. With secure
ZooKeeper, `zookeeper.ssl.hostnameVerification=false`, and a trusted
certificate whose identity differs from the connection address, these commands
still take ZooKeeper's ordinary Netty builder path and fail the handshake after
this upgrade. Please initialize the adapter in that launcher before client
construction. The standalone PerfBenchmarkDriver path against an existing
cluster also needs the same guard.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/tls/ZooKeeperSslContextSupplier.java:
##########
@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.common.utils.tls;
+
+import java.util.Properties;
+import java.util.function.Supplier;
+import javax.net.ssl.SSLContext;
+import org.apache.zookeeper.common.ClientX509Util;
+import org.apache.zookeeper.common.X509Exception.SSLContextException;
+import org.apache.zookeeper.common.ZKConfig;
+
+
+/// Preserves disabled ZooKeeper hostname verification with Netty 4.2 using
ZooKeeper's JDK context supplier hook.
+///
+/// Pinot's Helix clients use process-wide ZooKeeper TLS bootstrap properties.
Call [#initialize()] before creating
+/// these clients; existing custom suppliers retain ownership of their context
and provider. The vendor's no-argument
+/// supplier cannot consume arbitrary per-client TLS overrides, so it is
intended for these bootstrap-owned clients.
+/// Each context uses ZooKeeper's configured stores, protocol, revocation and
FIPS policy, without caching key material.
+public class ZooKeeperSslContextSupplier implements Supplier<SSLContext> {
+ /// Installs the JDK adapter only when bootstrap configuration explicitly
disables hostname verification.
+ /// Enabled verification and custom suppliers keep their existing
construction paths. TLS bootstrap configuration
+ /// must be supplied before client creation; it cannot be obtained from the
cluster being connected to.
+ public static void initialize() {
+ Properties properties = System.getProperties();
+ synchronized (properties) {
+ try (ClientX509Util util = new ClientX509Util()) {
+ ZKConfig config = new ZKConfig();
+ if (util.isServerHostnameVerificationEnabled(config)
+ || config.getProperty(util.getSslContextSupplierClassProperty())
!= null) {
+ return;
+ }
+ // ZooKeeper 3.9.6 supports supplied SSLContext instances only with
its JDK provider.
+ properties.setProperty(util.getSslProviderProperty(), "JDK");
Review Comment:
[P2, non-blocking] Preserve existing OpenSSL cipher-name compatibility
When hostname verification is disabled, this overwrites an explicit
`zookeeper.ssl.sslProvider=OPENSSL` or `OPENSSL_REFCNT` with JDK while leaving
`zookeeper.ssl.ciphersuites` unchanged. A previously valid value such as
`ECDHE-RSA-AES128-GCM-SHA256` is accepted by Netty's OpenSSL path, but
ZooKeeper's supplied-context path passes it unchanged through
`IdentityCipherSuiteFilter` to `SSLEngine.setEnabledCipherSuites()`. SunJSSE
rejects that spelling with an unsupported-cipher exception, preventing
ZooKeeper connections; its equivalent is
`TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256`. Please preserve provider compatibility
or translate configured cipher names as part of the JDK migration, and cover
this case in the regression test. The current handshake tests explicitly select
JDK and do not exercise the migration.
Source references: [Netty OpenSSL cipher-name
support](https://netty.io/4.2/api/io/netty/handler/ssl/OpenSsl.html#isCipherSuiteAvailable(java.lang.String)),
[JDK engine
configuration](https://netty.io/4.2/xref/io/netty/handler/ssl/JdkSslContext.html#365).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
