[FLINK-9816][network] add option to configure SSL engine provider for TM communication #6328
Conversation
… TM communication This prepares Flink to use OpenSSL for TM communication channels via netty. Currently, there is no easy way to provide the required native libraries, though. We'll either include these in a future version of flink-shaded or update instructions on how to include/build them manually.
|
Could you rebase this on top of #6326 ? That PR makes sure SSLEngine factories are used everywhere, giving a single point to integrate the provider such that it is available for all SSL endpoints. |
| @@ -160,6 +160,7 @@ private Configuration createSslConfig() throws Exception { | |||
| flinkConfig.setString(SecurityOptions.SSL_KEY_PASSWORD, "password"); | |||
| flinkConfig.setString(SecurityOptions.SSL_TRUSTSTORE, "src/test/resources/local127.truststore"); | |||
| flinkConfig.setString(SecurityOptions.SSL_TRUSTSTORE_PASSWORD, "password"); | |||
| // flinkConfig.setString(SecurityOptions.SSL_PROVIDER, "OPENSSL"); | |||
There was a problem hiding this comment.
if this is a useless dead code, can be removed
| */ | ||
| @Nullable | ||
| public static SSLContext createSSLServerContext(Configuration sslConfig) throws Exception { | ||
|
|
There was a problem hiding this comment.
this empty line is useless, can be removed
| * Creates the SSL Context for the server if SSL is configured. | ||
| * | ||
| * @param sslConfig | ||
| * The application configuration |
There was a problem hiding this comment.
the description of the param and throws do not need linefeed
| * configured. | ||
| * | ||
| * @param sslConfig | ||
| * The application configuration |
| /** | ||
| * Instances needed to set up an SSL client connection. | ||
| */ | ||
| public static class SSLServerTools { |
| /** | ||
| * Instances needed to set up an SSL client connection. | ||
| */ | ||
| public static class SSLClientTools { |
There was a problem hiding this comment.
the class name use singular looks better to me
| public static class SSLClientTools { | ||
| public final SSLProvider preferredSslProvider; | ||
| public final String sslProtocolVersion; | ||
| public final TrustManagerFactory trustManagerFactory; |
There was a problem hiding this comment.
mark these fields as private as provide getter/setter looks better to me
|
|
||
| public static SSLProvider fromString(String value) { | ||
| Preconditions.checkNotNull(value); | ||
| if (value.equalsIgnoreCase("OPENSSL")) { |
There was a problem hiding this comment.
provide a constructor like SSLProvider(String provider) to give the enum's string representation looks better than hard code.
What is the purpose of the change
Netty has the ability to run with different
SSLEngineimplementations but with our current setup, we are fixed to the JDK implementation, although one based on OpenSSL is expected to be faster [1].We should make this configurable and ideally also provide everything needed to run with OpenSSL in the future (the last part is not part of this PR).
[1] https://netty.io/wiki/requirements-for-4.x.html#benefits-of-using-openssl
Brief change log
security.ssl.providerSslContextBuilder(inNettyConfig) to have this flexibilityVerifying this change
This change can be verified as follows:
JDKSSL engine andOPENSSLusing a custom build usingnetty-tcnativewith statically linked boringssl libraries from http://netty.io/wiki/forked-tomcat-native.htmlDoes this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation