Fix stream transport TLS cert hot-reload by using live SSLContext fro…

[rishabhmaurya]

…m SecureTransportSettingsProvider

Description

Stream transport failed after reloadcerts with "cert file read errors". The original DefaultSslContextProvider built JdkSslContext using KeyManagerFactory/TrustManagerFactory obtained from SslConfiguration, which reads cert files from disk.It can happen that some implementation calls reloadcerts which rebuild the in-memory SSLContext and then delete the cert files. Any new connection after deletion triggers a fresh JdkSslContext build, which fails because the files no longer exist.

Why SecureTransportSettingsProvider needed a new method
transport-netty4 avoids this by calling buildSecureServerTransportEngine() / buildSecureClientTransportEngine() per connection — these return engines from the live in-memory SSLContext with no file reads. However, Arrow Flight uses gRPC-netty's ServerTlsHandler / ClientTlsHandler, which require a SslContext object (not a raw SSLEngine). The correct pattern (used by transport-grpc) is to wrap the live javax.SSLContext once in a JdkSslContext — JdkSslContext holds a reference to the SSLContext and calls createSSLEngine() on it per connection, so re-initializing the SSLContext with new key material takes effect on the next handshake automatically.

transport-grpc gets the live SSLContext via SecureAuxTransportSettingsProvider.buildSecureAuxServerTransportContext(), but Arrow Flight is a Transport (not AuxTransport) and receives SecureTransportSettingsProvider, which had no equivalent method. Adding buildSecureTransportContext() (default returns Optional.empty()) exposes the live SSLContext through the same interface Arrow Flight already has.

Security plugin change
The security plugin implements buildSecureTransportContext() to return the live SSLContext from SslContextHandler — one line, identical to how buildSecureAuxServerTransportContext() works. See: opensearch-project/security#5971

Hostname verification and ALPN
With the client using JdkSslContext directly, gRPC-netty ClientTlsHandler behaviors required additional handling:

1. ALPN: ClientTlsHandler reads getSSLParameters(), adds endpointIdentificationAlgorithm, then calls setSSLParameters(). ALPN must already be set on the engine before this round-trip, so AlpnPresettingClientSslContext pre-sets it in newEngine().

2. Hostname verification: ClientTlsHandler unconditionally sets endpointIdentificationAlgorithm="HTTPS" after newEngine() returns, regardless of the SslContext configuration. When enforce_hostname_verification=false, this causes handshake failures if the server cert lacks the peer IP as a SAN. AlpnAwareSSLEngineWrapper intercepts setSSLParameters() and strips the algorithm. It must live in the io.netty.handler.ssl package to implement ApplicationProtocolAccessor (package-private in Netty), which is required for SslHandler.applicationProtocol() to return the negotiated protocol post-handshake

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 0a82204.

Path	Line	Severity	Description
plugins/arrow-flight-rpc/src/main/java/io/netty/handler/ssl/AlpnAwareSSLEngineWrapper.java	47	critical	Unconditionally strips endpointIdentificationAlgorithm by setting it to empty string, completely disabling hostname verification in TLS connections. This enables trivial man-in-the-middle attacks when this wrapper is used.
plugins/arrow-flight-rpc/src/main/java/io/netty/handler/ssl/AlpnAwareSSLEngineWrapper.java	8	high	Package name spoofing: Class resides in io.netty.handler.ssl package but is not part of Netty library. Appears designed to access package-private Netty interfaces (ApplicationProtocolAccessor) to bypass normal security boundaries. This pattern is commonly used in supply chain attacks.
plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProvider.java	176	medium	Architecture intentionally bypasses hostname verification when enforceHostnameVerification is false. While technically respecting a configuration flag, the implementation makes it trivial to disable a critical security control via the AlpnAwareSSLEngineWrapper.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 1 | High: 1 | Medium: 1 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 9592324)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Hostname verification bypass: The AlpnAwareSSLEngineWrapper.setSSLParameters method unconditionally sets endpointIdentificationAlgorithm to an empty string (line 48), which disables hostname verification. While this appears intentional when enforceHostnameVerification is false, the wrapper itself has no awareness of this flag and will strip hostname verification on every setSSLParameters call. If this wrapper is accidentally used in a context where hostname verification should be enforced, it creates a vulnerability to man-in-the-middle attacks. Ensure the wrapper is only instantiated when hostname verification is explicitly disabled, and consider adding safeguards or documentation to prevent misuse.

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Add AlpnAwareSSLEngineWrapper for ALPN and hostname verification control Relevant files: plugins/arrow-flight-rpc/src/main/java/io/netty/handler/ssl/AlpnAwareSSLEngineWrapper.java plugins/arrow-flight-rpc/src/test/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProviderTests.java Sub-PR theme: Add buildSecureTransportContext method to SecureTransportSettingsProvider interface Relevant files: server/src/main/java/org/opensearch/plugins/SecureTransportSettingsProvider.java Sub-PR theme: Refactor DefaultSslContextProvider to support live SSLContext hot-reload Relevant files: plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProvider.java plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/SslContextProvider.java plugins/arrow-flight-rpc/src/internalClusterTest/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProviderFlightIT.java

⚡ Recommended focus areas for review Thread Safety The double-checked locking pattern for lazy initialization of serverSslContext and clientSslContext may not be fully thread-safe without marking the fields as volatile. While the fields are declared volatile, verify that the initialization logic within the synchronized blocks cannot be reordered or partially observed by other threads. if (serverSslContext == null) { synchronized (this) { if (serverSslContext == null) { serverSslContext = new LiveSslContext(false); } } } return serverSslContext; } @Override public SslContext getClientSslContext() { if (clientSslContext == null) { synchronized (this) { if (clientSslContext == null) { clientSslContext = new AlpnPresettingClientSslContext(new LiveSslContext(true), enforceHostnameVerification); } } } return clientSslContext; Security Concern The wrapper unconditionally strips endpointIdentificationAlgorithm by setting it to empty string in setSSLParameters. This disables hostname verification regardless of the original parameter value, which could be a security risk if the wrapper is used in contexts where hostname verification should be enforced. Verify this behavior aligns with the intended security model. public void setSSLParameters(SSLParameters p) { p.setEndpointIdentificationAlgorithm(""); d.setSSLParameters(p); } Resource Cleanup The test creates an ExecutorService but relies on shutdownNow() in the finally block. If an exception occurs before reaching the finally block or if shutdownNow() fails, the executor threads may not be properly cleaned up. Consider using try-with-resources or ensuring cleanup in a tearDown method. ExecutorService exec = Executors.newSingleThreadExecutor(); try (RootAllocator allocator = new RootAllocator(Integer.MAX_VALUE)) { FlightServer server = OSFlightServer.builder() .allocator(allocator.newChildAllocator("server", 0, Long.MAX_VALUE)) .location(location) .producer(new NoOpFlightProducer()) .sslContext(sslServer.getServerSslContext()) .executor(exec) .build(); server.start(); try ( FlightClient client = OSFlightClient.builder() .allocator(allocator.newChildAllocator("client", 0, Long.MAX_VALUE)) .location(location) .sslContext(sslClient.getClientSslContext()) .build() ) { try { triggerHandshake(client); assertEquals(cnBefore, getCN(capturedChain)); reload.run(); getChannel(client).enterIdle(); triggerHandshake(client); assertEquals(cnAfter, getCN(capturedChain)); } finally { server.shutdown(); server.awaitTermination(); server.close(); exec.shutdownNow(); } Error Handling The buildJdkSslContext method catches SSLException and wraps it in a RuntimeException, losing the original exception type. This may make it harder for callers to handle SSL-specific errors appropriately. Consider whether the exception should be propagated as-is or wrapped in a more specific custom exception. private JdkSslContext buildJdkSslContext(boolean isClient) { try { SSLContext sslContext = secureTransportSettingsProvider.buildSecureTransportContext(settings) .orElseThrow(() -> new IllegalStateException("No SSLContext from SecureTransportSettingsProvider")); SecureTransportSettingsProvider.SecureTransportParameters params = secureTransportSettingsProvider.parameters(settings) .orElseThrow(() -> new IllegalStateException("No SecureTransportParameters from SecureTransportSettingsProvider")); ClientAuth clientAuth = ClientAuth.valueOf(params.clientAuth().orElse("NONE").toUpperCase(Locale.ROOT)); return new JdkSslContext( sslContext, isClient, params.cipherSuites().isEmpty() ? null : params.cipherSuites(), SupportedCipherSuiteFilter.INSTANCE, ALPN_H2, isClient ? ClientAuth.NONE : clientAuth, DEFAULT_SSL_PROTOCOLS, enforceHostnameVerification ); } catch (SSLException e) { throw new RuntimeException(e); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9592324
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add volatile to ensure thread-safe initialization The double-checked locking pattern is used without declaring serverSslContext and clientSslContext as volatile. Without the volatile keyword, the pattern is broken in Java because another thread may see a partially constructed object. Declare both fields as volatile to ensure proper visibility across threads. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProvider.java [65-66] -@Override -public SslContext getServerSslContext() { - if (serverSslContext == null) { - synchronized (this) { - if (serverSslContext == null) { - serverSslContext = new LiveSslContext(false); - } - } - } - return serverSslContext; -} +private volatile SslContext serverSslContext; +private volatile SslContext clientSslContext; -@Override -public SslContext getClientSslContext() { - if (clientSslContext == null) { - synchronized (this) { - if (clientSslContext == null) { - clientSslContext = new AlpnPresettingClientSslContext(new LiveSslContext(true), enforceHostnameVerification); - } - } - } - return clientSslContext; -} - Suggestion importance[1-10]: 9 __ Why: This is a critical concurrency bug. The double-checked locking pattern without volatile can result in threads seeing partially constructed SslContext objects due to instruction reordering. This could cause subtle runtime failures in production under concurrent access.	High
General	Avoid mutating caller's SSLParameters object Mutating the input SSLParameters object directly can cause unexpected side effects for the caller, as they may reuse the same object. Create a defensive copy of the parameters before modifying them to avoid altering the caller's state. plugins/arrow-flight-rpc/src/main/java/io/netty/handler/ssl/AlpnAwareSSLEngineWrapper.java [47-50] @Override public void setSSLParameters(SSLParameters p) { - p.setEndpointIdentificationAlgorithm(""); - d.setSSLParameters(p); + SSLParameters copy = new SSLParameters( + p.getCipherSuites(), + p.getProtocols() + ); + copy.setEndpointIdentificationAlgorithm(""); + d.setSSLParameters(copy); } Suggestion importance[1-10]: 7 __ Why: Mutating the input SSLParameters object can cause unexpected side effects if the caller reuses it. However, the improved_code is incomplete—it doesn't copy all fields like setWantClientAuth, setNeedClientAuth, setServerNames, etc. A proper defensive copy would require cloning all relevant fields or using a copy constructor if available.	Medium
	Ensure proper exception handling during shutdown The server.close() call after server.awaitTermination() may fail if the server is not properly shut down. The FlightServer is created outside the try-with-resources block, so exceptions during shutdown won't be properly handled. Move the server lifecycle management into a try-with-resources block or ensure proper exception handling in the finally block. plugins/arrow-flight-rpc/src/internalClusterTest/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProviderFlightIT.java [147-179] try (RootAllocator allocator = new RootAllocator(Integer.MAX_VALUE)) { FlightServer server = OSFlightServer.builder() .allocator(allocator.newChildAllocator("server", 0, Long.MAX_VALUE)) .location(location) .producer(new NoOpFlightProducer()) .sslContext(sslServer.getServerSslContext()) .executor(exec) .build(); server.start(); try ( FlightClient client = OSFlightClient.builder() .allocator(allocator.newChildAllocator("client", 0, Long.MAX_VALUE)) .location(location) .sslContext(sslClient.getClientSslContext()) .build() ) { try { triggerHandshake(client); assertEquals(cnBefore, getCN(capturedChain)); reload.run(); getChannel(client).enterIdle(); triggerHandshake(client); assertEquals(cnAfter, getCN(capturedChain)); } finally { - server.shutdown(); - server.awaitTermination(); - server.close(); - exec.shutdownNow(); + try { + server.shutdown(); + server.awaitTermination(); + } finally { + server.close(); + exec.shutdownNow(); + } } } } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that server.close() could fail if shutdown is incomplete, but the existing code already handles this in a finally block which guarantees execution. Wrapping in nested try-finally adds defensive exception handling but provides only marginal improvement since the test would fail anyway if shutdown throws.	Low

---

Previous suggestions

Suggestions up to commit 0a82204

Category	Suggestion	Impact
General	Avoid mutating input parameters Mutating the input SSLParameters object directly can cause unexpected side effects for the caller. Create a defensive copy before modifying to avoid altering the caller's original object. plugins/arrow-flight-rpc/src/main/java/io/netty/handler/ssl/AlpnAwareSSLEngineWrapper.java [47-50] @Override public void setSSLParameters(SSLParameters p) { - p.setEndpointIdentificationAlgorithm(""); - d.setSSLParameters(p); + SSLParameters copy = new SSLParameters( + p.getCipherSuites(), + p.getProtocols() + ); + copy.setEndpointIdentificationAlgorithm(""); + d.setSSLParameters(copy); } Suggestion importance[1-10]: 7 __ Why: Mutating the input SSLParameters object can cause unexpected side effects for callers. Creating a defensive copy is a good practice, though the impact depends on how the method is called in practice.	Medium
	Ensure thread-safe field access The enforceHostnameVerification field is read without synchronization during lazy initialization. If this field can be modified after construction, declare it as volatile or ensure it's final to prevent visibility issues. plugins/arrow-flight-rpc/src/main/java/org/opensearch/arrow/flight/bootstrap/tls/DefaultSslContextProvider.java [64] -private volatile SslContext clientSslContext; +private final boolean enforceHostnameVerification; @Override public SslContext getClientSslContext() { if (clientSslContext == null) { synchronized (this) { if (clientSslContext == null) { clientSslContext = new AlpnPresettingClientSslContext(buildJdkSslContext(true), enforceHostnameVerification); } } } return clientSslContext; } Suggestion importance[1-10]: 5 __ Why: The enforceHostnameVerification field is set in the constructor and only read afterward. Making it final is a good practice for immutability and thread safety, though it's a minor improvement since the field is not modified after construction.	Low

[github-actions]

❌ Gradle check result for 0a82204: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 9592324

[github-actions]

✅ Gradle check result for 9592324: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 46.39175% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.27%. Comparing base (5c183b8) to head (9592324).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...o/netty/handler/ssl/AlpnAwareSSLEngineWrapper.java	15.21%	38 Missing and 1 partial ⚠️
...light/bootstrap/tls/DefaultSslContextProvider.java	76.00%	8 Missing and 4 partials ⚠️
...earch/plugins/SecureTransportSettingsProvider.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20734      +/-   ##
============================================
+ Coverage     73.23%   73.27%   +0.03%     
- Complexity    72016    72051      +35     
============================================
  Files          5783     5784       +1     
  Lines        329438   329525      +87     
  Branches      47534    47545      +11     
============================================
+ Hits         241268   241456     +188     
+ Misses        68868    68694     -174     
- Partials      19302    19375      +73     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[harshavamsi]

@rishabhmaurya looks like we added to the changelog