Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.0
    • Component/s: Security
    • Labels:
      None

      Description

      I configured ssl and start flink job, but found configured properties cannot apply properly:
      akka port: only ciper suites apply right, ssl version not
      blob server/netty server: both ssl version and ciper suites are not like what I configured

      I've found out the reason why:
      http://stackoverflow.com/questions/11504173/sslcontext-initialization (for blob server and netty server)
      https://groups.google.com/forum/#!topic/akka-user/JH6bGnWE8kY(for akka ssl version, it's fixed in akka 2.4:https://github.com/akka/akka/pull/21078)

      I'll fix the issue on blob server and netty server, and it seems like only upgrade for akka can solve issue in akka side(we'll consider later as upgrade is not a small action).

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user WangTaoTheTonic opened a pull request:

          https://github.com/apache/flink/pull/3486

          FLINK-5981[SECURITY]make ssl version and cipher suites work as configured

          I configured ssl and start flink job, but found configured properties cannot apply properly:
          ```
          akka port: only ciper suites apply right, ssl version not
          blob server/netty server: both ssl version and ciper suites are not like what I configured
          ```
          I've found out the reason why:

          http://stackoverflow.com/questions/11504173/sslcontext-initialization (for blob server and netty server)
          https://groups.google.com/forum/#!topic/akka-user/JH6bGnWE8kY(for akka ssl version, it's fixed in akka 2.4:https://github.com/akka/akka/pull/21078)

          Configs:
          ```
          security.ssl.protocol: TLSv1.1

          security.ssl.algorithms: TLS_RSA_WITH_AES_128_CBC_SHA
          ```
          *The scan results before:*
          ![before_blob_server](https://cloud.githubusercontent.com/assets/5276001/23655830/d37eb680-0371-11e7-952c-4a6514b1c42b.JPG)
          *The scan results after fix:*
          ![after_blob_server](https://cloud.githubusercontent.com/assets/5276001/23655841/dfc09da0-0371-11e7-8486-bc807e877dff.JPG)

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/WangTaoTheTonic/flink FLINK-5981

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/flink/pull/3486.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #3486


          commit c75c2e3f38e0a856ead1316223ad3d81061e4252
          Author: WangTaoTheTonic <wangtao111@huawei.com>
          Date: 2017-03-07T12:05:21Z

          make ssl version and cipher suites work as configured


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user WangTaoTheTonic opened a pull request: https://github.com/apache/flink/pull/3486 FLINK-5981 [SECURITY] make ssl version and cipher suites work as configured I configured ssl and start flink job, but found configured properties cannot apply properly: ``` akka port: only ciper suites apply right, ssl version not blob server/netty server: both ssl version and ciper suites are not like what I configured ``` I've found out the reason why: http://stackoverflow.com/questions/11504173/sslcontext-initialization (for blob server and netty server) https://groups.google.com/forum/#!topic/akka-user/JH6bGnWE8kY(for akka ssl version, it's fixed in akka 2.4: https://github.com/akka/akka/pull/21078 ) Configs: ``` security.ssl.protocol: TLSv1.1 security.ssl.algorithms: TLS_RSA_WITH_AES_128_CBC_SHA ``` * The scan results before: * ! [before_blob_server] ( https://cloud.githubusercontent.com/assets/5276001/23655830/d37eb680-0371-11e7-952c-4a6514b1c42b.JPG ) * The scan results after fix: * ! [after_blob_server] ( https://cloud.githubusercontent.com/assets/5276001/23655841/dfc09da0-0371-11e7-8486-bc807e877dff.JPG ) You can merge this pull request into a Git repository by running: $ git pull https://github.com/WangTaoTheTonic/flink FLINK-5981 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3486.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #3486 commit c75c2e3f38e0a856ead1316223ad3d81061e4252 Author: WangTaoTheTonic <wangtao111@huawei.com> Date: 2017-03-07T12:05:21Z make ssl version and cipher suites work as configured
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3486

          Thanks for the patch!

          @EronWright , @vijikarthi You are probably the most knowledgeable ones about that code, what do you think about this change?

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3486 Thanks for the patch! @EronWright , @vijikarthi You are probably the most knowledgeable ones about that code, what do you think about this change?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3486#discussion_r104704130

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/net/SSLUtils.java —
          @@ -55,6 +58,42 @@ public static boolean getSSLEnabled(Configuration sslConfig) {
          }

          /**
          + * Sets SSl version and cipher suites for SSLServerSocket
          + * @param socket
          + * Socket to be handled
          + * @param config
          + * The application configuration
          + */
          + public static void setSSLVerAndCipherSuites(ServerSocket socket, Configuration config) {
          + if (socket instanceof SSLServerSocket) {
          + ((SSLServerSocket) socket).setEnabledProtocols(config.getString(
          + ConfigConstants.SECURITY_SSL_PROTOCOL,
          + ConfigConstants.DEFAULT_SECURITY_SSL_PROTOCOL).split(","));
          — End diff –

          simply calling `split()` on a config value may lead to errors with bad error messages when the config value is misconfigured. I think it would be nice to do some more explicit handling here.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3486#discussion_r104704130 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/net/SSLUtils.java — @@ -55,6 +58,42 @@ public static boolean getSSLEnabled(Configuration sslConfig) { } /** + * Sets SSl version and cipher suites for SSLServerSocket + * @param socket + * Socket to be handled + * @param config + * The application configuration + */ + public static void setSSLVerAndCipherSuites(ServerSocket socket, Configuration config) { + if (socket instanceof SSLServerSocket) { + ((SSLServerSocket) socket).setEnabledProtocols(config.getString( + ConfigConstants.SECURITY_SSL_PROTOCOL, + ConfigConstants.DEFAULT_SECURITY_SSL_PROTOCOL).split(",")); — End diff – simply calling `split()` on a config value may lead to errors with bad error messages when the config value is misconfigured. I think it would be nice to do some more explicit handling here.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user WangTaoTheTonic commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3486#discussion_r104828499

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/net/SSLUtils.java —
          @@ -55,6 +58,42 @@ public static boolean getSSLEnabled(Configuration sslConfig) {
          }

          /**
          + * Sets SSl version and cipher suites for SSLServerSocket
          + * @param socket
          + * Socket to be handled
          + * @param config
          + * The application configuration
          + */
          + public static void setSSLVerAndCipherSuites(ServerSocket socket, Configuration config) {
          + if (socket instanceof SSLServerSocket) {
          + ((SSLServerSocket) socket).setEnabledProtocols(config.getString(
          + ConfigConstants.SECURITY_SSL_PROTOCOL,
          + ConfigConstants.DEFAULT_SECURITY_SSL_PROTOCOL).split(","));
          — End diff –

          By "do explicit handing", do you mean we should check if the elements in split resutls are both legal(must be one of `SSLv3, TLSv1, TLSv1.1....`)?

          BTW/FYI: This config also applies to akka, and it seems like akka only support single value setting but not multiple ones(It throws exception when I set the value to `TLSv1.1,TLSv1.2`).

          Show
          githubbot ASF GitHub Bot added a comment - Github user WangTaoTheTonic commented on a diff in the pull request: https://github.com/apache/flink/pull/3486#discussion_r104828499 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/net/SSLUtils.java — @@ -55,6 +58,42 @@ public static boolean getSSLEnabled(Configuration sslConfig) { } /** + * Sets SSl version and cipher suites for SSLServerSocket + * @param socket + * Socket to be handled + * @param config + * The application configuration + */ + public static void setSSLVerAndCipherSuites(ServerSocket socket, Configuration config) { + if (socket instanceof SSLServerSocket) { + ((SSLServerSocket) socket).setEnabledProtocols(config.getString( + ConfigConstants.SECURITY_SSL_PROTOCOL, + ConfigConstants.DEFAULT_SECURITY_SSL_PROTOCOL).split(",")); — End diff – By "do explicit handing", do you mean we should check if the elements in split resutls are both legal(must be one of `SSLv3, TLSv1, TLSv1.1....`)? BTW/FYI: This config also applies to akka, and it seems like akka only support single value setting but not multiple ones(It throws exception when I set the value to `TLSv1.1,TLSv1.2`).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3486#discussion_r105371699

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/net/SSLUtils.java —
          @@ -55,6 +58,42 @@ public static boolean getSSLEnabled(Configuration sslConfig) {
          }

          /**
          + * Sets SSl version and cipher suites for SSLServerSocket
          + * @param socket
          + * Socket to be handled
          + * @param config
          + * The application configuration
          + */
          + public static void setSSLVerAndCipherSuites(ServerSocket socket, Configuration config) {
          + if (socket instanceof SSLServerSocket) {
          + ((SSLServerSocket) socket).setEnabledProtocols(config.getString(
          + ConfigConstants.SECURITY_SSL_PROTOCOL,
          + ConfigConstants.DEFAULT_SECURITY_SSL_PROTOCOL).split(","));
          — End diff –

          I think you are right, it is probably hard to do good verification here, and better to rely on the verification by the `SSLServerSocket`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3486#discussion_r105371699 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/net/SSLUtils.java — @@ -55,6 +58,42 @@ public static boolean getSSLEnabled(Configuration sslConfig) { } /** + * Sets SSl version and cipher suites for SSLServerSocket + * @param socket + * Socket to be handled + * @param config + * The application configuration + */ + public static void setSSLVerAndCipherSuites(ServerSocket socket, Configuration config) { + if (socket instanceof SSLServerSocket) { + ((SSLServerSocket) socket).setEnabledProtocols(config.getString( + ConfigConstants.SECURITY_SSL_PROTOCOL, + ConfigConstants.DEFAULT_SECURITY_SSL_PROTOCOL).split(",")); — End diff – I think you are right, it is probably hard to do good verification here, and better to rely on the verification by the `SSLServerSocket`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3486

          I think this is a good fix.
          Is it possible to add some form of test that makes sure the config is properly applied in all cases? What do you think about adding to the `SSLUtilsTest` class and a case in the `AkkaSslITCase` that validates that the correct protocols are chosen?

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3486 I think this is a good fix. Is it possible to add some form of test that makes sure the config is properly applied in all cases? What do you think about adding to the `SSLUtilsTest` class and a case in the `AkkaSslITCase` that validates that the correct protocols are chosen?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user WangTaoTheTonic commented on the issue:

          https://github.com/apache/flink/pull/3486

          @StephanEwen I've added some test cases for testing new function and a ITCase to prove akka cannot accept more than one protocol setting. Let me know if there's better way to implementation

          Show
          githubbot ASF GitHub Bot added a comment - Github user WangTaoTheTonic commented on the issue: https://github.com/apache/flink/pull/3486 @StephanEwen I've added some test cases for testing new function and a ITCase to prove akka cannot accept more than one protocol setting. Let me know if there's better way to implementation
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vijikarthi commented on the issue:

          https://github.com/apache/flink/pull/3486

          I think the patch looks good. Is there a specific protocol version + cipher suite combination sets that the user should be aware which needs to be documented?

          Show
          githubbot ASF GitHub Bot added a comment - Github user vijikarthi commented on the issue: https://github.com/apache/flink/pull/3486 I think the patch looks good. Is there a specific protocol version + cipher suite combination sets that the user should be aware which needs to be documented?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user WangTaoTheTonic commented on the issue:

          https://github.com/apache/flink/pull/3486

          thanks for review @vijikarthi. I will check if there are mismatch between protocols and cipher suites and document it if any.

          Show
          githubbot ASF GitHub Bot added a comment - Github user WangTaoTheTonic commented on the issue: https://github.com/apache/flink/pull/3486 thanks for review @vijikarthi. I will check if there are mismatch between protocols and cipher suites and document it if any.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3486#discussion_r105910429

          — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServer.java —
          @@ -168,7 +168,8 @@ public ServerSocket createSocket(int port) throws IOException {
          if(socketAttempt == null)

          { throw new IOException("Unable to allocate socket for blob server in specified port range: "+serverPortRange); }

          else {

          • this.serverSocket = socketAttempt;
            + SSLUtils.setSSLVerAndCipherSuites(socketAttempt, config);
            + this.serverSocket = socketAttempt;
              • End diff –

          Indentation error?

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3486#discussion_r105910429 — Diff: flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServer.java — @@ -168,7 +168,8 @@ public ServerSocket createSocket(int port) throws IOException { if(socketAttempt == null) { throw new IOException("Unable to allocate socket for blob server in specified port range: "+serverPortRange); } else { this.serverSocket = socketAttempt; + SSLUtils.setSSLVerAndCipherSuites(socketAttempt, config); + this.serverSocket = socketAttempt; End diff – Indentation error?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3486#discussion_r105910791

          — Diff: flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java —
          @@ -125,4 +129,99 @@ public void testCreateSSLServerContextMisconfiguration() {
          }
          }

          + /**
          + * Tests if SSL Server Context creation fails with bad SSL configuration
          + */
          + @Test
          + public void testCreateSSLServerContextWithMultiProtocols() {
          +
          + Configuration serverConfig = new Configuration();
          + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true);
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1,TLSv1.2");
          +
          + try

          { + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig); + Assert.fail("SSL server context created even with multiple protocols set "); + }

          catch (Exception e)

          { + // Exception here is valid + }

          + }
          +
          + /**
          + * Tests if SSLUtils set the right ssl version and cipher suites for SSLServerSocket
          + */
          + @Test
          + public void testSetSSLVersionAndCipherSuitesForSSLServerSocket() throws Exception {
          +
          + Configuration serverConfig = new Configuration();
          + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true);
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1.1");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_ALGORITHMS, "TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_128_CBC_SHA256");
          +
          + int port = new Random().nextInt(65535);
          + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig);
          + ServerSocket socket = null;
          + try {
          + socket = serverContext.getServerSocketFactory().createServerSocket(port);
          +
          + String[] protocols = ((SSLServerSocket) socket).getEnabledProtocols();
          + String[] algorithms = ((SSLServerSocket) socket).getEnabledCipherSuites();
          + Assert.assertTrue(protocols.length > 1);
          + Assert.assertTrue(algorithms.length > 2);
          +
          + SSLUtils.setSSLVerAndCipherSuites(socket, serverConfig);
          + protocols = ((SSLServerSocket) socket).getEnabledProtocols();
          + algorithms = ((SSLServerSocket) socket).getEnabledCipherSuites();
          + Assert.assertTrue(protocols.length == 1);
          — End diff –

          Using `assertEquals` often helps in failing tests, because it prints expected and actual values. `assertTrue` gives no good error message (other than that the condition was violated).

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3486#discussion_r105910791 — Diff: flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java — @@ -125,4 +129,99 @@ public void testCreateSSLServerContextMisconfiguration() { } } + /** + * Tests if SSL Server Context creation fails with bad SSL configuration + */ + @Test + public void testCreateSSLServerContextWithMultiProtocols() { + + Configuration serverConfig = new Configuration(); + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1,TLSv1.2"); + + try { + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig); + Assert.fail("SSL server context created even with multiple protocols set "); + } catch (Exception e) { + // Exception here is valid + } + } + + /** + * Tests if SSLUtils set the right ssl version and cipher suites for SSLServerSocket + */ + @Test + public void testSetSSLVersionAndCipherSuitesForSSLServerSocket() throws Exception { + + Configuration serverConfig = new Configuration(); + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1.1"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_ALGORITHMS, "TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_128_CBC_SHA256"); + + int port = new Random().nextInt(65535); + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig); + ServerSocket socket = null; + try { + socket = serverContext.getServerSocketFactory().createServerSocket(port); + + String[] protocols = ((SSLServerSocket) socket).getEnabledProtocols(); + String[] algorithms = ((SSLServerSocket) socket).getEnabledCipherSuites(); + Assert.assertTrue(protocols.length > 1); + Assert.assertTrue(algorithms.length > 2); + + SSLUtils.setSSLVerAndCipherSuites(socket, serverConfig); + protocols = ((SSLServerSocket) socket).getEnabledProtocols(); + algorithms = ((SSLServerSocket) socket).getEnabledCipherSuites(); + Assert.assertTrue(protocols.length == 1); — End diff – Using `assertEquals` often helps in failing tests, because it prints expected and actual values. `assertTrue` gives no good error message (other than that the condition was violated).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user WangTaoTheTonic commented on the issue:

          https://github.com/apache/flink/pull/3486

          @vijikarthi I've checked the [JDK doc](http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites) and not found any notes about combination of ssl version and ciper suites.
          About cihper suites, it says `The following list contains the standard JSSE cipher suite names. Over time, various groups have added additional cipher suites to the SSL/TLS namespace. `, so i think we better not add additional description about that and let user to follow JRE/JDK rules.

          @StephanEwen Two comments you mentioned was fixed

          Show
          githubbot ASF GitHub Bot added a comment - Github user WangTaoTheTonic commented on the issue: https://github.com/apache/flink/pull/3486 @vijikarthi I've checked the [JDK doc] ( http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites ) and not found any notes about combination of ssl version and ciper suites. About cihper suites, it says `The following list contains the standard JSSE cipher suite names. Over time, various groups have added additional cipher suites to the SSL/TLS namespace. `, so i think we better not add additional description about that and let user to follow JRE/JDK rules. @StephanEwen Two comments you mentioned was fixed
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on a diff in the pull request:

          https://github.com/apache/flink/pull/3486#discussion_r106140551

          — Diff: flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java —
          @@ -125,4 +129,101 @@ public void testCreateSSLServerContextMisconfiguration() {
          }
          }

          + /**
          + * Tests if SSL Server Context creation fails with bad SSL configuration
          + */
          + @Test
          + public void testCreateSSLServerContextWithMultiProtocols() {
          +
          + Configuration serverConfig = new Configuration();
          + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true);
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1,TLSv1.2");
          +
          + try

          { + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig); + Assert.fail("SSL server context created even with multiple protocols set "); + }

          catch (Exception e)

          { + // Exception here is valid + }

          + }
          +
          + /**
          + * Tests if SSLUtils set the right ssl version and cipher suites for SSLServerSocket
          + */
          + @Test
          + public void testSetSSLVersionAndCipherSuitesForSSLServerSocket() throws Exception {
          +
          + Configuration serverConfig = new Configuration();
          + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true);
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1.1");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_ALGORITHMS, "TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_128_CBC_SHA256");
          +
          + int port = new Random().nextInt(65535);
          + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig);
          + ServerSocket socket = null;
          + try

          { + socket = serverContext.getServerSocketFactory().createServerSocket(port); + + String[] protocols = ((SSLServerSocket) socket).getEnabledProtocols(); + String[] algorithms = ((SSLServerSocket) socket).getEnabledCipherSuites(); + + Assert.assertNotEquals(protocols.length, 1); + Assert.assertNotEquals(algorithms.length, 2); + + SSLUtils.setSSLVerAndCipherSuites(socket, serverConfig); + protocols = ((SSLServerSocket) socket).getEnabledProtocols(); + algorithms = ((SSLServerSocket) socket).getEnabledCipherSuites(); + + Assert.assertEquals(protocols.length, 1); + Assert.assertEquals(protocols[0], "TLSv1.1"); + Assert.assertEquals(algorithms.length, 2); + Assert.assertTrue(algorithms[0].equals("TLS_RSA_WITH_AES_128_CBC_SHA") || algorithms[0].equals("TLS_RSA_WITH_AES_128_CBC_SHA256")); + Assert.assertTrue(algorithms[1].equals("TLS_RSA_WITH_AES_128_CBC_SHA") || algorithms[1].equals("TLS_RSA_WITH_AES_128_CBC_SHA256")); + }

          finally {
          + if (socket != null)

          { + socket.close(); + }

          + }
          + }
          +
          + /**
          + * Tests if SSLUtils set the right ssl version and cipher suites for SSLEngine
          + */
          + @Test
          + public void testSetSSLVersionAndCipherSuitesForSSLEngine() throws Exception {
          +
          + Configuration serverConfig = new Configuration();
          + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true);
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1");
          + serverConfig.setString(ConfigConstants.SECURITY_SSL_ALGORITHMS, "TLS_DHE_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_RSA_WITH_AES_128_CBC_SHA256");
          +
          + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig);
          + SSLEngine engine = serverContext.createSSLEngine();
          +
          + String[] protocols = engine.getEnabledProtocols();
          + String[] algorithms = engine.getEnabledCipherSuites();
          +
          + Assert.assertNotEquals(protocols.length, 1);
          + Assert.assertNotEquals(algorithms.length, 2);
          +
          + SSLUtils.setSSLVerAndCipherSuites(engine, serverConfig);
          + protocols = engine.getEnabledProtocols();
          + algorithms = engine.getEnabledCipherSuites();
          +
          + Assert.assertEquals(protocols.length, 1);
          — End diff –

          `assertEquals` takes the parameters the other way around: (expected, actual) rather than (actual, expected).

          Will fix that on the fly while merging...

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3486#discussion_r106140551 — Diff: flink-runtime/src/test/java/org/apache/flink/runtime/net/SSLUtilsTest.java — @@ -125,4 +129,101 @@ public void testCreateSSLServerContextMisconfiguration() { } } + /** + * Tests if SSL Server Context creation fails with bad SSL configuration + */ + @Test + public void testCreateSSLServerContextWithMultiProtocols() { + + Configuration serverConfig = new Configuration(); + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1,TLSv1.2"); + + try { + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig); + Assert.fail("SSL server context created even with multiple protocols set "); + } catch (Exception e) { + // Exception here is valid + } + } + + /** + * Tests if SSLUtils set the right ssl version and cipher suites for SSLServerSocket + */ + @Test + public void testSetSSLVersionAndCipherSuitesForSSLServerSocket() throws Exception { + + Configuration serverConfig = new Configuration(); + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1.1"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_ALGORITHMS, "TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_128_CBC_SHA256"); + + int port = new Random().nextInt(65535); + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig); + ServerSocket socket = null; + try { + socket = serverContext.getServerSocketFactory().createServerSocket(port); + + String[] protocols = ((SSLServerSocket) socket).getEnabledProtocols(); + String[] algorithms = ((SSLServerSocket) socket).getEnabledCipherSuites(); + + Assert.assertNotEquals(protocols.length, 1); + Assert.assertNotEquals(algorithms.length, 2); + + SSLUtils.setSSLVerAndCipherSuites(socket, serverConfig); + protocols = ((SSLServerSocket) socket).getEnabledProtocols(); + algorithms = ((SSLServerSocket) socket).getEnabledCipherSuites(); + + Assert.assertEquals(protocols.length, 1); + Assert.assertEquals(protocols[0], "TLSv1.1"); + Assert.assertEquals(algorithms.length, 2); + Assert.assertTrue(algorithms[0].equals("TLS_RSA_WITH_AES_128_CBC_SHA") || algorithms[0].equals("TLS_RSA_WITH_AES_128_CBC_SHA256")); + Assert.assertTrue(algorithms[1].equals("TLS_RSA_WITH_AES_128_CBC_SHA") || algorithms[1].equals("TLS_RSA_WITH_AES_128_CBC_SHA256")); + } finally { + if (socket != null) { + socket.close(); + } + } + } + + /** + * Tests if SSLUtils set the right ssl version and cipher suites for SSLEngine + */ + @Test + public void testSetSSLVersionAndCipherSuitesForSSLEngine() throws Exception { + + Configuration serverConfig = new Configuration(); + serverConfig.setBoolean(ConfigConstants.SECURITY_SSL_ENABLED, true); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE, "src/test/resources/local127.keystore"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEYSTORE_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_KEY_PASSWORD, "password"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_PROTOCOL, "TLSv1"); + serverConfig.setString(ConfigConstants.SECURITY_SSL_ALGORITHMS, "TLS_DHE_RSA_WITH_AES_128_CBC_SHA,TLS_DHE_RSA_WITH_AES_128_CBC_SHA256"); + + SSLContext serverContext = SSLUtils.createSSLServerContext(serverConfig); + SSLEngine engine = serverContext.createSSLEngine(); + + String[] protocols = engine.getEnabledProtocols(); + String[] algorithms = engine.getEnabledCipherSuites(); + + Assert.assertNotEquals(protocols.length, 1); + Assert.assertNotEquals(algorithms.length, 2); + + SSLUtils.setSSLVerAndCipherSuites(engine, serverConfig); + protocols = engine.getEnabledProtocols(); + algorithms = engine.getEnabledCipherSuites(); + + Assert.assertEquals(protocols.length, 1); — End diff – `assertEquals` takes the parameters the other way around: (expected, actual) rather than (actual, expected). Will fix that on the fly while merging...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3486

          Thanks, looks good modulo a mini issue that I can fix while merging...

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3486 Thanks, looks good modulo a mini issue that I can fix while merging...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user WangTaoTheTonic commented on the issue:

          https://github.com/apache/flink/pull/3486

          Since not merged, I've turned them around. Sorry for the carelessness

          Show
          githubbot ASF GitHub Bot added a comment - Github user WangTaoTheTonic commented on the issue: https://github.com/apache/flink/pull/3486 Since not merged, I've turned them around. Sorry for the carelessness
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3486

          I have seen a failure on Travis with a "permission denied" exception: https://s3.amazonaws.com/archive.travis-ci.org/jobs/211674112/log.txt

          Not sure why that only occurred in one build. Do you have any more insight into this?

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3486 I have seen a failure on Travis with a "permission denied" exception: https://s3.amazonaws.com/archive.travis-ci.org/jobs/211674112/log.txt Not sure why that only occurred in one build. Do you have any more insight into this?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user StephanEwen commented on the issue:

          https://github.com/apache/flink/pull/3486

          I think the problem is that the "random port" was below 1024, which requires admin rights.
          Can you modify the test to use port "0"? That should auto-select a free permitted port and always work.

          Show
          githubbot ASF GitHub Bot added a comment - Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3486 I think the problem is that the "random port" was below 1024, which requires admin rights. Can you modify the test to use port "0"? That should auto-select a free permitted port and always work.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user WangTaoTheTonic commented on the issue:

          https://github.com/apache/flink/pull/3486

          Fixed

          Show
          githubbot ASF GitHub Bot added a comment - Github user WangTaoTheTonic commented on the issue: https://github.com/apache/flink/pull/3486 Fixed
          Hide
          StephanEwen Stephan Ewen added a comment -

          Fixed via e0614f6551a232706b74963563694486fe2461b1

          Show
          StephanEwen Stephan Ewen added a comment - Fixed via e0614f6551a232706b74963563694486fe2461b1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/flink/pull/3486

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/3486

            People

            • Assignee:
              WangTao Tao Wang
              Reporter:
              WangTao Tao Wang
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development