You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GJL <gi...@git.apache.org> on 2018/05/09 11:47:45 UTC
[GitHub] flink pull request #5973: [FLINK-9261][ssl] Fix SSL support for REST API and...
GitHub user GJL opened a pull request:
https://github.com/apache/flink/pull/5973
[FLINK-9261][ssl] Fix SSL support for REST API and Web UI.
## What is the purpose of the change
*Fix SSL support for REST API and Web UI.*
cc: @tillrohrmann @StephanEwen @zentol
## Brief change log
- *Remove wrong reuse of SSLEngine instances. SSLEngine must be re-created for
every SocketChannel initialization.*
- *Add ChunkedWriteHandler to REST server pipeline because StaticFileServerHandler
relies on it.*
- *Add integration tests to verify that SSL can be enabled.*
## Verifying this change
This change added tests and can be verified as follows:
- *Added integration tests for check if the RestServerEndpoint works with SSL enabled.*
- *Manually verified the change by submitting a job to a cluster with SSL enabled via the CLI, and by accessing the Web UI.*
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (**yes** / no / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (**not applicable** / docs / JavaDocs / not documented)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/GJL/flink FLINK-9261
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5973.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 #5973
----
commit a74fee4853e8b97427e9607b8ed6bdaeedae0c12
Author: gyao <ga...@...>
Date: 2018-05-09T11:30:23Z
[FLINK-9261][ssl,flip6] Fix SSL support for REST API and Web UI.
- Remove wrong reuse of SSLEngine instances. SSLEngine must be re-created for
every SocketChannel initialization.
- Add ChunkedWriteHandler to REST server pipeline because StaticFileServerHandler
relies on it.
- Add integration tests to verify that SSL can be enabled.
commit 710c7e222ff7c13b48ecb4a1549571afae60312c
Author: gyao <ga...@...>
Date: 2018-05-09T11:41:29Z
[hotfix][ssl,docs] Use markdown hyperlink instead of writing out the URL.
----
---
[GitHub] flink pull request #5973: [FLINK-9261][ssl] Fix SSL support for REST API and...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/5973#discussion_r187025268
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/net/SSLUtils.java ---
@@ -81,16 +85,62 @@ public static void setSSLVerAndCipherSuites(ServerSocket socket, Configuration c
}
}
+ /**
+ * Creates a {@link SSLEngineFactory} to be used by the Server.
+ *
+ * @param config The application configuration.
+ */
+ public static SSLEngineFactory createServerSSLEngineFactory(final Configuration config) throws Exception {
+ return createSSLEngineFactory(config, false);
+ }
+
+ /**
+ * Creates a {@link SSLEngineFactory} to be used by the Client.
+ * @param config The application configuration.
+ */
+ public static SSLEngineFactory createClientSSLEngineFactory(final Configuration config) throws Exception {
+ return createSSLEngineFactory(config, true);
+ }
+
+ private static SSLEngineFactory createSSLEngineFactory(
+ final Configuration config,
+ final boolean clientMode) throws Exception {
+
+ final SSLContext sslContext = clientMode ?
+ createSSLClientContext(config) :
+ createSSLServerContext(config);
+
+ checkState(sslContext != null, "%s it not enabled", SecurityOptions.SSL_ENABLED.key());
+
+ return new SSLEngineFactory(
+ sslContext,
+ getEnabledProtocols(config),
+ getEnabledCipherSuites(config),
+ clientMode);
+ }
+
/**
* Sets SSL version and cipher suites for SSLEngine.
- * @param engine
- * SSLEngine to be handled
- * @param config
- * The application configuration
+ *
+ * @param engine SSLEngine to be handled
+ * @param config The application configuration
+ * @deprecated Use {@link #createClientSSLEngineFactory(Configuration)} or
+ * {@link #createServerSSLEngineFactory(Configuration)}.
*/
+ @Deprecated
public static void setSSLVerAndCipherSuites(SSLEngine engine, Configuration config) {
- engine.setEnabledProtocols(config.getString(SecurityOptions.SSL_PROTOCOL).split(","));
- engine.setEnabledCipherSuites(config.getString(SecurityOptions.SSL_ALGORITHMS).split(","));
+ engine.setEnabledProtocols(getEnabledProtocols(config));
+ engine.setEnabledCipherSuites(getEnabledCipherSuites(config));
+ }
+
+ private static String[] getEnabledProtocols(final Configuration config) {
+ requireNonNull(config, "config must not be null");
--- End diff --
For private internal utilities, I suggest to skip the null check in most places, especially when it will eagerly fail with an exception on null anyways.
In any case, if you believe the check should be there, please use `Preconditions.checkNotNull` rather than `requireNonNull`.
---
[GitHub] flink pull request #5973: [FLINK-9261][ssl] Fix SSL support for REST API and...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on a diff in the pull request:
https://github.com/apache/flink/pull/5973#discussion_r187194770
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/net/SSLUtils.java ---
@@ -81,16 +85,62 @@ public static void setSSLVerAndCipherSuites(ServerSocket socket, Configuration c
}
}
+ /**
+ * Creates a {@link SSLEngineFactory} to be used by the Server.
+ *
+ * @param config The application configuration.
+ */
+ public static SSLEngineFactory createServerSSLEngineFactory(final Configuration config) throws Exception {
+ return createSSLEngineFactory(config, false);
+ }
+
+ /**
+ * Creates a {@link SSLEngineFactory} to be used by the Client.
+ * @param config The application configuration.
+ */
+ public static SSLEngineFactory createClientSSLEngineFactory(final Configuration config) throws Exception {
+ return createSSLEngineFactory(config, true);
+ }
+
+ private static SSLEngineFactory createSSLEngineFactory(
+ final Configuration config,
+ final boolean clientMode) throws Exception {
+
+ final SSLContext sslContext = clientMode ?
+ createSSLClientContext(config) :
+ createSSLServerContext(config);
+
+ checkState(sslContext != null, "%s it not enabled", SecurityOptions.SSL_ENABLED.key());
+
+ return new SSLEngineFactory(
+ sslContext,
+ getEnabledProtocols(config),
+ getEnabledCipherSuites(config),
+ clientMode);
+ }
+
/**
* Sets SSL version and cipher suites for SSLEngine.
- * @param engine
- * SSLEngine to be handled
- * @param config
- * The application configuration
+ *
+ * @param engine SSLEngine to be handled
+ * @param config The application configuration
+ * @deprecated Use {@link #createClientSSLEngineFactory(Configuration)} or
+ * {@link #createServerSSLEngineFactory(Configuration)}.
*/
+ @Deprecated
public static void setSSLVerAndCipherSuites(SSLEngine engine, Configuration config) {
- engine.setEnabledProtocols(config.getString(SecurityOptions.SSL_PROTOCOL).split(","));
- engine.setEnabledCipherSuites(config.getString(SecurityOptions.SSL_ALGORITHMS).split(","));
+ engine.setEnabledProtocols(getEnabledProtocols(config));
+ engine.setEnabledCipherSuites(getEnabledCipherSuites(config));
+ }
+
+ private static String[] getEnabledProtocols(final Configuration config) {
+ requireNonNull(config, "config must not be null");
--- End diff --
Ok, I will use Flink's `checkNotNull` next time.
---
[GitHub] flink issue #5973: [FLINK-9261][ssl] Fix SSL support for REST API and Web UI...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5973
Merging this...
---
[GitHub] flink issue #5973: [FLINK-9261][ssl] Fix SSL support for REST API and Web UI...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5973
Merged in `master` in 3afa5eb3c47158086ab29012a835f96682a85d34
---
[GitHub] flink pull request #5973: [FLINK-9261][ssl] Fix SSL support for REST API and...
Posted by GJL <gi...@git.apache.org>.
Github user GJL closed the pull request at:
https://github.com/apache/flink/pull/5973
---