spuru9 commented on code in PR #28160:
URL: https://github.com/apache/flink/pull/28160#discussion_r3247887330
##########
flink-rpc/flink-rpc-akka/src/main/java/org/apache/flink/runtime/rpc/pekko/PekkoUtils.java:
##########
@@ -352,7 +355,21 @@ private static void addSslRemoteConfig(
.collect(Collectors.joining("\",\"", "[\"",
"\"]"))
: "[]";
- final String sslProtocol =
configuration.get(SecurityOptions.SSL_PROTOCOL);
+ final String sslProtocolsString =
configuration.get(SecurityOptions.SSL_PROTOCOL);
+ final String[] sslProtocolsArray =
+ Arrays.stream(sslProtocolsString.split(","))
+ .map(String::trim)
+ .toArray(String[]::new);
+ // Pekko's ConfigSSLEngineProvider uses the "protocol" field for both
Review Comment:
I believe this comment was generated by AI. Can we shorten it for
readability?
##########
flink-rpc/flink-rpc-akka/src/main/java/org/apache/flink/runtime/rpc/pekko/PekkoUtils.java:
##########
@@ -610,6 +628,38 @@ private static String booleanToOnOrOff(boolean flag) {
return flag ? "on" : "off";
}
+ /**
+ * Returns the highest TLS protocol from {@code configuredProtocols}
according to the JVM's own
+ * ordering of supported protocols.
+ *
+ * <p>The JVM's {@link SSLContext#getSupportedSSLParameters()} returns
protocols ordered from
+ * lowest to highest, so this method is automatically correct for any
future TLS version without
+ * requiring a manually maintained ranking list.
+ *
+ * @throws IllegalArgumentException if none of the configured protocols
are supported by the JVM
+ */
+ private static String highestSupportedProtocol(String[]
configuredProtocols) {
+ final String[] jvmOrdered;
+ try {
+ SSLContext ctx = SSLContext.getInstance("TLS");
+ ctx.init(null, null, null);
+ jvmOrdered = ctx.getSupportedSSLParameters().getProtocols();
+ } catch (Exception e) {
+ 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,
Review Comment:
The inline comment in the method body says the opposite of JavaDoc.
##########
flink-rpc/flink-rpc-akka/src/main/java/org/apache/flink/runtime/rpc/pekko/PekkoUtils.java:
##########
@@ -610,6 +628,38 @@ private static String booleanToOnOrOff(boolean flag) {
return flag ? "on" : "off";
}
+ /**
+ * Returns the highest TLS protocol from {@code configuredProtocols}
according to the JVM's own
+ * ordering of supported protocols.
+ *
+ * <p>The JVM's {@link SSLContext#getSupportedSSLParameters()} returns
protocols ordered from
+ * lowest to highest, so this method is automatically correct for any
future TLS version without
+ * requiring a manually maintained ranking list.
+ *
+ * @throws IllegalArgumentException if none of the configured protocols
are supported by the JVM
+ */
+ private static String highestSupportedProtocol(String[]
configuredProtocols) {
+ final String[] jvmOrdered;
+ try {
+ SSLContext ctx = SSLContext.getInstance("TLS");
Review 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.
##########
flink-rpc/flink-rpc-akka/src/main/java/org/apache/flink/runtime/rpc/pekko/PekkoUtils.java:
##########
@@ -610,6 +628,38 @@ private static String booleanToOnOrOff(boolean flag) {
return flag ? "on" : "off";
}
+ /**
+ * Returns the highest TLS protocol from {@code configuredProtocols}
according to the JVM's own
+ * ordering of supported protocols.
+ *
+ * <p>The JVM's {@link SSLContext#getSupportedSSLParameters()} returns
protocols ordered from
+ * lowest to highest, so this method is automatically correct for any
future TLS version without
+ * requiring a manually maintained ranking list.
+ *
+ * @throws IllegalArgumentException if none of the configured protocols
are supported by the JVM
+ */
+ private static String highestSupportedProtocol(String[]
configuredProtocols) {
+ final String[] jvmOrdered;
+ try {
+ SSLContext ctx = SSLContext.getInstance("TLS");
+ ctx.init(null, null, null);
+ jvmOrdered = ctx.getSupportedSSLParameters().getProtocols();
+ } catch (Exception e) {
+ 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,
+ // so the first match in that list is the highest configured protocol.
+ return Arrays.stream(jvmOrdered)
+ .filter(configured::contains)
+ .findFirst()
Review 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.
--
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]
