[FLINK-39635][security][rpc] Support comma-separated list in security.ssl.protocol#28160
[FLINK-39635][security][rpc] Support comma-separated list in security.ssl.protocol#28160balassai wants to merge 3 commits into
Conversation
| Arrays.stream(sslProtocolsString.split(",")) | ||
| .map(String::trim) | ||
| .toArray(String[]::new); | ||
| // Pekko's ConfigSSLEngineProvider uses the "protocol" field for both |
There was a problem hiding this comment.
I believe this comment was generated by AI. Can we shorten it for readability?
There was a problem hiding this comment.
Agree, removing the comment the method name is self explanatory.
| throw new IllegalStateException("Failed to query supported TLS protocols from JVM", e); | ||
| } | ||
| final Set<String> configured = new HashSet<>(Arrays.asList(configuredProtocols)); | ||
| // getSupportedSSLParameters().getProtocols() returns protocols from highest to lowest, |
There was a problem hiding this comment.
The inline comment in the method body says the opposite of JavaDoc.
There was a problem hiding this comment.
removed the comment.
| // so the first match in that list is the highest configured protocol. | ||
| return Arrays.stream(jvmOrdered) | ||
| .filter(configured::contains) | ||
| .findFirst() |
There was a problem hiding this comment.
nit: SSLParameters.getProtocols() doesn't specify any ordering — OpenJDK happens to return descending (hardcoded in AbstractTLSContext) but others return ascending, in which case
findFirst() silently picks the lowest version and the user gets a TLSv1.3 → TLSv1.2 downgrade with no failing test.
Could we compare versions explicitly instead? e.g. .max(Comparator.comparingInt(this::tlsRank)) with a small regex parsing the TLSv{major}.{minor} digits.
There was a problem hiding this comment.
Thanks! Creating a comparator.
| private static String highestSupportedProtocol(String[] configuredProtocols) { | ||
| final String[] jvmOrdered; | ||
| try { | ||
| SSLContext ctx = SSLContext.getInstance("TLS"); |
There was a problem hiding this comment.
nit(skippable): Could use SSLContext.getDefault().getSupportedSSLParameters().getProtocols() here — the default context is already initialized so it skips the getInstance + init(null,null,null) + try/catch dance. Same
result.
There was a problem hiding this comment.
Thanks! Using SSLContext.getDefault().getSupportedSSLParameters().getProtocols()
github-actions
Bot
added
the
community-reviewed
3 participants
What is the purpose of the change
security.ssl.protocolonly accepted a single TLS version. This change allows a comma-separated list (e.g."TLSv1.2,TLSv1.3") clusters can support multiple protocol versions at the same time.Brief change log
SecurityOptions.SSL_PROTOCOLdescription updated to reflect comma-separated supportprotocol = TLSand passes the individual versions viaenabled-protocolsCustomSSLEngineProvideroverridescreateServerSSLEngine/createClientSSLEngineto apply the enabled protocolsVerifying this change
This change added tests and can be verified as follows:
PekkoUtilsTest#getConfigSingleSslProtocolUsesGenericTlsContext— single value still works correctlyPekkoUtilsTest#getConfigCommaSeparatedSslProtocolsAreAllEnabled— comma-separated values are acceptedDoes this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?