Copilot commented on code in PR #17358: URL: https://github.com/apache/pinot/pull/17358#discussion_r2686336617
########## pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/DefaultSslContextProvider.java: ########## @@ -0,0 +1,54 @@ +/** + * 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.client; + +import javax.annotation.Nullable; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import org.asynchttpclient.DefaultAsyncHttpClientConfig; + + +/** + * Default SSL context provider that uses the JDK/BCJSSE stack and disables OpenSSL. + */ +public class DefaultSslContextProvider implements SslContextProvider { + + @Override + public DefaultAsyncHttpClientConfig.Builder configure(DefaultAsyncHttpClientConfig.Builder builder, + @Nullable SSLContext sslContext, TlsProtocols tlsProtocols) { + builder.setUseOpenSsl(false); + + String[] enabledProtocols = + tlsProtocols != null ? tlsProtocols.getEnabledProtocols().toArray(new String[0]) : new String[0]; Review Comment: The null check on `tlsProtocols` does not prevent a potential NPE when calling `getEnabledProtocols()` if `getEnabledProtocols()` returns null. Consider guarding against null returns from the method call, or ensure that the `TlsProtocols.getEnabledProtocols()` contract guarantees a non-null list. ########## pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/MailboxService.java: ########## @@ -192,4 +205,120 @@ private static Duration getIdleTimeout(PinotConfiguration config) { // Use a reasonable maximum idle timeout (1 year) to avoid overflow. return Duration.ofDays(365); } + + @Nullable + private static SslContext resolveClientSslContext(InstanceType instanceType, @Nullable TlsConfig tlsConfig) { + if (tlsConfig == null) { + return null; + } + switch (instanceType) { + case BROKER: + case SERVER: + case CONTROLLER: + return getOrBuildClientSslContext(instanceType, tlsConfig); + default: + return ServerGrpcQueryClient.buildSslContext(tlsConfig); + } + } + + @Nullable + private static SslContext resolveServerSslContext(InstanceType instanceType, @Nullable TlsConfig tlsConfig) { + if (tlsConfig == null) { + return null; + } + switch (instanceType) { + case BROKER: + case SERVER: + case CONTROLLER: + return getOrBuildServerSslContext(instanceType, tlsConfig); + default: + return GrpcQueryServer.buildGrpcSslContext(tlsConfig); + } + } + + private static SslContext getOrBuildClientSslContext(InstanceType instanceType, TlsConfig tlsConfig) { + switch (instanceType) { + case BROKER: + return getOrBuildBrokerClientSslContext(tlsConfig); + case SERVER: + return getOrBuildServerClientSslContext(tlsConfig); + case CONTROLLER: + return getOrBuildControllerClientSslContext(tlsConfig); + default: + return ServerGrpcQueryClient.buildSslContext(tlsConfig); + } + } + + private static SslContext getOrBuildServerSslContext(InstanceType instanceType, TlsConfig tlsConfig) { + switch (instanceType) { + case BROKER: + return getOrBuildBrokerServerSslContext(tlsConfig); + case SERVER: + return getOrBuildServerServerSslContext(tlsConfig); + case CONTROLLER: + return getOrBuildControllerServerSslContext(tlsConfig); + default: + return GrpcQueryServer.buildGrpcSslContext(tlsConfig); + } + } Review Comment: The methods `getOrBuildClientSslContext` and `getOrBuildServerSslContext` contain identical switch structures that delegate to instance-type-specific methods. This pattern repeats in six helper methods (lines 239-323) with only the method names differing. Consider consolidating this logic using a strategy pattern or method references to reduce duplication and improve maintainability. ########## pinot-common/src/main/java/org/apache/pinot/common/utils/ClientSSLContextGenerator.java: ########## @@ -95,24 +108,25 @@ private TrustManager[] setupTrustManagers() // trustStore. if (_serverCACertFile != null) { LOGGER.info("Initializing trust store from {}", _serverCACertFile); - FileInputStream is = new FileInputStream(new File(_serverCACertFile)); KeyStore trustStore = KeyStore.getInstance(KeyStore.getDefaultType()); trustStore.load(null); - CertificateFactory certificateFactory = CertificateFactory.getInstance(CERTIFICATE_TYPE); - int i = 0; - while (is.available() > 0) { - X509Certificate cert = (X509Certificate) certificateFactory.generateCertificate(is); - LOGGER.info("Read certificate serial number {} by issuer {} ", cert.getSerialNumber().toString(16), - cert.getIssuerDN().toString()); + try (FileInputStream is = new FileInputStream(new File(_serverCACertFile))) { Review Comment: The FileInputStream is wrapped in a try-with-resources, which is good. However, the file reading loop starting at line 116 does not handle incomplete certificate reads. If `is.available()` returns 0 before a complete certificate is read, the loop terminates, potentially leaving partial data. Consider using `CertificateFactory.generateCertificates()` to handle multiple certificates more robustly. ########## pinot-clients/pinot-java-client/src/test/java/org/apache/pinot/client/SslContextProviderTest.java: ########## @@ -0,0 +1,196 @@ +/** + * 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.client; + +import java.lang.reflect.Constructor; +import java.net.URL; +import java.net.URLClassLoader; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.List; +import javax.annotation.Nullable; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import org.asynchttpclient.DefaultAsyncHttpClientConfig; +import org.asynchttpclient.Dsl; +import org.asynchttpclient.SslEngineFactory; +import org.testng.annotations.Test; + +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertTrue; + + +public class SslContextProviderTest { + private static final String PROVIDER_PROPERTY = "pinot.client.sslContextProvider"; + + @Test + public void testDefaultProviderConfiguresProtocolsAndDisablesOpenSsl() + throws Exception { + DefaultAsyncHttpClientConfig.Builder builder = Dsl.config(); + SSLContext probeContext = SSLContext.getInstance("TLS"); + probeContext.init(null, null, null); + String protocol = selectSupportedProtocol(probeContext); + TlsProtocols tlsProtocols = tlsProtocolsWith(protocol); + + SslContextProvider provider = new DefaultSslContextProvider(); + provider.configure(builder, null, tlsProtocols); + + DefaultAsyncHttpClientConfig config = builder.build(); + assertFalse(config.isUseOpenSsl()); + assertEquals(config.getEnabledProtocols(), new String[] {protocol}); + } + + @Test + public void testDefaultProviderCreatesClientSslEngine() + throws Exception { + DefaultAsyncHttpClientConfig.Builder builder = Dsl.config(); + + SSLContext sslContext = SSLContext.getInstance("TLS"); + sslContext.init(null, null, null); + String protocol = selectSupportedProtocol(sslContext); + + SslContextProvider provider = new DefaultSslContextProvider(); + provider.configure(builder, sslContext, tlsProtocolsWith(protocol)); + + DefaultAsyncHttpClientConfig config = builder.build(); + assertFalse(config.isUseOpenSsl()); + assertEquals(config.getEnabledProtocols(), new String[] {protocol}); + + SslEngineFactory engineFactory = config.getSslEngineFactory(); + assertNotNull(engineFactory); + SSLEngine engine = engineFactory.newSslEngine(config, "localhost", 443); + assertTrue(engine.getUseClientMode()); + assertEquals(engine.getEnabledProtocols(), new String[] {protocol}); + } + + @Test + public void testFactoryUsesSystemPropertyProvider() { + String originalProperty = System.getProperty(PROVIDER_PROPERTY); + System.setProperty(PROVIDER_PROPERTY, PropertyProvider.class.getName()); + try { + SslContextProvider provider = SslContextProviderFactory.create(); + assertTrue(provider instanceof PropertyProvider); + } finally { + restoreProperty(originalProperty); + } + } + + @Test + public void testFactoryUsesServiceLoaderProviderWhenPropertyMissing() + throws Exception { + String originalProperty = System.getProperty(PROVIDER_PROPERTY); + restoreProperty(null); + try { + SslContextProvider provider = createFromServiceLoader(ServiceLoaderProvider.class); + assertTrue(provider instanceof ServiceLoaderProvider); + } finally { + restoreProperty(originalProperty); + } + } + + @Test + public void testFactoryAcceptsDefaultProviderSubclassFromServiceLoader() + throws Exception { + String originalProperty = System.getProperty(PROVIDER_PROPERTY); + restoreProperty(null); + try { + SslContextProvider provider = createFromServiceLoader(DefaultProviderSubclass.class); + assertTrue(provider instanceof DefaultProviderSubclass); + } finally { + restoreProperty(originalProperty); + } + } + + @Test + public void testFactoryFallsBackToDefaultWhenPropertyInvalidAndNoServiceLoader() { + String originalProperty = System.getProperty(PROVIDER_PROPERTY); + System.setProperty(PROVIDER_PROPERTY, String.class.getName()); + try { + SslContextProvider provider = SslContextProviderFactory.create(); + assertTrue(provider instanceof DefaultSslContextProvider); + } finally { + restoreProperty(originalProperty); + } + } + + private static void restoreProperty(@Nullable String value) { + if (value == null) { + System.clearProperty(PROVIDER_PROPERTY); + } else { + System.setProperty(PROVIDER_PROPERTY, value); + } + } + + private static SslContextProvider createFromServiceLoader(Class<? extends SslContextProvider> providerClass) + throws Exception { + ClassLoader originalClassLoader = Thread.currentThread().getContextClassLoader(); + + Path tempDir = Files.createTempDirectory("pinot-ssl-provider"); + Path servicesDir = tempDir.resolve("META-INF").resolve("services"); + Files.createDirectories(servicesDir); + Path serviceFile = servicesDir.resolve(SslContextProvider.class.getName()); + Files.writeString(serviceFile, providerClass.getName(), StandardCharsets.UTF_8); + + try (URLClassLoader loader = + new URLClassLoader(new URL[] {tempDir.toUri().toURL()}, SslContextProviderTest.class.getClassLoader())) { Review Comment: The test creates a temporary directory (`tempDir`) at line 148 but does not clean it up after the test completes. Consider adding cleanup logic in a `finally` block or using `@AfterMethod` to ensure the temporary directory is deleted. -- 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]
