gaborgsomogyi opened a new pull request, #29265:
URL: https://github.com/apache/flink/pull/29265
## What is the purpose of the change
This pull request fixes two related SSL/TLS bugs found while auditing
`security.ssl.protocol`.
`security.ssl.protocol` is documented as not supporting a comma-separated
list of protocols, but that is only true for one of its three consumers. The
internal Netty data transport, the REST endpoints, and the Blob server/client
already support a comma-separated list correctly (they build a generic SSL
context and narrow it with the full protocol array). Only the Pekko-based RPC
control-plane path breaks: the value is spliced unquoted into the generated
HOCON config, so any comma in it causes a config parse failure, and even past
that, Pekko's own `ConfigSSLEngineProvider` only accepts a single protocol name.
While adding end-to-end negotiation tests for every consumer of this option,
a second, unrelated and more serious issue surfaced: internal SSL is documented
to skip hostname verification, because it is designed around one shared,
mutually-trusted certificate distributed to every node in the cluster (this is
what makes container-based setups such as Kubernetes practical - a certificate
cannot realistically carry a Subject Alternative Name for every
dynamically-scheduled pod). A prior cleanup of the SSL hostname-verification
configuration removed the explicit override that enforced this for internal
SSL, without replacing it. Since Netty enables hostname verification by default
from 4.2 onward, internal SSL has since been silently verifying the peer
hostname against the shared certificate on both the JDK and OpenSSL providers,
and will fail every connection whose resolved hostname is not covered by that
certificate - effectively any real multi-node deployment.
Together, this pull request:
- Fixes the Pekko RPC path so it honors a full comma-separated protocol
list like the other consumers
- Restores the documented behavior that internal SSL does not require the
peer hostname to match the shared certificate
- Corrects the `security.ssl.protocol` documentation to describe the
actual (now consistent) behavior
- Adds regression tests that lock in the negotiation behavior and the
hostname-verification behavior for every consumer of internal SSL
## Brief change log
- *Quote the `protocol` value when building the Pekko remoting HOCON
config in `PekkoUtils`, fixing a config parse failure on any comma-separated
value*
- *Override `createServerSSLEngine()`/`createClientSSLEngine()` in
`CustomSSLEngineProvider` to build the SSL engine from the full split protocol
list instead of relying on Pekko's own single-protocol
`ConfigSSLEngineProvider` implementation*
- *Explicitly disable endpoint identification for internal SSL in
`SSLUtils#createInternalNettySSLContext`, restoring hostname-verification-free
negotiation against the shared certificate on both the JDK and OpenSSL
providers*
- *Update `SecurityOptions.SSL_PROTOCOL`'s description to state that a
comma-separated list is supported, including how the highest mutually supported
protocol is negotiated and a note on the OpenSSL provider's contiguous-range
behavior*
- *Regenerate the affected configuration documentation snippets*
- *Add regression tests covering multi-protocol negotiation for internal
SSL, REST SSL, Blob SSL, and Pekko RPC SSL*
- *Add a regression test asserting that internal SSL negotiates
successfully even when the peer hostname does not match the shared certificate*
## Verifying this change
Please make sure both new and modified tests in this PR follow [the
conventions for tests defined in our code quality
guide](https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#7-testing).
This change added tests and can be verified as follows:
- *Added `CustomSSLEngineProviderTest`, which builds a real `ActorSystem`
from the actual production config path and drives a real socket-based TLS
handshake through the fixed `CustomSSLEngineProvider`, asserting negotiation to
TLSv1.3 when both sides support it and a graceful fallback to TLSv1.2 when no
TLSv1.3 cipher suite is configured*
- *Extended `SSLUtilsTest` with equivalent real-handshake regression tests
for the internal Netty data transport, REST, and Blob server/client SSL paths,
asserting the same negotiate-or-fall-back behavior, run against both the JDK
and OpenSSL providers*
- *Added `SSLUtilsTest#testInternalSSLIgnoresPeerHostMismatch`, which
connects to a real socket while telling the client engine to verify a
fabricated hostname the shared certificate was never meant to cover, and
asserts negotiation still succeeds on both the JDK and OpenSSL providers*
- *Confirmed all added tests fail before the corresponding fix and pass
after, and ran the full set of existing SSL-related test classes in
`flink-runtime` with the OpenSSL provider enabled to confirm no regressions*
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: yes, `SecurityOptions` is `@PublicEvolving`; the change
only clarifies and extends the accepted value format of an existing option and
restores previously-documented behavior, it does not change any method signature
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its
components), Checkpointing, Kubernetes/Yarn, ZooKeeper: yes - this restores
internal SSL connectivity between JobManager and TaskManager for any deployment
where the shared certificate does not carry every node's hostname as a Subject
Alternative Name, which is the common case for container-based deployments such
as Kubernetes
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? docs (updated
`security.ssl.protocol` description, reflected in the generated configuration
docs)
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (Claude Code)
<!--
Generated-by: Claude Code (Sonnet 5)
-->
--
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]
