gaborgsomogyi commented on code in PR #24919:
URL: https://github.com/apache/flink/pull/24919#discussion_r1633123484
##########
flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java:
##########
@@ -148,6 +148,17 @@ void testRESTClientSSLWrongPassword(String sslProvider) {
.isInstanceOf(Exception.class);
}
+ /** Tests that REST Client SSL Engine creation fails with bad SSL
configuration. */
+ @ParameterizedTest
+ @MethodSource("parameters")
+ void testRESTClientSSLBadTruststoreType(String sslProvider) {
+ Configuration clientConfig =
createRestSslConfigWithTrustStore(sslProvider);
+ clientConfig.set(SecurityOptions.SSL_REST_TRUSTSTORE_TYPE, "BKS");
Review Comment:
Can we follow the pattern what we've already? I mean `bad-truststore-type`
or something. This applies to all other places
##########
flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java:
##########
@@ -306,6 +341,19 @@ void testInternalSSLWrongKeyPassword(String sslProvider) {
.isInstanceOf(Exception.class);
}
+ @ParameterizedTest
+ @MethodSource("parameters")
+ void testInternalSSLWrongKeystoreType(String sslProvider) {
+ final Configuration config =
createInternalSslConfigWithKeyAndTrustStores(sslProvider);
+ config.set(SecurityOptions.SSL_INTERNAL_KEYSTORE_TYPE, "JCEKS");
+
+ assertThatThrownBy(() ->
SSLUtils.createInternalServerSSLEngineFactory(config))
+ .isInstanceOf(java.io.IOException.class);
+
+ assertThatThrownBy(() ->
SSLUtils.createInternalClientSSLEngineFactory(config))
+ .isInstanceOf(java.io.IOException.class);
Review Comment:
Here too.
##########
flink-rpc/flink-rpc-akka/src/main/java/org/apache/flink/runtime/rpc/pekko/CustomSSLEngineProvider.java:
##########
@@ -59,13 +71,38 @@ public TrustManager[] trustManagers() {
.fingerprints(sslCertFingerprints)
.build();
- trustManagerFactory.init(loadKeystore(sslTrustStore,
sslTrustStorePassword));
+ trustManagerFactory.init(
+ loadKeystore(sslTrustStore, sslTrustStorePassword,
sslTrustStoreType));
return trustManagerFactory.getTrustManagers();
- } catch (GeneralSecurityException e) {
+ } catch (GeneralSecurityException | IOException e) {
// replicate exception handling from SSLEngineProvider
throw new RemoteTransportException(
"Server SSL connection could not be established because
SSL context could not be constructed",
e);
}
}
+
+ @Override
+ public KeyStore loadKeystore(String filename, String password) {
+ try {
+ return loadKeystore(filename, password, sslKeyStoreType);
+ } catch (IOException
+ | CertificateException
+ | NoSuchAlgorithmException
+ | KeyStoreException e) {
+ throw new RemoteTransportException(
Review Comment:
What is the plan here with the wrapping? Couple of lines before I see the
same wrapping pattern. It would be good to have a behavior like we've in Scala.
##########
docs/layouts/shortcodes/generated/security_configuration.html:
##########
@@ -134,6 +134,12 @@
<td>String</td>
<td>The secret to decrypt the keystore file for Flink's for
Flink's internal endpoints (rpc, data transport, blob server).</td>
</tr>
+ <tr>
+ <td><h5>security.ssl.internal.keystore-type</h5></td>
+ <td style="word-wrap: break-word;">"pkcs12"</td>
Review Comment:
This is JVM version dependent. Up until 1.8 `jks`, 1.9+ `pkcs12` + it can be
overwritten with `keystore.type`, right? I would write here JVM detected
default version.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java:
##########
@@ -293,6 +315,19 @@ void testInternalSSLWrongTruststorePassword(String
sslProvider) {
.isInstanceOf(Exception.class);
}
+ @ParameterizedTest
+ @MethodSource("parameters")
+ void testInternalSSLWrongTruststoreType(String sslProvider) {
+ final Configuration config =
createInternalSslConfigWithKeyAndTrustStores(sslProvider);
+ config.set(SecurityOptions.SSL_INTERNAL_TRUSTSTORE_TYPE, "BKS");
+
+ assertThatThrownBy(() ->
SSLUtils.createInternalServerSSLEngineFactory(config))
+ .isInstanceOf(java.security.KeyStoreException.class);
Review Comment:
I've the feeling that `java.security.` can be eliminated with an import,
right?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java:
##########
@@ -306,6 +341,19 @@ void testInternalSSLWrongKeyPassword(String sslProvider) {
.isInstanceOf(Exception.class);
}
+ @ParameterizedTest
+ @MethodSource("parameters")
+ void testInternalSSLWrongKeystoreType(String sslProvider) {
+ final Configuration config =
createInternalSslConfigWithKeyAndTrustStores(sslProvider);
+ config.set(SecurityOptions.SSL_INTERNAL_KEYSTORE_TYPE, "JCEKS");
+
+ assertThatThrownBy(() ->
SSLUtils.createInternalServerSSLEngineFactory(config))
+ .isInstanceOf(java.io.IOException.class);
Review Comment:
I've the feeling that `java.io.` can be eliminated with an import, right?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java:
##########
@@ -293,6 +315,19 @@ void testInternalSSLWrongTruststorePassword(String
sslProvider) {
.isInstanceOf(Exception.class);
}
+ @ParameterizedTest
+ @MethodSource("parameters")
+ void testInternalSSLWrongTruststoreType(String sslProvider) {
+ final Configuration config =
createInternalSslConfigWithKeyAndTrustStores(sslProvider);
+ config.set(SecurityOptions.SSL_INTERNAL_TRUSTSTORE_TYPE, "BKS");
+
+ assertThatThrownBy(() ->
SSLUtils.createInternalServerSSLEngineFactory(config))
+ .isInstanceOf(java.security.KeyStoreException.class);
+
+ assertThatThrownBy(() ->
SSLUtils.createInternalClientSSLEngineFactory(config))
+ .isInstanceOf(java.security.KeyStoreException.class);
Review Comment:
Here too.
--
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]
