Skip to content

[FIP-29] Add SSL config parsing and SslContext/SslHandler factory - #3813

Open
MicheleGuerriero wants to merge 2 commits into
apache:mainfrom
MicheleGuerriero:3796-ssl-config-and-sslcontext-factory
Open

[FIP-29] Add SSL config parsing and SslContext/SslHandler factory#3813
MicheleGuerriero wants to merge 2 commits into
apache:mainfrom
MicheleGuerriero:3796-ssl-config-and-sslcontext-factory

Conversation

@MicheleGuerriero

Copy link
Copy Markdown

Purpose

Linked issue: close #3796

First building block of FIP-29 (mTLS support for Fluss RPC). This adds
the configuration surface and a factory for building Netty SSL
contexts/handlers, but does not wire them into any pipeline yet — no
runtime behavior changes as a result of this PR.

Brief change log

  • ConfigOptions: add server-side security.ssl.* options (enabled
    listeners, protocols, cipher suites, keystore/truststore paths and
    passwords, reload interval) and client-side client.security.ssl.*
    options (mirrors the server options, plus endpoint identification
    algorithm for hostname verification).
  • SslConfig: new immutable holder that parses and validates the
    above options from a Configuration, with fromServerConfig/
    fromClientConfig factory methods. Fails fast with a clear message
    if a server enables TLS without configuring a keystore.
  • SslContextFactory: new factory building Netty SslContext/
    SslHandler instances from an SslConfig, using the JDK SSL
    provider. Supports per-listener client-certificate requirement
    (setNeedClientAuth, for mTLS listeners) on the server side, and
    SNI + hostname verification on the client side.

Later tickets (#3792 server pipeline, #3797 client pipeline) will wire
SslContextFactory into the actual Netty channel pipelines.

Tests

Added SslContextFactoryTest, backed by TestSslUtils (generates a
self-signed certificate and on-the-fly JKS keystore/truststore files,
no committed key material):

  • server/client SslContext creation
  • enabled-protocol and cipher-suite filtering
  • key-password fallback to keystore password
  • client endpoint-identification-algorithm toggling (hostname
    verification on/off)
  • server needClientAuth toggling for mTLS
  • an end-to-end embedded-channel TLS handshake between a server and
    client SslHandler, asserting the negotiated session uses a real
    TLS protocol/cipher (not plaintext)
  • fail-fast validation when no keystore is configured

Verified locally on fluss-common and fluss-rpc: mvn test (all
green, SslContextFactoryTest 9/9), spotless:check,
checkstyle:check, Apache RAT license check, and a JDK 8 build
(-Pjava8) confirming Java 8 source compatibility.

API and Format

Adds new ConfigOptions (security.ssl.*, client.security.ssl.*)
and two new @Internal classes in fluss-rpc
(org.apache.fluss.rpc.netty.ssl). No changes to existing public
APIs, RPC wire format, or storage format. The new classes are
currently unused by any production code path.

Documentation

No user-facing behavior yet, so no documentation changes in this PR.
Configuration docs for security.ssl.* / client.security.ssl.*
will be added once these options take effect (server/client pipeline
wiring in the following tickets).

  • No generative AI tools used
  • Yes (Claude Code, Claude Opus 5)

First building block of FIP-29 (mTLS support), see apache#3796.

Adds the server-side security.ssl.* and client-side client.security.ssl.*
config options (protocols, cipher suites, keystore/truststore, endpoint
identification, reload interval), an immutable SslConfig holder parsing
them with fail-fast validation, and an SslContextFactory building Netty
SslContext/SslHandler instances for server and client (JDK provider,
per-listener client-auth requirement, SNI + hostname verification).

These are currently-unused building blocks; later tickets wire them into
the Netty pipeline. No runtime behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@affo affo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The work looks very good and aligned with the FIP 1:1, thank you! 🤝

I only have a couple doubts marked as a comments 🤝

// Server-side TLS (transport encryption + mTLS) options
// ------------------------------------------------------------------------

public static final ConfigOption<List<String>> SERVER_SSL_ENABLED_LISTENERS =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there is a way to mark these options as "experimental" for the time being 🤔

My concern is that these options are now exposed to the outer world without anything wired, and this may compromise the release (say that we don't finalize the feature and it is half baked).

I would need the feedback of a committer here, or a PMC even :) @fresh-borzoni would you feel like it?

}

/** Build and validate the server-side TLS configuration. */
public static SslConfig fromServerConfig(Configuration conf) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do understand that this contribution does not switch yet on SERVER_SSL_ENABLED_LISTENERS to enforce the validation that remains implicit, but this also makes sense as we don't have yet the wiring and those methods stay not invoked for now.

In the future one should extract the SSL config by checking that.

But that would turn to be cumbersome as part of the logic of checking whether SSL is enabled would fall out of the config class itself.

I wonder if it would be a neater approach to embed right now all this knowledge into this class, may return an Optional to embed the semantics of the fact that SSL may be there or not.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. And since nothing consumes these factory methods yet, this PR is the cheapest moment to fix this. I have changed it in c31c730.

One nuance: Optional can only encode the "is TLS configured at all" question. On the server side enablement is per-listener, so the pipeline wiring will still need the listener set for per-listener handler installation. To keep that knowledge adjacent to the config class, I have also exposed enabledListeners() on the server-side SslConfig, so callers read the listener set from the parsed config object instead of going back to the raw Configuration.

Address review feedback on apache#3813: whether TLS is enabled was decided
outside SslConfig, forcing future pipeline wiring to consult the raw
configuration before parsing it. Embed that knowledge in the config
class instead:

- SslConfig.fromServerConfig returns Optional.empty() when no listener
  is listed in security.ssl.enabled.listeners; the fail-fast keystore
  validation is unchanged when TLS is enabled.
- SslConfig.fromClientConfig returns Optional.empty() when
  client.security.ssl.enabled is false.
- SslConfig exposes enabledListeners() so the server pipeline wiring
  can read the TLS listener set from the parsed config instead of the
  raw Configuration.
- The Configuration-taking SslContextFactory overloads now return
  Optional<SslContext> accordingly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@affo affo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing job, I sent en email at dev@ to see whether it would be best to merge this contributions on a separate branch and merge after 1.0 feature freeze, waiting for consensus there on how to operate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[TLS] Add SSL config parsing and SslContext/SslHandler factory

2 participants