giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [giraph] dlogothetis commented on a change in pull request #150: GIRAPH-1251: Add SSLHandler for all Netty Communication (Initial diff)
Date Mon, 26 Apr 2021 07:01:29 GMT

dlogothetis commented on a change in pull request #150:
URL: https://github.com/apache/giraph/pull/150#discussion_r619748156



##########
File path: giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java
##########
@@ -1335,5 +1346,17 @@
   BooleanConfOption FAIL_ON_EMPTY_INPUT = new BooleanConfOption(
       "giraph.failOnEmptyInput", true,
       "Whether to fail the job or just warn when input is empty");
+
+  /** 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");

Review comment:
       Can you describe in the documentation what these are?

##########
File path: giraph-core/src/main/java/org/apache/giraph/comm/netty/NettySSLHandler.java
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.giraph.comm.netty;
+
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.channel.Channel;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslHandler;
+import io.netty.util.concurrent.Future;
+import org.apache.giraph.conf.ImmutableClassesGiraphConfiguration;
+import org.apache.log4j.Logger;
+
+import javax.net.ssl.SSLException;
+
+/**
+ * Utility class for all SSL related functions

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

##########
File path: giraph-core/src/main/java/org/apache/giraph/comm/netty/SSLConfig.java
##########
@@ -0,0 +1,326 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.giraph.comm.netty;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+
+import io.netty.handler.ssl.ClientAuth;
+import org.apache.giraph.conf.StrConfOption;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
+import org.apache.giraph.conf.ImmutableClassesGiraphConfiguration;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+import javax.net.ssl.SSLException;
+
+/**
+ * Helper class to build client and server SSL Context.
+ */
+class SslConfig
+{
+  /** Certificate Authority path for SSL config */
+  public static final StrConfOption CA_PATH =
+      new StrConfOption("giraph.sslConfig.caPath", null,
+        "Certificate Authority path for SSL config");
+
+  /** Client certificate path for SSL config */
+  public static final StrConfOption CLIENT_PATH =
+      new StrConfOption(
+        "giraph.sslConfig.clientPath", null,
+        "Client certificate path for SSL config");
+
+  /** Server certificate path for SSL config */
+  public static final StrConfOption SERVER_PATH =
+      new StrConfOption("giraph.sslConfig.serverPath", null,
+        "Server certificate path for SSL config");
+
+  /**
+   * Enum for the verification mode during SS authentication
+   */
+  public enum VerifyMode
+  {
+    /**
+     * For server side - request a client certificate and verify the
+     * certificate if it is sent.  Does not fail if the client does not
+     * present a certificate.
+     * For client side - validates the server certificate or fails.
+     */
+    VERIFY,
+    /**
+     * For server side - same as VERIFY but will fail if no certificate
+     * is sent.
+     * For client side - same as VERIFY.
+     */
+    VERIFY_REQ_CLIENT_CERT,
+    /**
+     * Request Client certificate
+     */
+    REQ_CLIENT_CERT,
+    /** No verification is done for both server and client side */
+    NO_VERIFY
+  }
+
+  /** Class logger */
+  private static final Logger LOG = Logger.getLogger(SslConfig.class);
+
+  /** Client or server creating the config */
+  private boolean client;
+  /** Verification mode as per the enum defined above */
+  private VerifyMode verifyMode;
+  /** Certificate authority File */
+  private File caFile;
+  /** Certificate File */
+  private File certFile;
+  /** Key File */
+  private File keyFile;
+
+  /**
+   * Constructor to set the file paths based on verification mode
+   *
+   * @param client client or server
+   * @param verifyMode verify mode as described in the enum above
+   * @param  caPath certificate authority file path
+   * @param certPath certificate file path
+   * @param keyPath key file path
+   */
+  public SslConfig(
+      boolean client, VerifyMode verifyMode, String caPath,

Review comment:
       nit: name `isClient`? 

##########
File path: giraph-core/src/main/java/org/apache/giraph/comm/netty/NettySSLHandler.java
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.giraph.comm.netty;
+
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.channel.Channel;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslHandler;
+import io.netty.util.concurrent.Future;
+import org.apache.giraph.conf.ImmutableClassesGiraphConfiguration;
+import org.apache.log4j.Logger;
+
+import javax.net.ssl.SSLException;
+
+/**

Review comment:
       Can you add some description about how the handler is used, e.g. when it's created,
when the methods are supposed to be called etc.

##########
File path: giraph-core/src/main/java/org/apache/giraph/comm/netty/SSLConfigReader.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.giraph.comm.netty;
+
+import org.apache.giraph.conf.ImmutableClassesGiraphConfigurable;
+
+/**
+ * An SSLConfigReader is used to handle ways in which the
+ * SSL Configs can be read. The configs include
+ * Certificate Authority file path, client and server identity file paths.
+ */
+public interface SSLConfigReader
+    extends ImmutableClassesGiraphConfigurable
+{
+  /**
+   * Read certificate authority Path from Env variable

Review comment:
       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?

##########
File path: giraph-core/src/main/java/org/apache/giraph/comm/netty/NettySSLHandler.java
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.giraph.comm.netty;
+
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.channel.Channel;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslHandler;
+import io.netty.util.concurrent.Future;
+import org.apache.giraph.conf.ImmutableClassesGiraphConfiguration;
+import org.apache.log4j.Logger;
+
+import javax.net.ssl.SSLException;
+
+/**
+ * Utility class for all SSL related functions
+ */
+public class NettySSLHandler
+{
+  /** Class Logger */
+  private static final Logger LOG = Logger.getLogger(NettySSLHandler.class);
+  /** Client or Server */
+  private boolean client;
+  /** Giraph Configuration */
+  private ImmutableClassesGiraphConfiguration conf;
+  /** SSL Event Handler interface */
+  private SSLEventHandler sslEventHandler;
+
+  /**
+   * Constructor
+   *
+   * @param client client/server for which the ssl handler needs to be created
+   * @param conf configuration object
+   */
+  public NettySSLHandler(
+    boolean client, ImmutableClassesGiraphConfiguration conf) {
+    this.client = client;
+    this.conf = conf;
+    sslEventHandler = conf.createSSLEventHandler();
+  }
+
+  /**
+   * Build the client or server SSL Context, create new SSL handler,
+   * add a listener function to onSslHandshakeComplete and return
+   *
+   * @param allocator ByteBufAllocator of the channel
+   *
+   * @throws SSLException
+   *
+   * @return The SSL Handler object
+   */
+  public SslHandler getSslHandler(ByteBufAllocator allocator)
+      throws SSLException
+  {
+    SslContext sslContext = new SslConfig.Builder(this.client, this.conf)
+        .verifyMode(SslConfig.VerifyMode.VERIFY_REQ_CLIENT_CERT)
+        .build()
+        .buildSslContext();
+    SslHandler handler = sslContext.newHandler(allocator);
+    handler.handshakeFuture().addListener(
+        f -> onSslHandshakeComplete(f, handler));
+    return handler;
+  }
+
+  /**
+   * Build the client or server SSL Context, create new SSL handler,
+   * add a listener function to onSslHandshakeComplete and return
+   *
+   * @param future Future object to be notified once handshake completes
+   * @param sslHandler SslHandler object
+   *
+   * @throws Exception
+   */
+  private void onSslHandshakeComplete(
+      Future<? super Channel> future,
+        SslHandler sslHandler) throws Exception
+  {
+    if (!future.isSuccess()) {
+      throw new SSLException("SSL Handshake failure", future.cause());
+    }
+    if (sslEventHandler != null) {

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

##########
File path: giraph-core/src/main/java/org/apache/giraph/comm/netty/SSLEventHandler.java
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.giraph.comm.netty;
+
+import io.netty.handler.ssl.SslHandler;
+import org.apache.giraph.conf.ImmutableClassesGiraphConfigurable;
+
+/**
+ * An SSLEventHandler is used to handle events during the SSL
+ * handshake process and override the default behavior.
+ */
+public interface SSLEventHandler
+  extends ImmutableClassesGiraphConfigurable
+{
+  /**
+   * Handle the event 'onSslHandshakeComplete' after TLS handshake
+   *
+   * @param client whether it is the client or server involved
+   * @param sslHandler the SslHandler
+   */
+  void handleOnSslHandshakeComplete(boolean client,

Review comment:
       nit: name the boolean `isClient`?

##########
File path: giraph-core/src/main/java/org/apache/giraph/comm/netty/SSLConfig.java
##########
@@ -0,0 +1,326 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.giraph.comm.netty;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
+
+import io.netty.handler.ssl.ClientAuth;
+import org.apache.giraph.conf.StrConfOption;
+import io.netty.handler.ssl.SslContext;
+import io.netty.handler.ssl.SslContextBuilder;
+import io.netty.handler.ssl.util.InsecureTrustManagerFactory;
+import org.apache.giraph.conf.ImmutableClassesGiraphConfiguration;
+import org.apache.log4j.Logger;
+
+import java.io.File;
+
+import javax.net.ssl.SSLException;
+
+/**
+ * Helper class to build client and server SSL Context.
+ */
+class SslConfig
+{
+  /** Certificate Authority path for SSL config */
+  public static final StrConfOption CA_PATH =
+      new StrConfOption("giraph.sslConfig.caPath", null,
+        "Certificate Authority path for SSL config");
+
+  /** Client certificate path for SSL config */
+  public static final StrConfOption CLIENT_PATH =
+      new StrConfOption(
+        "giraph.sslConfig.clientPath", null,
+        "Client certificate path for SSL config");
+
+  /** Server certificate path for SSL config */
+  public static final StrConfOption SERVER_PATH =
+      new StrConfOption("giraph.sslConfig.serverPath", null,
+        "Server certificate path for SSL config");
+
+  /**
+   * Enum for the verification mode during SS authentication
+   */
+  public enum VerifyMode
+  {
+    /**
+     * For server side - request a client certificate and verify the
+     * certificate if it is sent.  Does not fail if the client does not
+     * present a certificate.
+     * For client side - validates the server certificate or fails.
+     */
+    VERIFY,
+    /**
+     * For server side - same as VERIFY but will fail if no certificate
+     * is sent.
+     * For client side - same as VERIFY.
+     */
+    VERIFY_REQ_CLIENT_CERT,
+    /**
+     * Request Client certificate
+     */
+    REQ_CLIENT_CERT,
+    /** No verification is done for both server and client side */
+    NO_VERIFY
+  }
+
+  /** Class logger */
+  private static final Logger LOG = Logger.getLogger(SslConfig.class);
+
+  /** Client or server creating the config */
+  private boolean client;
+  /** Verification mode as per the enum defined above */
+  private VerifyMode verifyMode;
+  /** Certificate authority File */
+  private File caFile;
+  /** Certificate File */
+  private File certFile;
+  /** Key File */
+  private File keyFile;
+
+  /**
+   * Constructor to set the file paths based on verification mode
+   *
+   * @param client client or server
+   * @param verifyMode verify mode as described in the enum above
+   * @param  caPath certificate authority file path
+   * @param certPath certificate file path
+   * @param keyPath key file path
+   */
+  public SslConfig(
+      boolean client, VerifyMode verifyMode, String caPath,
+      String certPath, String keyPath)
+  {
+    this.client = client;
+    this.verifyMode = verifyMode;
+    try {
+      if (verifyMode != VerifyMode.NO_VERIFY) {
+        checkNotNull(caPath, "CA file path should not be null");
+        caFile = new File(caPath);
+        checkArgument(caFile.exists(), "CA file %s doesn't exist", caPath);
+      }
+      if (!client || verifyMode == VerifyMode.VERIFY_REQ_CLIENT_CERT ||
+          verifyMode == VerifyMode.REQ_CLIENT_CERT) {
+        checkNotNull(certPath, "certificate file path should not be null");
+        certFile = new File(certPath);
+        checkArgument(certFile.exists(),
+          "cert file %s doesn't exist", certPath);
+
+        checkNotNull(keyPath, "key path should not be null");
+        keyFile = new File(keyPath);
+        checkArgument(keyFile.exists(), "key file %s doesn't exist", keyPath);
+      }
+    } catch (NullPointerException | IllegalArgumentException e) {
+      LOG.error("Failed to load required SSL files. Exception: " +
+        e.getMessage());
+      LOG.error("Failure happened when using SslConfig: isClient = " +
+        String.valueOf(client) + " with verifyMode = " +
+        String.valueOf(verifyMode) + " and paths ca:" + caPath + ", cert:" +
+        certPath + ", key:" + keyPath, e);
+      throw e;
+    }
+  }
+
+  /**
+   * Wrapper Function to build client or server SSL context
+   *
+   * @throws SSLException
+   * @return Built SslContext
+   */
+  SslContext buildSslContext()
+      throws SSLException
+  {
+    if (client) {
+      return buildClientSslContext();
+    } else {
+      return buildServerSslContext();
+    }
+  }
+
+  /**
+   * Function to build client SSL context
+   *
+   * @throws SSLException
+   * @return Built SslContext
+   */
+  private SslContext buildClientSslContext()
+      throws SSLException
+  {
+    SslContextBuilder builder = SslContextBuilder.forClient();
+    if (verifyMode == VerifyMode.NO_VERIFY) {
+      builder.trustManager(InsecureTrustManagerFactory.INSTANCE);
+    } else {
+      builder.trustManager(caFile);
+    }
+    if (verifyMode == VerifyMode.VERIFY_REQ_CLIENT_CERT) {
+      builder.keyManager(certFile, keyFile);
+    }
+    return builder.build();
+  }
+
+  /**
+   * Function to build Server SSL context
+   *
+   * @throws SSLException
+   * @return Built SslContext
+   */
+  private SslContext buildServerSslContext()
+      throws SSLException
+  {
+    SslContextBuilder builder = SslContextBuilder.forServer(certFile, keyFile);
+    if (verifyMode == VerifyMode.NO_VERIFY) {
+      builder.trustManager(InsecureTrustManagerFactory.INSTANCE);
+    } else {
+      builder.trustManager(caFile);
+    }
+    if (verifyMode == VerifyMode.VERIFY) {
+      builder.clientAuth(ClientAuth.OPTIONAL);
+    } else if (verifyMode == VerifyMode.VERIFY_REQ_CLIENT_CERT) {
+      builder.clientAuth(ClientAuth.REQUIRE);
+    }
+    return builder.build();
+  }
+
+  /**
+   * Static Builder class and wrapper around SSLConfig
+   * Checks and assigns the certificate and key paths - first
+   * from a SSLConfigReader and then from the provided files.
+   */
+  public static class Builder
+  {
+    /** Client or server creating the config */
+    private boolean client;
+    /** Verification mode as per the enum defined above */
+    private VerifyMode verifyMode;
+    /** Certificate authority File */
+    private String caPath;
+    /** Key File */
+    private String keyPath;
+    /** Certificate File */
+    private String certPath;
+    /** Giraph configuration */
+    private ImmutableClassesGiraphConfiguration conf;
+    /** SSLConfigReader interface */
+    private SSLConfigReader sslConfigReader;
+
+    /**
+     * Constructor
+     *
+     * @param client client or server
+     * @param conf Giraph configuration
+     */
+    public Builder(boolean client, ImmutableClassesGiraphConfiguration conf)
+    {
+      verifyMode = VerifyMode.VERIFY;
+      this.client = client;
+      this.conf = conf;
+      sslConfigReader = conf.createSSLConfigReader();
+      assignCAPath();
+      assignCertAndKeyPath();
+    }
+
+    /**
+     * Assign certificate authority path from
+     * sslConfigReader or default ca path provided
+     */
+    private void assignCAPath()
+    {
+      this.caPath = CA_PATH.get(conf);
+
+      if (sslConfigReader != null) {
+        String envVarTlsCAPath = sslConfigReader.readCAPathFromEnv();
+        if (envVarTlsCAPath != null) {
+          this.caPath = envVarTlsCAPath;
+        }
+      }

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




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message