TLS: allow protocol allowlist; harden JVM SSL init by shivam-startree · Pull Request #17776 · apache/pinot

shivam-startree · 2026-02-26T18:01:29Z

Prevent JVM startup failure when JvmDefaultSslContext initialization throws (log warning and continue; components configure TLS during their own startup).
Add configurable TLS protocol allowlist (tls..protocols) and apply it to Netty client/server SSL contexts.
Extend TlsConfig with allowedProtocols, fix copy-constructor to include _insecure, and update equals/hashCode.
Use KeyStore.getDefaultType() as Kafka truststore default instead of hardcoded "jks".

xiangfu0 · 2026-02-27T17:12:28Z

+    return result;
  }
-}
+}


There should be a newline at the file end

xiangfu0 · 2026-02-27T17:17:29Z

  private static final String DEFAULT_KEYSTORE_TYPE = "PKCS12";
  private static final String DEFAULT_SECURITY_PROTOCOL = "SSL";
-  private static final String DEFAULT_TRUSTSTORE_TYPE = "jks";
+  private static final String DEFAULT_TRUSTSTORE_TYPE = KeyStore.getDefaultType();


Please add comments.

Would this introduce backward compatibility for existing users?

On standard JDKs, KeyStore.getDefaultType() returns "jks" (the same value we had hardcoded), so existing users see identical behavior. The only difference is that JVMs configured with a different default keystore type (via java.security properties) will now get the correct type automatically instead of being forced to "jks". Users can still override this via explicit Kafka SSL configuration properties as before.

xiangfu0 · 2026-02-27T17:36:40Z

+    // Read allowed TLS protocols from config (e.g., "TLSv1.2,TLSv1.3")
+    String protocolsConfig = pinotConfig.getProperty(key(namespace, PROTOCOLS));
+    if (protocolsConfig != null && !protocolsConfig.isEmpty()) {
+      tlsConfig.setAllowedProtocols(protocolsConfig.split(","));


Better trim the string after split?

xiangfu0 · 2026-02-27T17:36:57Z

      // HttpsURLConnection
      HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory());
      setSslContext(sc);
-      logTlsDiagnosticsOnce("https.default", sc, null, false);


Why removed this?

Copilot

Pull request overview

This PR enhances TLS/SSL configuration in Apache Pinot by adding protocol allowlist support and improving JVM startup reliability. The changes prevent JVM startup failures when SSL context initialization fails (logging warnings instead), add configurable TLS protocol restrictions, and fix several bugs in the TlsConfig class.

Changes:

Added configurable TLS protocol allowlist (tls.<namespace>.protocols) that can restrict TLS versions
Prevented JVM startup failures by catching SSL initialization errors in JvmDefaultSslContext and logging warnings instead
Fixed TlsConfig copy constructor to include the _insecure field and added _allowedProtocols field with proper equals/hashCode implementation
Changed Kafka truststore default from hardcoded "jks" to KeyStore.getDefaultType() for better portability

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
pinot-plugins/pinot-stream-ingestion/pinot-kafka-base/src/main/java/org/apache/pinot/plugin/stream/kafka/KafkaSSLUtils.java	Changed default truststore type from hardcoded "jks" to KeyStore.getDefaultType() for better platform compatibility
pinot-common/src/main/java/org/apache/pinot/common/config/TlsConfig.java	Added allowedProtocols field with proper getters/setters, fixed copy constructor to include _insecure, and updated equals/hashCode methods
pinot-common/src/main/java/org/apache/pinot/common/utils/tls/TlsUtils.java	Added parsing of protocol allowlist from configuration and applied it to Netty client/server SSL contexts
pinot-common/src/main/java/org/apache/pinot/common/utils/tls/JvmDefaultSslContext.java	Wrapped SSL initialization in try-catch to prevent JVM startup failures, added detailed documentation, and fixed typo in log message

Copilot · 2026-02-27T17:39:17Z

+    // Read allowed TLS protocols from config (e.g., "TLSv1.2,TLSv1.3")
+    String protocolsConfig = pinotConfig.getProperty(key(namespace, PROTOCOLS));
+    if (protocolsConfig != null && !protocolsConfig.isEmpty()) {
+      tlsConfig.setAllowedProtocols(protocolsConfig.split(","));


The protocol strings should be trimmed after splitting to handle whitespace in configuration (e.g., "TLSv1.2, TLSv1.3" vs "TLSv1.2,TLSv1.3"). Consider using a pattern like:

String[] protocols = Arrays.stream(protocolsConfig.split(",")) .map(String::trim) .filter(s -> !s.isEmpty()) .toArray(String[]::new); tlsConfig.setAllowedProtocols(protocols);

This follows the established pattern in the codebase, as seen in AuditRequestProcessor.parseAllowedHeaders.

Suggested change

tlsConfig.setAllowedProtocols(protocolsConfig.split(","));

String[] protocols = Arrays.stream(protocolsConfig.split(","))

.map(String::trim)

.filter(s -> !s.isEmpty())

.toArray(String[]::new);

tlsConfig.setAllowedProtocols(protocols);

Copilot · 2026-02-27T17:39:18Z

+  @Nullable
+  public String[] getAllowedProtocols() {
+    if (_allowedProtocols != null) {
+      return Arrays.copyOf(_allowedProtocols, _allowedProtocols.length);
+    }
+    return null;
+  }
+
+  public void setAllowedProtocols(@Nullable String[] allowedProtocols) {
+    _allowedProtocols = allowedProtocols != null
+        ? Arrays.copyOf(allowedProtocols, allowedProtocols.length)
+        : null;
+  }


The new functionality for configuring allowed TLS protocols lacks test coverage. Consider adding tests that verify:

Protocol allowlist is correctly parsed from configuration (e.g., "TLSv1.2,TLSv1.3")

The allowlist is applied to both client and server SSL contexts

Whitespace handling in protocol strings (e.g., "TLSv1.2, TLSv1.3")

Edge cases like empty strings or null values

A test file such as pinot-common/src/test/java/org/apache/pinot/common/config/TlsConfigTest.java would be appropriate to verify the copy constructor, equals, hashCode, and the new allowedProtocols field.

Copilot · 2026-02-27T17:39:18Z

+    try {
+      initDefaultSslContextInternal();
+    } catch (Exception e) {
+      // Do NOT rethrow — this runs in PinotAdministrator's static initializer.
+      // Rethrowing would cause ExceptionInInitializerError and prevent the JVM from starting.
+      // Pinot components re-initialize TLS individually during their own startup.
+      LOGGER.warn("Failed to initialize JVM default SSL context. This is non-fatal — Pinot components "
+          + "will configure TLS individually during startup. trustStoreType='{}', trustStore='{}'. Error: {}",
+          System.getProperty(JVM_TRUST_STORE_TYPE), System.getProperty(JVM_TRUST_STORE), e.getMessage());
+      LOGGER.debug("Full stack trace for SSL context initialization failure:", e);
+    }
+    _initialized = true;
+  }


The new error handling logic that prevents JVM startup failure lacks test coverage. Consider adding a test that verifies:

When SSL context initialization fails, a warning is logged but the method completes normally

The _initialized flag is still set to true even when initialization fails

Subsequent calls to initDefaultSslContext() are properly short-circuited

This is a critical change for reliability, and automated tests would help ensure the behavior is maintained in future refactorings.

codecov-commenter · 2026-02-27T18:16:38Z

Codecov Report

❌ Patch coverage is 55.55556% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.20%. Comparing base (7250717) to head (d3a9077).
⚠️ Report is 13 commits behind head on master.

Files with missing lines	Patch %	Lines
...va/org/apache/pinot/common/utils/tls/TlsUtils.java	40.00%	9 Missing and 3 partials ⚠️
...java/org/apache/pinot/common/config/TlsConfig.java	54.54%	0 Missing and 10 partials ⚠️
...e/pinot/common/utils/tls/JvmDefaultSslContext.java	81.81%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #17776      +/-   ##
============================================
- Coverage     63.22%   63.20%   -0.02%     
  Complexity     1454     1454              
============================================
  Files          3179     3183       +4     
  Lines        191310   191476     +166     
  Branches      29263    29285      +22     
============================================
+ Hits         120950   121021      +71     
- Misses        60918    60996      +78     
- Partials       9442     9459      +17

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`63.18% <55.55%> (+<0.01%)`	⬆️
java-21	`63.18% <55.55%> (-0.01%)`	⬇️
temurin	`63.20% <55.55%> (-0.02%)`	⬇️
unittests	`63.20% <55.55%> (-0.02%)`	⬇️
unittests1	`55.62% <49.05%> (+<0.01%)`	⬆️
unittests2	`34.12% <18.51%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Copilot · 2026-02-28T10:04:23Z

    }
-    _initialized = true;
-    LOGGER.info("Successfully initialized mvm default SSL context");
+    LOGGER.info("Successfully initialized JVM default SSL context");
  }


LOGGER.info("Successfully initialized JVM default SSL context") is logged unconditionally even when the method didn’t actually set a new default SSLContext (e.g., when neither javax.net.ssl.keyStore nor javax.net.ssl.trustStore is provided, or when non-file schemes are used). Consider logging success only when initialization was performed, and log a different message (or nothing) when the method no-ops.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Copilot · 2026-02-28T10:13:00Z

+import org.testng.annotations.Test;
+
+public class JvmDefaultSslContextTest {
+  private static final String JVM_TRUST_STORE = "javax.net.ssl.trustStore";
+
+  @AfterMethod
+  public void cleanup() throws Exception {
+    System.clearProperty(JVM_TRUST_STORE);
+    setInitialized(false);


This test clears javax.net.ssl.trustStore and forces JvmDefaultSslContext._initialized=false after each method. If the test JVM was started with an existing trustStore system property (or if other tests in the same JVM rely on it), this can cause cross-test interference. Consider capturing the previous property value / initialized state in @BeforeMethod and restoring them in @AfterMethod instead of always clearing/resetting.

Suggested change

import org.testng.annotations.Test;

public class JvmDefaultSslContextTest {

private static final String JVM_TRUST_STORE = "javax.net.ssl.trustStore";

@AfterMethod

public void cleanup() throws Exception {

System.clearProperty(JVM_TRUST_STORE);

setInitialized(false);

import org.testng.annotations.BeforeMethod;

import org.testng.annotations.Test;

public class JvmDefaultSslContextTest {

private static final String JVM_TRUST_STORE = "javax.net.ssl.trustStore";

private String _originalTrustStore;

private boolean _originalInitialized;

@BeforeMethod

public void setUp() throws Exception {

_originalTrustStore = System.getProperty(JVM_TRUST_STORE);

_originalInitialized = isInitialized();

}

@AfterMethod

public void cleanup() throws Exception {

if (_originalTrustStore != null) {

System.setProperty(JVM_TRUST_STORE, _originalTrustStore);

} else {

System.clearProperty(JVM_TRUST_STORE);

}

setInitialized(_originalInitialized);

Copilot · 2026-02-28T10:13:01Z

  private static final String DEFAULT_KEYSTORE_TYPE = "PKCS12";
  private static final String DEFAULT_SECURITY_PROTOCOL = "SSL";
-  private static final String DEFAULT_TRUSTSTORE_TYPE = "jks";
+  // Follow the JVM default keystore type (typically "jks") unless explicitly configured.


The comment says the JVM default keystore type is "typically "jks"", but on modern JDKs (including Java 11) KeyStore.getDefaultType() is commonly "pkcs12". To avoid misleading operators, consider rewording this comment to be version-agnostic (e.g., "depends on JVM 'keystore.type'"), or mention both common defaults.

Suggested change

// Follow the JVM default keystore type (typically "jks") unless explicitly configured.

// Follow the JVM default keystore type (from 'keystore.type', commonly "jks" or "pkcs12") unless explicitly configured.

Copilot · 2026-02-28T10:13:01Z

              .map(String::trim).filter(StringUtils::isNotBlank).orElse(null);
      RenewableTlsUtils.enableAutoRenewalFromFileStoreForSSLFactory(jvmSslFactory, jvmKeystoreType, jvmKeyStorePath,
          jvmKeystorePassword, jvmTrustStoreType, jvmTrustStorePath, jvmTrustStorePassword, null, null, () -> false);


The RenewableTlsUtils.enableAutoRenewalFromFileStoreForSSLFactory(...) call uses jvmKeystoreType, but jvmKeystoreType is currently derived from javax.net.ssl.trustStoreType (same property used for jvmTrustStoreType) instead of javax.net.ssl.keyStoreType. This can ignore an explicitly configured keystore type and cause SSLFactory identity material loading to fail when keystore/truststore types differ.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

xiangfu0

lgtm

betavirus777 added 2 commits February 26, 2026 22:59

Made TLS configurable

c5991c0

Made TLS configurable

8e409b3

shivam-startree changed the title ~~Tls configurations changes~~ TLS: allow protocol allowlist; harden JVM SSL init Feb 26, 2026

xiangfu0 requested a review from Copilot February 27, 2026 17:35

Copilot started reviewing on behalf of xiangfu0 February 27, 2026 17:36 View session

xiangfu0 reviewed Feb 27, 2026

View reviewed changes

Copilot AI reviewed Feb 27, 2026

View reviewed changes

Address PR feedback: trim protocols, restore diagnostics, add tests

0715f95

shivam-startree requested a review from Copilot February 28, 2026 10:01

Copilot started reviewing on behalf of shivam-startree February 28, 2026 10:01 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

Address PR feedback: trim protocols, restore diagnostics, add tests

b62e355

shivam-startree requested a review from Copilot February 28, 2026 10:09

Copilot started reviewing on behalf of shivam-startree February 28, 2026 10:10 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

xiangfu0 requested review from Copilot and xiangfu0 February 28, 2026 20:05

Copilot started reviewing on behalf of xiangfu0 February 28, 2026 20:06 View session

Copilot AI reviewed Feb 28, 2026

View reviewed changes

Address PR feedback: trim protocols, restore diagnostics, add tests

d3a9077

xiangfu0 approved these changes Feb 28, 2026

View reviewed changes

xiangfu0 merged commit 6c5d200 into apache:master Feb 28, 2026
15 of 16 checks passed

xiangfu0 added kafka Related to Kafka stream connector plugins Related to the plugin system labels Mar 20, 2026

-      tlsConfig.setAllowedProtocols(protocolsConfig.split(","));
+      String[] protocols = Arrays.stream(protocolsConfig.split(","))
+          .map(String::trim)
+          .filter(s -> !s.isEmpty())
+          .toArray(String[]::new);
+      tlsConfig.setAllowedProtocols(protocols);

	// Follow the JVM default keystore type (typically "jks") unless explicitly configured.
	// Follow the JVM default keystore type (from 'keystore.type', commonly "jks" or "pkcs12") unless explicitly configured.

Conversation

shivam-startree commented Feb 26, 2026

Uh oh!

xiangfu0 Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

shivam-startree Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

xiangfu0 Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

xiangfu0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Feb 27, 2026 •

edited

Loading