GIRAPH-1251: Add optional SSLHandler for all Netty Communication #150

atanu1991 · 2021-04-19T14:29:41Z

GIRAPH-1251: Add optional SSLHandler for all Netty Communication
With this change its possible to encrypt all communication via Netty
with SSL Handler. It also exposes function to add custom authn and
authz checks on tls handshake complete. Once this flag is set, all
Netty traffic will be ensured to be encrypted.

Testing:

mvn clean verify passes.
When the sslEncrypt flag is off there were no changes to the existing workflow.
On enabling the flag (sslEncrypt) and having a custom
SSL Event Handler, thhe performance was varied, on some test cases
there was an improvement in perf (+25%), while few others showed
~30+% degrade. Initial tests didnt give any strong indications
of severe performance degrade throghout.

dlogothetis · 2021-04-25T04:22:39Z

Can you add a high-level description of how the authentication mechanism works and also how this was tested?

dlogothetis · 2021-04-25T04:32:00Z

giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java

+  /** SSLConfigReader class - optional */
+  ClassConfOption<SSLConfigReader> SSL_CONFIG_READER_CLASS =
+          ClassConfOption.create("giraph.sslConfigReader",
+              null, SSLConfigReader.class,
+                  "SSLConfigReader class - optional");
+
+  /** SSLEventHandler class - optional */
+  ClassConfOption<SSLEventHandler> SSL_EVENT_HANDLER_CLASS =
+    ClassConfOption.create("giraph.sslEventHandler",
+      null, SSLEventHandler.class,
+      "SSLEventHandler class - optional");


Can you describe in the documentation what these are?

dlogothetis · 2021-04-25T04:37:21Z

giraph-core/src/main/java/org/apache/giraph/comm/netty/NettySSLHandler.java

+import javax.net.ssl.SSLException;
+
+/**
+ * Utility class for all SSL related functions


nit: this doesn't look like the typical utility class

giraph-core/src/main/java/org/apache/giraph/comm/netty/NettySSLHandler.java

dlogothetis · 2021-04-25T04:41:03Z

giraph-core/src/main/java/org/apache/giraph/comm/netty/SSLConfigReader.java

+    extends ImmutableClassesGiraphConfigurable
+{
+  /**
+   * Read certificate authority Path from Env variable


Regarding the naming of the methods, is the assumption that everything is is going to be read from environment variables? If not, maybe choose different naming?

dlogothetis · 2021-04-25T04:42:57Z

giraph-core/src/main/java/org/apache/giraph/comm/netty/SSLEventHandler.java

+   * @param client whether it is the client or server involved
+   * @param sslHandler the SslHandler
+   */
+  void handleOnSslHandshakeComplete(boolean client,


nit: name the boolean isClient?

dlogothetis · 2021-04-25T04:46:08Z

giraph-core/src/main/java/org/apache/giraph/comm/netty/SSLConfig.java

+   * @param keyPath key file path
+   */
+  public SslConfig(
+      boolean client, VerifyMode verifyMode, String caPath,


nit: name isClient?

dlogothetis · 2021-04-25T04:51:04Z

giraph-core/src/main/java/org/apache/giraph/comm/netty/SSLConfig.java

+      this.caPath = CA_PATH.get(conf);
+
+      if (sslConfigReader != null) {
+        String envVarTlsCAPath = sslConfigReader.readCAPathFromEnv();
+        if (envVarTlsCAPath != null) {
+          this.caPath = envVarTlsCAPath;
+        }
+      }


What is the reason for having both a Giraph option for the path and a Giraph option for the SslConfigReader class?

Removed this logic

dlogothetis · 2021-04-25T05:44:46Z

giraph-core/src/main/java/org/apache/giraph/comm/netty/NettySSLHandler.java

+    if (!future.isSuccess()) {
+      throw new SSLException("SSL Handshake failure", future.cause());
+    }
+    if (sslEventHandler != null) {


Why would we create a NettySSLHandler, if this is not defined?

Its not necessary to have sslEventHandler (onSslHandshakeComplete) event defined. Its required for facebook use case where we need to do custom authn and authz. If you want the default behavior then do nothing on onSslHandshakeComplete. So sslEventHandler is null by default

checkstyle.xml

atanu1991 · 2021-05-03T23:39:43Z

Can you add a high-level description of how the authentication mechanism works and also how this was tested?

In the summary you mean?

dlogothetis

Can you add a high-level description of how the authentication mechanism works and also how this was tested?

In the summary you mean?

Yes, please add in the summary a description and also a summary of how this was tested. Please do run mvn clean verify as well.

Also, can you comment here about the optimizations you have identified, for instance, you mentioned not creating the ssl config multiple times.

giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyClient.java

giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyServer.java

dlogothetis · 2021-05-14T02:12:16Z

Also, have we checked whether performance is impacted?

atanu1991 · 2021-05-14T09:27:38Z

Performance will be impacted quite a bit. The testing of that is something planned for next half. For now we have to block the migration,

dlogothetis · 2021-05-14T15:42:20Z

I agree, but I still think we should have a couple of data points. As an extreme, say this makes runtime 3x more. Would it be acceptable?

…

On Fri, May 14, 2021 at 2:27 AM Atanu Ghosh ***@***.***> wrote: Performance will be impacted quite a bit. The testing of that is something planned for next half. For now we have to block the migration, — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#150 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAPZYOER7NBJQ5YQRMOU4EDTNTUJRANCNFSM43F6K5DA> .

With this change its possible to encrypt all communication via Netty with SSL Handler. It also exposes function to add custom authn and authz checks on tls handshake complete. Once this flag is set, all Netty traffic will be ensured to be encrypted. Testing: - mvn clean verify passes. - When the sslEncrypt flag is off there were no changes to the existing workflow. - On enabling the flag (sslEncrypt) and having a custom SSL Event Handler, there was no performance hit. A standard set of tests were run with and without this flag in the same enviornment and the results showed -5% to 5% difference in total CPU time utilized by the jobs. This is within acceptable limits and is normal variance.

atanu1991 force-pushed the netty branch 4 times, most recently from 628570e to 55013ce Compare April 23, 2021 14:48

dlogothetis reviewed Apr 25, 2021

View reviewed changes

atanu1991 force-pushed the netty branch 4 times, most recently from f8a452d to 8d715bb Compare May 1, 2021 01:38

atanu1991 commented May 3, 2021

View reviewed changes

checkstyle.xml Show resolved Hide resolved

atanu1991 force-pushed the netty branch 2 times, most recently from 5073094 to 675c7f1 Compare May 7, 2021 21:50

atanu1991 force-pushed the netty branch from 675c7f1 to d9d5e8d Compare May 14, 2021 01:19

atanu1991 changed the title ~~GIRAPH-1251: Add SSLHandler for all Netty Communication (Initial diff)~~ GIRAPH-1251: Add optional SSLHandler for all Netty Communication May 14, 2021

dlogothetis reviewed May 14, 2021

View reviewed changes

giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyClient.java Show resolved Hide resolved

giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyServer.java Show resolved Hide resolved

atanu1991 force-pushed the netty branch 3 times, most recently from 7a35d43 to 3f196b2 Compare May 19, 2021 20:21

atanu1991 force-pushed the netty branch from 3f196b2 to df41e55 Compare June 3, 2021 01:07

dlogothetis approved these changes Jun 3, 2021

View reviewed changes

asfgit closed this in 2c63aa2 Jun 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GIRAPH-1251: Add optional SSLHandler for all Netty Communication #150

GIRAPH-1251: Add optional SSLHandler for all Netty Communication #150

atanu1991 commented Apr 19, 2021 •

edited

Loading

dlogothetis commented Apr 25, 2021

dlogothetis Apr 25, 2021

dlogothetis Apr 25, 2021

dlogothetis Apr 25, 2021

dlogothetis Apr 25, 2021

dlogothetis Apr 25, 2021

dlogothetis Apr 25, 2021

atanu1991 May 3, 2021

dlogothetis Apr 25, 2021

atanu1991 May 3, 2021

atanu1991 commented May 3, 2021

dlogothetis left a comment

dlogothetis commented May 14, 2021

atanu1991 commented May 14, 2021

dlogothetis commented May 14, 2021 via email

GIRAPH-1251: Add optional SSLHandler for all Netty Communication #150

GIRAPH-1251: Add optional SSLHandler for all Netty Communication #150

Conversation

atanu1991 commented Apr 19, 2021 • edited Loading

dlogothetis commented Apr 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atanu1991 commented May 3, 2021

dlogothetis left a comment

Choose a reason for hiding this comment

dlogothetis commented May 14, 2021

atanu1991 commented May 14, 2021

dlogothetis commented May 14, 2021 via email

atanu1991 commented Apr 19, 2021 •

edited

Loading