Repository navigation
[FIP-29] Add SSL config parsing and SslContext/SslHandler factory - #3813
fresh-borzoni merged 2 commits into
Conversation
affo
left a comment
affo
left a comment
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@affo Let's just move with feature branch until the feature is complete and then include it into the appropriate release, it's the least obtrusive approach
| } | ||
|
|
||
| /** Build and validate the server-side TLS configuration. */ | ||
| public static SslConfig fromServerConfig(Configuration conf) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <[email protected]>
affo
left a comment
affo
left a comment
There was a problem hiding this comment.
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
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@MicheleGuerriero Thank you for the PR, I left some comments, PTAL
| if (!config.cipherSuites().isEmpty()) { | ||
| builder.ciphers(config.cipherSuites()); | ||
| } | ||
| if (config.truststorePath() != null) { |
There was a problem hiding this comment.
When no truststore is configured this skips trustManager(), so the context falls back to the JVM default truststore and setNeedClientAuth(true) at line 141 then accepts any client cert chaining to a public CA in cacerts.
FIP-29 wants an mTLS listener without security.ssl.truststore.path to refuse to start.
Should SslConfig read security.protocol.map so it can be enforced?
There was a problem hiding this comment.
SslConfig now reads security.protocol.map. Fixed in 54bf270. fromServerConfig intersects the protocol map with security.ssl.enabled.listeners, collects the listeners whose protocol is mTLS (matched with equalsIgnoreCase, same as AuthenticationFactory matches authentication protocol names), and refuses the configuration when any of them has no security.ssl.truststore.path — naming the listener and saying why, rather than letting the context inherit cacerts. It's exposed as requiresClientAuth(listener) / clientAuthListeners(), so the pipeline wiring reads the requirement off the parsed config instead of the raw Configuration.
| "The format of the server truststore file. Supported values are `JKS` " | ||
| + "and `PKCS12`. The default is `JKS`."); | ||
|
|
||
| public static final ConfigOption<Duration> SERVER_SSL_RELOAD_INTERVAL = |
There was a problem hiding this comment.
This and client.security.ssl.reload.interval describe hot-reload that isn't implemented yet, and docgen reflects over every option in this class, so they land in the published config table as soon as it's regenerated.
Can the reload options come with the reloader instead?
There was a problem hiding this comment.
Removed both in 5b5b115. security.ssl.reload.interval and client.security.ssl.reload.interval are gone, along with the SslConfig.reloadInterval field they fed, and they'll come back in the PR that actually adds the reloader.
| } | ||
|
|
||
| String keystorePath = conf.getString(ConfigOptions.SERVER_SSL_KEYSTORE_PATH); | ||
| checkArgument( |
There was a problem hiding this comment.
nit: Should this be IllegalConfigurationException? That's what LocalDiskManager throws for bad server config?
There was a problem hiding this comment.
Fixed in f53277a
| } | ||
|
|
||
| @Test | ||
| void testServerSslHandlerClientAuthRequirement() { |
There was a problem hiding this comment.
This checks the flag but not that a client without a cert gets rejected. testServerAndClientNegotiateTls already pumps a handshake, can you add one with requireClientAuth=true and no client keystore?
PKCS12 is advertised in the keystore.type descriptions and untested too.
There was a problem hiding this comment.
Both added in 172dfd1. I extracted the handshake pump from testServerAndClientNegotiateTls as you suggested, and built three tests on it: an mTLS server rejecting a client that presents no certificate (Empty client certificate chain), the same server accepting one that presents a trusted certificate, and a full mutual handshake with PKCS12 keystores and truststores on both sides.
|
|
||
| /** Generate a self-signed certificate whose subject CN is {@code fqdn}. */ | ||
| public static SelfSignedCertificate generateCertificate(String fqdn) throws Exception { | ||
| return new SelfSignedCertificate(fqdn); |
There was a problem hiding this comment.
SelfSignedCertificate needs BouncyCastle or the sun.security.x509 internals, so all 12 tests error on JDK 17 and pass on 11. CI only runs tests on 11 so it stays green. --add opens=java.base/sun.security.x509=ALL-UNNAMED in extraJavaTestArgs makes them pass on 17, or a test scoped bcpkix(I guess this is better since it will work on jdk 21 as well)
Can we do one of these?
There was a problem hiding this comment.
Went with the test-scoped bcpkix as you suggested, fixed in d4b39c6. Netty's SelfSignedCertificate prefers BouncyCastle when it's on the classpath, so the dependency alone is enough and TestSslUtils didn't need rewriting.
| // Server-side TLS (transport encryption + mTLS) options | ||
| // ------------------------------------------------------------------------ | ||
|
|
||
| public static final ConfigOption<List<String>> SERVER_SSL_ENABLED_LISTENERS = |
There was a problem hiding this comment.
@affo Let's just move with feature branch until the feature is complete and then include it into the appropriate release, it's the least obtrusive approach
| config.truststoreType(), | ||
| config.truststorePassword())); | ||
| } | ||
| return builder.build(); |
There was a problem hiding this comment.
The engine is created per connection in createServerSslHandler, and that's the only place these protocol and cipher names get validated. So a typo in security.ssl.enabled.protocols or security.ssl.cipher.suites starts the server cleanly and then throws IllegalArgumentException: Unsupported protocol, while building the handler for every TLS connection, without naming the option that's wrong.
An empty enabled.protocols is worse as the engine comes up with no protocols enabled and every handshake just fails.
There was a problem hiding this comment.
Confirmed all three cases against the JDK provider before fixing, and it's exactly as you describe — the context builds fine in every one of them.
Fixed in f3cd33e: fromServerConfig/fromClientConfig now reject unsupported protocol and cipher suite names and an empty protocol list, throwing IllegalConfigurationException that names the exact option (server or client key, since the validator takes the ConfigOption rather than just the values), the offending entries, and the supported set. Supported names come from one throwaway SSLEngine off the default SSLContext.
Address review feedback on apache#3813: security.ssl.reload.interval and client.security.ssl.reload.interval documented certificate hot-reload that this PR does not implement, and ConfigOptionsDocGenerator reflects over every ConfigOption field in ConfigOptions with no exclusion mechanism, so both would appear in the published configuration table as soon as it is regenerated. Remove both options and the SslConfig.reloadInterval field they fed; they belong in the PR that adds the reloader itself. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the missing-keystore check used Preconditions.checkArgument, which surfaces an IllegalArgumentException. Invalid Fluss configuration is reported as IllegalConfigurationException elsewhere in the codebase (LocalDiskManager and RemoteDirDynamicLoader on the server, WriterClient and ClientUtils on the client), so use that instead. The message is unchanged; the test asserts the new type. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: protocol and cipher suite names were only validated when an SSLEngine was created, which happens once per connection. A typo in security.ssl.enabled.protocols or security.ssl.cipher.suites let the server start cleanly and then failed every TLS connection with an engine-level "Unsupported protocol" / "Unsupported CipherSuite" that does not name the option at fault. An explicitly empty protocol list was worse: nothing failed, the engine came up with no protocol enabled and every handshake failed. SslConfig.fromServerConfig/fromClientConfig now reject, per side and naming the exact option, protocol and cipher suite names this JVM does not support (listing the supported ones), as well as an empty protocol list. Cipher suites are checked against JVM support only, not against the enabled protocols: which suites can be negotiated depends on the protocol agreed during the handshake. An empty cipher suite list stays valid and selects the provider defaults, as documented. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: SslContextFactory skips trustManager() when no truststore is configured, so the server SSL context falls back to the JVM default truststore. A listener that requires a client certificate would then accept any certificate issued by a public CA in cacerts, which is not the mTLS guarantee FIP-29 describes. SslConfig now reads security.protocol.map, derives which of the TLS-enabled listeners are mTLS listeners (matched case-insensitively, as AuthenticationFactory matches authentication protocol names), and refuses the configuration when any of them has no security.ssl.truststore.path. Listeners that are mTLS but not TLS-enabled impose nothing, and an encryption-only listener still needs no truststore. The requirement is exposed as requiresClientAuth(listener) / clientAuthListeners(), so the server pipeline wiring reads it from the parsed config rather than re-deriving it from the raw Configuration. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the client-auth test only asserted the needClientAuth flag, and PKCS12 was advertised in the keystore.type descriptions without a test. Extract the handshake pump from testServerAndClientNegotiateTls into a helper and add three tests on top of it: an mTLS server rejects a client that presents no certificate ("Empty client certificate chain"), accepts one that presents a trusted certificate, and completes the same mutual handshake with PKCS12 keystores and truststores on both sides. The rejection is asserted on the server handshake future only. Under TLS 1.3 the client sends its Finished before the server validates the certificate, so the client's future completes successfully and only learns of the rejection from the alert that follows. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: every test in SslContextFactoryTest errored on JDK 17 and only passed on 11, which CI does not catch because it runs the Java tests on 11 (and nightly on 8). Netty's SelfSignedCertificate generates the certificate with BouncyCastle when it is on the classpath and otherwise falls back to a generator built on sun.security.x509 internals. Add bcpkix-jdk18on in test scope so the first path is taken. No production dependency and nothing shaded, so the shipped artifacts are unchanged. Verified: 24/24 tests pass on JDK 11, 17 and 24 with this change, against 24 errors on 17 and 24 without it. The alternative --add-opens=java.base/sun.security.x509=ALL-UNNAMED in extraJavaTestArgs was tried too: it fixes JDK 17 but still fails on 24, which is why the dependency is the better answer. bcutil and bcprov are bcpkix's own transitive dependencies, pinned here because bcpkix declares them through version ranges — those resolve differently over time and make every build fetch their maven-metadata.xml. All three artifacts target Java 8 bytecode, so the nightly JDK 8 test run keeps working. Co-Authored-By: Claude Opus 5 <[email protected]>
| } | ||
| return builder.build(); | ||
| } catch (Exception e) { | ||
| throw new FlussRuntimeException("Failed to build the server SSL context.", e); |
There was a problem hiding this comment.
make the error messages more descriptive, so the users can understand why things fail.. As it is right now, it would be hard for a user to identify the reason of failure - for example, paths, passwords etc.
There was a problem hiding this comment.
Fixed in f87dd52. Each store now carries the options that configure it, so the failures an operator can actually cause name both the file and the option.
Same on the client side, naming the client.security.ssl.* options. The generic "Failed to build the SSL context" wrapper is now only around the context build itself, so it no longer swallows these.
| } | ||
| return builder.build(); | ||
| } catch (Exception e) { | ||
| throw new FlussRuntimeException("Failed to build the client SSL context.", e); |
|
|
||
| private static KeyStore loadKeyStore(String path, String type, String password) | ||
| throws Exception { | ||
| KeyStore keyStore = KeyStore.getInstance(type); |
There was a problem hiding this comment.
type isn't validated at parse time, a typo starts the server and fails late.
There was a problem hiding this comment.
Fixed in bdcb58d. SslConfig now validates the store types at parse time and throws IllegalConfigurationException naming the exact option (server or client), the bad value, and the types the JVM does support.
|
|
||
| Map<String, String> protocolMap = conf.get(ConfigOptions.SERVER_SECURITY_PROTOCOL_MAP); | ||
| Set<String> clientAuthListeners = | ||
| enabledListeners.stream() |
There was a problem hiding this comment.
protocolMap.get(listener) is exact-match while the value is equalsIgnoreCase.
With enabled.listeners: client + protocol.map: CLIENT:mTLS misses; we have one-way TLS, no truststore, silent cacerts fallback. We can document exact-match requirement or normalize.
There was a problem hiding this comment.
Documented rather than normalized — ed758ad.
The asymmetry mirrors how the server resolves each half at runtime: FlussProtocolPlugin picks a listener's authenticator with a plain suppliers.get(listenerName) on security.protocol.map, while AuthenticationFactory matches a plugin to a protocol name with equalsIgnoreCase. If I matched listener names loosely here, SslConfig would classify a listener as mTLS and demand a truststore for it, while the server still authenticates it as PLAINTEXT, a disagreement between the two layers instead of one wrong but consistent outcome.
| * sun.security.x509}, which needs {@code --add-opens} from JDK 9 on and stops working altogether | ||
| * from JDK 18 on. | ||
| */ | ||
| public class TestSslUtils { |
There was a problem hiding this comment.
The current test only asserts the param is set. Add an end-to-end handshake with "https" against a cert whose CN ≠ host and assert rejection.
And maybe it's good to add a handshake where the client truststore does not contain the server cert and
There was a problem hiding this comment.
Both added in 0f3a107. The handshake helper now takes the host the client dials and the endpoint identification algorithm, and three tests run on it: a certificate issued for another host is rejected with https set, the same handshake succeeds when the certificate matches the host and a server certificate outside the client truststore is rejected.
|
@MicheleGuerriero thank you for your contribution.. Overall LGTM 👍 I left some comments, and the CI needs fixing, it seems |
Address review feedback on apache#3813: SslConfig looks the listener up in security.protocol.map by exact name while comparing the protocol name with equalsIgnoreCase, which makes the option look more forgiving about case than it is. Both halves mirror how the server resolves them at runtime: FlussProtocolPlugin picks a listener's authenticator with a plain map lookup, and AuthenticationFactory matches a plugin to a protocol name with equalsIgnoreCase. Matching listener names loosely here would classify a listener as mTLS that the server then authenticates as PLAINTEXT, so the exact match stays and is documented instead: in the security.ssl.enabled.listeners description, and in a comment explaining why the two halves differ. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: a typo in security.ssl.keystore.type or any of its three siblings was only caught when the store was loaded, as a KeyStoreException from KeyStore.getInstance that does not name the option at fault. SslConfig now rejects a store type this JVM has no provider for, per side and naming the exact option, listing the types the JVM does support. Only the type of a store that is actually configured is checked, since the type of an absent store is never used to load anything. Type names stay as case-insensitive as JCA itself is. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: every key material failure collapsed into "Failed to build the server SSL context." with the reason only in the nested JCA exception, so an operator could not tell which of the four stores was at fault, let alone which option to fix. Each store now carries the options that configure it, and the failures an operator can actually cause are reported with the file and the option: a missing file names the path option, a store that will not open names the password option, and a private key that cannot be recovered names the key password option and the keystore password it falls back to. The generic wrapper is now only around the context build itself. The messages follow what the JDK really does: a wrong store password surfaces as IOException, a wrong key password as UnrecoverableKeyException, and a store loaded under the other type is not an error at all, since the JKS and PKCS12 providers read both formats. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the hostname verification test only read back the endpoint identification algorithm from the engine, so it asserted that a parameter was set rather than that it has any effect. The handshake helper now takes the host the client dials and the endpoint identification algorithm, and three tests run on top of it: a certificate issued for another host is rejected with the algorithm set to https, the same handshake succeeds when the certificate matches the host, and a server certificate outside the client truststore is rejected. The client is the rejecting side in all three, the mirror of the client-certificate test where the server rejects. Two details the assertions follow: Netty's SelfSignedCertificate carries no dNSName extension, so the JDK falls back to the subject CN and reports "No name matching localhost found"; and since both certificates in the truststore test are issued for localhost, the JDK finds a trust anchor by name and rejects it on the signature, which shows trust is decided by key rather than by subject. Co-Authored-By: Claude Opus 5 <[email protected]>
|
@fresh-borzoni do you have any further comments here? If not, we can merge. @MicheleGuerriero, maybe it's also worth rebasing first. |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@MicheleGuerriero Thank you, took a look, some comments, PTAL
| // loosely | ||
| // here would classify a listener as mTLS that the server then authenticates as PLAINTEXT. | ||
| Map<String, String> protocolMap = conf.get(ConfigOptions.SERVER_SECURITY_PROTOCOL_MAP); | ||
| Set<String> clientAuthListeners = |
There was a problem hiding this comment.
The FIP validation table says an mTLS listener that isn't in security.ssl.enabled.listeners should refuse to start.
Here it's accepted, and testMtlsListenerWithoutTlsNeedsNoTruststore asserts exactly that. It can't happen yet since there's no mTLS plugin, but this class already reads security.protocol.map, so the check fits here. Can we add it and flip that test?
There was a problem hiding this comment.
You're right, fixed in d292952. I had the derivation walking security.ssl.enabled.listeners and looking each one up in the protocol map, so an mTLS listener without TLS was filtered out before any check could see it. It now starts from security.protocol.map instead, and every mTLS listener missing from security.ssl.enabled.listeners is rejected by name.testMtlsListenerWithoutTlsNeedsNoTruststore is flipped accordingly.
| // Server-side TLS (transport encryption + mTLS) options | ||
| // ------------------------------------------------------------------------ | ||
|
|
||
| public static final ConfigOption<List<String>> SERVER_SSL_ENABLED_LISTENERS = |
There was a problem hiding this comment.
Agree we shouldn't match loosely. But a typo here means the listener silently runs plaintext, and with SASL that means credentials in clear.
Can we reject names that aren't in bind.listeners? Fine to do it in #3792 if that's easier. security.protocol.map has the same hole today - a typo'd key falls back to PLAINTEXT.
There was a problem hiding this comment.
Agreed, I'll do it in #3792 as you suggest.
| * disabled (i.e. {@code client.security.ssl.enabled} is false). | ||
| */ | ||
| public static Optional<SslConfig> fromClientConfig(Configuration conf) { | ||
| if (!conf.get(ConfigOptions.CLIENT_SSL_ENABLED)) { |
There was a problem hiding this comment.
Same for the client rows of the FIP table: client.security.protocol=mTLS without client.security.ssl.enabled, or without a keystore, should fail. Now it's accepted.
There was a problem hiding this comment.
Both added in aedf46a. fromClientConfig now rejects client.security.protocol=mTLS when client.security.ssl.enabled is false ("requires TLS transport") and when client.security.ssl.keystore.path is unset ("the client has no certificate to present to the server"), naming the option at fault in each case.
|
|
||
| /** Build a server {@link SslContext} from a parsed {@link SslConfig}. */ | ||
| public static SslContext createServerSslContext(SslConfig config) { | ||
| KeyManagerFactory kmf = |
There was a problem hiding this comment.
If the keystore has no private key (e.g. someone points it at the truststore), the context builds fine and every handshake fails with No available authentication scheme.
Can we check for a key entry when loading it?
There was a problem hiding this comment.
added in 94915ab. The keystore is now checked for a key entry as it's loaded, on both sides, and reported with the file, the option that configures it, and what the handshake would have done.
| truststorePath, | ||
| conf.getString(ConfigOptions.CLIENT_SSL_TRUSTSTORE_PASSWORD), | ||
| truststoreType, | ||
| conf.getString( |
There was a problem hiding this comment.
nit: htps passes parsing and then fails every handshake. Shall we validate it like the protocols: https/ldaps ignoring case, or empty?
Address review feedback on apache#3813: the FIP-29 validation table requires a listener whose security.protocol.map entry is mTLS to also appear in security.ssl.enabled.listeners, and to refuse to start otherwise, since certificate authentication has no transport to run on without TLS. The derivation walked the TLS listeners and looked each one up in the protocol map, so an mTLS listener without TLS was filtered out before anything could complain, and a test asserted that as intended. The derivation now starts from the protocol map, and every mTLS listener missing from security.ssl.enabled.listeners is reported by name. The check runs before the early return taken when no listener enables TLS, so the worst case - an mTLS listener on a server with no TLS configured anywhere - is rejected too rather than reported as simply having no TLS. fromServerConfig can therefore throw while otherwise returning an empty Optional. testMtlsListenerWithoutTlsNeedsNoTruststore asserted the old behaviour and is flipped; the no-TLS-at-all case is covered by a new test. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the FIP-29 validation table has two client rows that were not implemented. A client configured with client.security.protocol=mTLS must have client.security.ssl.enabled set, since certificate authentication has no transport to run on otherwise, and must have client.security.ssl.keystore.path set, since it has no certificate to present without one. Both were accepted silently. fromClientConfig now rejects each with the option at fault named, before the early return taken when TLS is disabled, so the combination that turns TLS off outright is rejected rather than reported as a client with no TLS. The protocol name is compared ignoring case, as AuthenticationFactory compares it. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: a keystore with no key entry, which is what a truststore pointed at by mistake looks like, loaded and built a context without complaint. Verified what follows: the server handshake then fails with "No available authentication scheme" on every connection, and the client side reports no cause at all, so the connection just drops. The keystore is now checked for a key entry as it is loaded, on both the server and the client side, and reported with the file, the option that configures it and what the handshake would have done. Truststores are not checked, since holding only trusted certificates is what they are for. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: an unknown value of client.security.ssl.endpoint.identification.algorithm was accepted by SSLParameters and only rejected once a handshake ran, as "Unknown identification algorithm", i.e. on every connection. SslConfig now accepts https and ldaps ignoring case, which are the two the JDK implements, and an empty value, which disables hostname verification as documented. Anything else is rejected with the option named and the supported values listed. Verified what the JDK does with each: https and HTTPS and ldaps all run the check, an empty value skips it and the handshake succeeds, and htps fails the handshake rather than silently skipping verification. Co-Authored-By: Claude Opus 5 <[email protected]>
9824713 to
49febc0
Compare
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 <[email protected]>
Address review feedback on apache#3813: security.ssl.reload.interval and client.security.ssl.reload.interval documented certificate hot-reload that this PR does not implement, and ConfigOptionsDocGenerator reflects over every ConfigOption field in ConfigOptions with no exclusion mechanism, so both would appear in the published configuration table as soon as it is regenerated. Remove both options and the SslConfig.reloadInterval field they fed; they belong in the PR that adds the reloader itself. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the missing-keystore check used Preconditions.checkArgument, which surfaces an IllegalArgumentException. Invalid Fluss configuration is reported as IllegalConfigurationException elsewhere in the codebase (LocalDiskManager and RemoteDirDynamicLoader on the server, WriterClient and ClientUtils on the client), so use that instead. The message is unchanged; the test asserts the new type. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: protocol and cipher suite names were only validated when an SSLEngine was created, which happens once per connection. A typo in security.ssl.enabled.protocols or security.ssl.cipher.suites let the server start cleanly and then failed every TLS connection with an engine-level "Unsupported protocol" / "Unsupported CipherSuite" that does not name the option at fault. An explicitly empty protocol list was worse: nothing failed, the engine came up with no protocol enabled and every handshake failed. SslConfig.fromServerConfig/fromClientConfig now reject, per side and naming the exact option, protocol and cipher suite names this JVM does not support (listing the supported ones), as well as an empty protocol list. Cipher suites are checked against JVM support only, not against the enabled protocols: which suites can be negotiated depends on the protocol agreed during the handshake. An empty cipher suite list stays valid and selects the provider defaults, as documented. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: SslContextFactory skips trustManager() when no truststore is configured, so the server SSL context falls back to the JVM default truststore. A listener that requires a client certificate would then accept any certificate issued by a public CA in cacerts, which is not the mTLS guarantee FIP-29 describes. SslConfig now reads security.protocol.map, derives which of the TLS-enabled listeners are mTLS listeners (matched case-insensitively, as AuthenticationFactory matches authentication protocol names), and refuses the configuration when any of them has no security.ssl.truststore.path. Listeners that are mTLS but not TLS-enabled impose nothing, and an encryption-only listener still needs no truststore. The requirement is exposed as requiresClientAuth(listener) / clientAuthListeners(), so the server pipeline wiring reads it from the parsed config rather than re-deriving it from the raw Configuration. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the client-auth test only asserted the needClientAuth flag, and PKCS12 was advertised in the keystore.type descriptions without a test. Extract the handshake pump from testServerAndClientNegotiateTls into a helper and add three tests on top of it: an mTLS server rejects a client that presents no certificate ("Empty client certificate chain"), accepts one that presents a trusted certificate, and completes the same mutual handshake with PKCS12 keystores and truststores on both sides. The rejection is asserted on the server handshake future only. Under TLS 1.3 the client sends its Finished before the server validates the certificate, so the client's future completes successfully and only learns of the rejection from the alert that follows. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: every test in SslContextFactoryTest errored on JDK 17 and only passed on 11, which CI does not catch because it runs the Java tests on 11 (and nightly on 8). Netty's SelfSignedCertificate generates the certificate with BouncyCastle when it is on the classpath and otherwise falls back to a generator built on sun.security.x509 internals. Add bcpkix-jdk18on in test scope so the first path is taken. No production dependency and nothing shaded, so the shipped artifacts are unchanged. Verified: 24/24 tests pass on JDK 11, 17 and 24 with this change, against 24 errors on 17 and 24 without it. The alternative --add-opens=java.base/sun.security.x509=ALL-UNNAMED in extraJavaTestArgs was tried too: it fixes JDK 17 but still fails on 24, which is why the dependency is the better answer. bcutil and bcprov are bcpkix's own transitive dependencies, pinned here because bcpkix declares them through version ranges — those resolve differently over time and make every build fetch their maven-metadata.xml. All three artifacts target Java 8 bytecode, so the nightly JDK 8 test run keeps working. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: SslConfig looks the listener up in security.protocol.map by exact name while comparing the protocol name with equalsIgnoreCase, which makes the option look more forgiving about case than it is. Both halves mirror how the server resolves them at runtime: FlussProtocolPlugin picks a listener's authenticator with a plain map lookup, and AuthenticationFactory matches a plugin to a protocol name with equalsIgnoreCase. Matching listener names loosely here would classify a listener as mTLS that the server then authenticates as PLAINTEXT, so the exact match stays and is documented instead: in the security.ssl.enabled.listeners description, and in a comment explaining why the two halves differ. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: a typo in security.ssl.keystore.type or any of its three siblings was only caught when the store was loaded, as a KeyStoreException from KeyStore.getInstance that does not name the option at fault. SslConfig now rejects a store type this JVM has no provider for, per side and naming the exact option, listing the types the JVM does support. Only the type of a store that is actually configured is checked, since the type of an absent store is never used to load anything. Type names stay as case-insensitive as JCA itself is. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: every key material failure collapsed into "Failed to build the server SSL context." with the reason only in the nested JCA exception, so an operator could not tell which of the four stores was at fault, let alone which option to fix. Each store now carries the options that configure it, and the failures an operator can actually cause are reported with the file and the option: a missing file names the path option, a store that will not open names the password option, and a private key that cannot be recovered names the key password option and the keystore password it falls back to. The generic wrapper is now only around the context build itself. The messages follow what the JDK really does: a wrong store password surfaces as IOException, a wrong key password as UnrecoverableKeyException, and a store loaded under the other type is not an error at all, since the JKS and PKCS12 providers read both formats. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the hostname verification test only read back the endpoint identification algorithm from the engine, so it asserted that a parameter was set rather than that it has any effect. The handshake helper now takes the host the client dials and the endpoint identification algorithm, and three tests run on top of it: a certificate issued for another host is rejected with the algorithm set to https, the same handshake succeeds when the certificate matches the host, and a server certificate outside the client truststore is rejected. The client is the rejecting side in all three, the mirror of the client-certificate test where the server rejects. Two details the assertions follow: Netty's SelfSignedCertificate carries no dNSName extension, so the JDK falls back to the subject CN and reports "No name matching localhost found"; and since both certificates in the truststore test are issued for localhost, the JDK finds a trust anchor by name and rejects it on the signature, which shows trust is decided by key rather than by subject. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the FIP-29 validation table requires a listener whose security.protocol.map entry is mTLS to also appear in security.ssl.enabled.listeners, and to refuse to start otherwise, since certificate authentication has no transport to run on without TLS. The derivation walked the TLS listeners and looked each one up in the protocol map, so an mTLS listener without TLS was filtered out before anything could complain, and a test asserted that as intended. The derivation now starts from the protocol map, and every mTLS listener missing from security.ssl.enabled.listeners is reported by name. The check runs before the early return taken when no listener enables TLS, so the worst case - an mTLS listener on a server with no TLS configured anywhere - is rejected too rather than reported as simply having no TLS. fromServerConfig can therefore throw while otherwise returning an empty Optional. testMtlsListenerWithoutTlsNeedsNoTruststore asserted the old behaviour and is flipped; the no-TLS-at-all case is covered by a new test. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the FIP-29 validation table has two client rows that were not implemented. A client configured with client.security.protocol=mTLS must have client.security.ssl.enabled set, since certificate authentication has no transport to run on otherwise, and must have client.security.ssl.keystore.path set, since it has no certificate to present without one. Both were accepted silently. fromClientConfig now rejects each with the option at fault named, before the early return taken when TLS is disabled, so the combination that turns TLS off outright is rejected rather than reported as a client with no TLS. The protocol name is compared ignoring case, as AuthenticationFactory compares it. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: a keystore with no key entry, which is what a truststore pointed at by mistake looks like, loaded and built a context without complaint. Verified what follows: the server handshake then fails with "No available authentication scheme" on every connection, and the client side reports no cause at all, so the connection just drops. The keystore is now checked for a key entry as it is loaded, on both the server and the client side, and reported with the file, the option that configures it and what the handshake would have done. Truststores are not checked, since holding only trusted certificates is what they are for. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: an unknown value of client.security.ssl.endpoint.identification.algorithm was accepted by SSLParameters and only rejected once a handshake ran, as "Unknown identification algorithm", i.e. on every connection. SslConfig now accepts https and ldaps ignoring case, which are the two the JDK implements, and an empty value, which disables hostname verification as documented. Anything else is rejected with the option named and the supported values listed. Verified what the JDK does with each: https and HTTPS and ldaps all run the check, an empty value skips it and the handshake succeeds, and htps fails the handshake rather than silently skipping verification. Co-Authored-By: Claude Opus 5 <[email protected]>
|
@polyzos @fresh-borzoni rebased onto main (49febc0) and addressed the latest round of comments. |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@MicheleGuerriero Thank you for the change, LGTM overall, some minor comments
| } | ||
| } | ||
|
|
||
| private static TrustManagerFactory trustManagerFactory( |
There was a problem hiding this comment.
A PKCS12 truststore with truststore.password unset loads with zero certificates, because PKCS12 encrypts them with that password. The context builds, then every handshake fails with the trustAnchors parameter must be non-empty. JKS loads fine without a password, so it's an easy mistake.
Shall we reject a truststore with no certificates, like the keystore check?
There was a problem hiding this comment.
Reproduced and fixed in d778e2e. The truststore is now checked for a trusted certificate as it's loaded, mirroring the keystore check, and the error names the file, the path option and the password option. There's also a test that a JKS truststore with no password still loads, so the check doesn't tighten the case that legitimately works today.
| * concern, not a configuration error. An empty cipher suite list is not an error either — it | ||
| * selects the provider defaults. | ||
| */ | ||
| private static void validateProtocolsAndCipherSuites( |
There was a problem hiding this comment.
On JDK 8u252 this default fails validation, because that JDK has no TLSv1.3. So TLS can't start there unless the option is set, though the description says it can.
Kafka defaults to TLSv1.2 alone on pre-11 JVMs. Could we drop unsupported entries from the default when the option isn't set? Or at least fix the description. Same for client.security.ssl.enabled.protocols.
There was a problem hiding this comment.
Thanks for flagging this, that one's a regression I introduced with the protocol validation. Fixed by narrowing, as you suggested.
| .withDescription( | ||
| "Listener names with TLS enabled, e.g. `CLIENT,INTERNAL`; others " | ||
| + "accept plaintext. Requires `security.ssl.keystore.path`, and " | ||
| + "is orthogonal to `security.protocol.map`. Names match exactly: " |
There was a problem hiding this comment.
nit: this isn't orthogonal to security.protocol.map anymore. An mTLS entry there now requires the listener to be listed here.
There was a problem hiding this comment.
right, replaced it with the actual relationship
| .stringType() | ||
| .noDefaultValue() | ||
| .withDescription( | ||
| "Truststore file holding the certificates the client trusts. Empty " |
There was a problem hiding this comment.
nit: the description says "Empty uses the JVM default", but "" fails with Failed to read the client truststore at ''.
It used to say "If unset", which matches the code. Shall we go back to that?
Address review feedback on apache#3813: validating the configured protocols against the running JVM also validated the default list, so a JDK without TLS 1.3 - 8u252 and older - refused to start TLS at all on a default nobody chose, while the option's description promised that JDK 8 is supported. An explicitly configured list is still validated strictly, so a typo fails at startup with the option named. An unset option now narrows the default to the protocols the JVM supports, which leaves TLS 1.2 on those JDKs and both protocols on 11 and later, and fails with the option named only when the JVM supports none of them. Both the server and the client option go through the same path. The narrowing is a separate pure function so the 8u252 case is tested directly rather than assumed, since the test JVM has TLS 1.3. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: a PKCS12 truststore keeps its contents under the store password and loads with no entries at all when that password is not configured, reporting nothing. The same certificate in a JKS store reads either way, so the mistake only appears on PKCS12. Verified: the context builds on both sides, and every handshake then fails on InvalidAlgorithmParameterException, the trustAnchors parameter must be non-empty, which names neither the file nor the option. The truststore is now checked for a trusted certificate as it is loaded, mirroring the keystore key check, and reported with the file, the path option and the password option. The check looks for a certificate entry, which is what the JDK trust manager collects its anchors from, so a store that holds only key entries is rejected as well. A JKS truststore without a password keeps working, covered by its own test. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: security.ssl.enabled.listeners said TLS is orthogonal to security.protocol.map, which stopped being true when an mTLS entry there started requiring the listener to be listed here. State that requirement instead. Co-Authored-By: Claude Opus 5 <[email protected]>
Address review feedback on apache#3813: the description said an empty value uses the JVM default truststore, but the code gates on the path being null, so unset falls back to the JVM default while an empty string is taken as a path and fails to load. The earlier "If unset" wording was correct and is restored; it was lost while shortening the descriptions to fit the file length limit. The other shortened descriptions that mention an empty value, security.ssl.cipher.suites and client.security.ssl.endpoint.identification.algorithm, are accurate: both treat unset and empty alike. Co-Authored-By: Claude Opus 5 <[email protected]>
|
@fresh-borzoni addressed the latest four comments. The workflow runs need approving again after the pushes. |
c6b3272 to
93116ac
Compare
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@MicheleGuerriero Thank you for the changes, LGTM 👍
I've folded some minor correction, I'll merge once CI pass
93116ac to
d45e03f
Compare
4 participants
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-sidesecurity.ssl.*options (enabledlisteners, 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 theabove options from a
Configuration, withfromServerConfig/fromClientConfigfactory methods. Fails fast with a clear messageif a server enables TLS without configuring a keystore.
SslContextFactory: new factory building NettySslContext/SslHandlerinstances from anSslConfig, using the JDK SSLprovider. Supports per-listener client-certificate requirement
(
setNeedClientAuth, for mTLS listeners) on the server side, andSNI + hostname verification on the client side.
Later tickets (#3792 server pipeline, #3797 client pipeline) will wire
SslContextFactoryinto the actual Netty channel pipelines.Tests
Added
SslContextFactoryTest, backed byTestSslUtils(generates aself-signed certificate and on-the-fly JKS keystore/truststore files,
no committed key material):
SslContextcreationverification on/off)
needClientAuthtoggling for mTLSclient
SslHandler, asserting the negotiated session uses a realTLS protocol/cipher (not plaintext)
Verified locally on
fluss-commonandfluss-rpc:mvn test(allgreen,
SslContextFactoryTest9/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
@Internalclasses influss-rpc(
org.apache.fluss.rpc.netty.ssl). No changes to existing publicAPIs, 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).