You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by StephanEwen <gi...@git.apache.org> on 2018/05/07 20:21:48 UTC
[GitHub] flink pull request #5966: [FLINK-9312] [security] Add mutual authentication ...
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/5966
[FLINK-9312] [security] Add mutual authentication for RPC and data plane
## What is the purpose of the change
Currently, the Flink processes encrypted connections via SSL:
- Data exchange TM - TM
- RPC JM - TM
- Blob Service JM - TM
- (Optionally to ZooKeeper and connectors, this is connector specific and not in scope of this change)
However, the server side always accepts any client to build up the connection, meaning the connections are not strongly authenticated. Activating SSL mutual authentication strengthens this significantly - only processes that have access to the same certificate can connect.
## Brief change log
- Activate mutual auth in akka (via akka config)
- Activate mutual auth in Netty for data shuffles via `SSLContext` and `SSLEngine` parameters
## Verifying this change
- Adds a test to the `NettyClientServerSslTest`
## 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/StephanEwen/incubator-flink mutual_auth
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5966.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 #5966
----
commit 8bceb03d5653c94247b72d6256f4e9e37b036e35
Author: Stephan Ewen <se...@...>
Date: 2018-05-07T17:44:33Z
[FLINK-9313] [security] Activate mutual authentication for RPC/akka
commit 59b017580d30904418e0867ac122a8183dc5db70
Author: Stephan Ewen <se...@...>
Date: 2018-05-07T19:28:41Z
[FLINK-9314] [security] Add mutual authentication for Netty / TaskManager's data plane
----
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on the issue:
https://github.com/apache/flink/pull/5966
I will take a look at this later today.
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by EronWright <gi...@git.apache.org>.
Github user EronWright commented on the issue:
https://github.com/apache/flink/pull/5966
@StephanEwen I would like to work on this issue, building on your PR, would that be OK?
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5966
I agree, we need different key/truststores for the internal/external connectivity. This PR was meant as a step in that direction, separating at least within the SSL Utils the internal and external context setup.
In your thinking, is there ever a case for a different internal authentication method than "single trusted certificate"? What if were not tied to akka? (Side note: I think for internal communication, 'authentication is authorization' is probably reasonable, because the are no different users/roles for internal communication).
Would you assume that internally, we never do hostname verification?
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by EronWright <gi...@git.apache.org>.
Github user EronWright commented on the issue:
https://github.com/apache/flink/pull/5966
To be honest I don't see a great need to support anything other than a single trusted certificate for internal communication. We could easily build some conveniences around that, like generating a certificate upon cluster startup with an truststore containing only that certificate. I see no need to perform hostname verification because the truststore is constrained.
One drawback is that the queryable state interface is practically inaccessible because the cluster certificate is likely unknown to the client.
Regarding external connectivity, I don't think that mutual SSL is universally applicable, and I explore some options in FLIP-26.
---
[GitHub] flink pull request #5966: [FLINK-9312] [security] Add mutual authentication ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/5966#discussion_r186800971
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/netty/NettyServer.java ---
@@ -170,8 +171,8 @@ public void initChannel(SocketChannel channel) throws Exception {
localAddress = (InetSocketAddress) bindFuture.channel().localAddress();
- long end = System.currentTimeMillis();
- LOG.info("Successful initialization (took {} ms). Listening on SocketAddress {}.", (end - start), bindFuture.channel().localAddress().toString());
+ final long duration = (start - System.nanoTime()) / 1_000_000;
+ LOG.info("Successful initialization (took {} ms). Listening on SocketAddress {}.", duration, localAddress);
--- End diff --
You are absolutely right, thanks for catching this!
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5966
@EronWright Just saw this - I have concurrently reworked this PR into #6326 which does things more cleanly. I would like to get that PR in for 1.6 (got many users asking for this).
I would be happy if you want to build on top of that for the next steps...
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5966
@EronWright This might be interesting to you as well
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5966
This PR is subsumed by #6326
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5966
I would move ahead with this PR as follows:
- Separate internal and external SSL config
- Activate SSL client auth for akka, netty, and blob server (pure internal communication)
Let's discuss external connectivity on FLIP-26
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by GJL <gi...@git.apache.org>.
Github user GJL commented on the issue:
https://github.com/apache/flink/pull/5966
This is relevant http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/Discuss-FLIP-26-SSL-Mutual-Authentication-td22188.html
---
[GitHub] flink issue #5966: [FLINK-9312] [security] Add mutual authentication for RPC...
Posted by EronWright <gi...@git.apache.org>.
Github user EronWright commented on the issue:
https://github.com/apache/flink/pull/5966
This looks great but let's please separate the intra-cluster SSL configuration options from the external-cluster options. In particular, the web/API endpoints should be governed by a different keystore and truststore than are the internal endpoints. Likewise, the "hostname verification" option should pertain only to external connectivity.
My rationale is that:
1. the truststore to be used for internal connectivity must be highly restrictive; it should never be the system truststore. Meanwhile, the truststore for external connectivity (e.g. from the Flink client) should typically be the system truststore.
2. The certificate to be used for internal connectivity may simply be a generated certificate. Meanwhile, the certificate for external connectivity should be obtained from a cluster CA. For example, in K8s one might expect a cert obtained from the cluster CA and with a CN corresponding to a Service resource (`flink.default.svc.cluster.local`).
The whole issue of using a restrictive truststore for internal connectivity has been discussed on various Akka forums, for example [this PR](https://github.com/akka/akka/pull/23568#issuecomment-331919364). Keep in mind that Akka has no authorization layer; any authenticated client is considered authorized. Therefore, a liberal truststore (such as the system truststore) would present an extremely low barrier of entry since public SSL certs are easy to get.
---
[GitHub] flink pull request #5966: [FLINK-9312] [security] Add mutual authentication ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen closed the pull request at:
https://github.com/apache/flink/pull/5966
---
[GitHub] flink pull request #5966: [FLINK-9312] [security] Add mutual authentication ...
Posted by sihuazhou <gi...@git.apache.org>.
Github user sihuazhou commented on a diff in the pull request:
https://github.com/apache/flink/pull/5966#discussion_r186769177
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/netty/NettyServer.java ---
@@ -170,8 +171,8 @@ public void initChannel(SocketChannel channel) throws Exception {
localAddress = (InetSocketAddress) bindFuture.channel().localAddress();
- long end = System.currentTimeMillis();
- LOG.info("Successful initialization (took {} ms). Listening on SocketAddress {}.", (end - start), bindFuture.channel().localAddress().toString());
+ final long duration = (start - System.nanoTime()) / 1_000_000;
+ LOG.info("Successful initialization (took {} ms). Listening on SocketAddress {}.", duration, localAddress);
--- End diff --
Is this should be `final long duration = (System.nanoTime() - start) / 1_000_000;`
---