[GitHub] [flink] gaborgsomogyi commented on a diff in pull request #21457: [FLINK-30309][runtime] Allow users to supply custom SslContexts

[GitBox <gi...@apache.org>]

gaborgsomogyi commented on code in PR #21457:
URL: https://github.com/apache/flink/pull/21457#discussion_r1040584906


##########
flink-core/src/main/java/org/apache/flink/configuration/SecurityOptions.java:
##########
@@ -217,6 +217,16 @@ public class SecurityOptions {
                     .withDescription(
                             "Turns on SSL for external communication via the REST endpoints.");
 
+    @Documentation.Section(Documentation.Sections.EXPERT_SECURITY_SSL)
+    public static final ConfigOption<String> SSL_REST_SSL_CONTEXT_SUPPLIER =
+            key("security.ssl.rest.ssl-context-supplier")
+                    .stringType()
+                    .noDefaultValue()
+                    .withDescription(
+                            "A fully qualified class name that implements the Supplier<SslContext> interface."
+                                    + " The implementation must have a public constructor with the signature"
+                                    + " (Configuration config, boolean isClient, SslProvider sslProvider)");

Review Comment:
   Adding an interface would enforce type safety and doesn't require to put the contract into documentation, please see `SecurityModule` or something like that.



##########
flink-runtime/src/main/java/org/apache/flink/runtime/net/SSLUtils.java:
##########
@@ -90,44 +96,78 @@ public static ServerSocketFactory createSSLServerSocketFactory(Configuration con
      */
     public static SocketFactory createSSLClientSocketFactory(Configuration config)
             throws Exception {
-        SSLContext sslContext = createInternalSSLContext(config, true);
+        Supplier<SSLContext> sslContext = createInternalSSLContext(config, true);

Review Comment:
   `createInternalSSLContext` -> this is not SSLContext but a supplier.
   
   This is a general suggestion to all other places. Where we give back a supplier then it would be good to call it `get...Supplier` or something. The same applies to variable names.



##########
flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java:
##########
@@ -230,6 +266,30 @@ public void testInternalSSL() throws Exception {
         assertNotNull(SSLUtils.createInternalClientSSLEngineFactory(config));
     }
 
+    @Test
+    public void testInternalSSLServerCustomSupplier() throws Exception {
+        final Configuration config = createInternalSslConfigWithCustomSupplier();
+        SSLHandlerFactory sslHandlerFactory = SSLUtils.createInternalServerSSLEngineFactory(config);
+        assertFalse(TestSslContextSupplier.lastInstance().isClient);

Review Comment:
   `TestSslContextSupplier.reset();` is simply missing.
   



##########
flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java:
##########
@@ -438,6 +498,46 @@ public void testInvalidFingerprintParsing() throws Exception {
 
     // ------------------------------- utils ----------------------------------
 
+    private static class TestSslContextSupplier implements Supplier<SslContext> {
+        private static final ThreadLocal<Integer> callCount = ThreadLocal.withInitial(() -> 0);

Review Comment:
   What is the plan here w/ `ThreadLocal`?



##########
flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java:
##########
@@ -230,6 +266,30 @@ public void testInternalSSL() throws Exception {
         assertNotNull(SSLUtils.createInternalClientSSLEngineFactory(config));
     }
 
+    @Test
+    public void testInternalSSLServerCustomSupplier() throws Exception {
+        final Configuration config = createInternalSslConfigWithCustomSupplier();
+        SSLHandlerFactory sslHandlerFactory = SSLUtils.createInternalServerSSLEngineFactory(config);
+        assertFalse(TestSslContextSupplier.lastInstance().isClient);
+
+        sslHandlerFactory.createNettySSLHandler(ByteBufAllocator.DEFAULT);
+
+        // our supplier should have been called at least once
+        MatcherAssert.assertThat(TestSslContextSupplier.callCount(), Matchers.greaterThan(0));
+    }
+
+    @Test
+    public void testInternalSSLClientCustomSupplier() throws Exception {
+        final Configuration config = createInternalSslConfigWithCustomSupplier();
+        SSLHandlerFactory sslHandlerFactory = SSLUtils.createInternalClientSSLEngineFactory(config);
+        assertTrue(TestSslContextSupplier.lastInstance().isClient);

Review Comment:
   `TestSslContextSupplier.reset();` is simply missing.
   



##########
flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java:
##########
@@ -171,6 +179,20 @@ public void testRESTClientSSLWrongPassword() throws Exception {
         }
     }
 
+    @Test
+    public void testRESTClientSSLCustomSupplier() throws Exception {

Review Comment:
   These tests are copy-pastes which can be simplified.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org