You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "mridulm (via GitHub)" <gi...@apache.org> on 2024/04/02 01:10:55 UTC
[PR] [CELEBORN-1351] Add support for SSLFactory, and enable TLS support [celeborn]
mridulm opened a new pull request, #2438:
URL: https://github.com/apache/celeborn/pull/2438
### What changes were proposed in this pull request?
Add SSLFactory, and wire up TLS support with rest of Celeborn to enable secure over the wire communication.
### Why are the changes needed?
Add support for TLS to secure wire communication.
### Does this PR introduce _any_ user-facing change?
Yes, completes basic support for TLS in Celeborn.
### How was this patch tested?
Existing tests, augmented with additional unit tests.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Add SSLFactory, and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1547111630
##########
common/src/main/java/org/apache/celeborn/common/network/TransportContext.java:
##########
@@ -135,31 +146,53 @@ public TransportServer createServer() {
return createServer(null, 0, Collections.emptyList());
}
+ public boolean sslEncryptionEnabled() {
+ return this.sslFactory != null;
+ }
+
public TransportChannelHandler initializePipeline(
- SocketChannel channel, ChannelInboundHandlerAdapter decoder) {
- return initializePipeline(channel, decoder, msgHandler);
+ SocketChannel channel, ChannelInboundHandlerAdapter decoder, boolean isClient) {
+ return initializePipeline(channel, decoder, msgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
- SocketChannel channel, BaseMessageHandler resolvedMsgHandler) {
- return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler);
+ SocketChannel channel, BaseMessageHandler resolvedMsgHandler, boolean isClient) {
+ return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
SocketChannel channel,
ChannelInboundHandlerAdapter decoder,
- BaseMessageHandler resolvedMsgHandler) {
+ BaseMessageHandler resolvedMsgHandler,
+ boolean isClient) {
try {
ChannelPipeline pipeline = channel.pipeline();
if (nettyLogger.getLoggingHandler() != null) {
pipeline.addLast("loggingHandler", nettyLogger.getLoggingHandler());
}
+ if (sslEncryptionEnabled()) {
+ if (!isClient && !sslFactory.hasKeyManagers()) {
+ throw new IllegalStateException("Not a client connection and no keys configured");
+ }
+
+ SslHandler sslHandler;
+ try {
+ sslHandler = new SslHandler(sslFactory.createSSLEngine(isClient, channel.alloc()));
+ } catch (Exception e) {
+ throw new IllegalStateException("Error creating Netty SslHandler", e);
+ }
+ pipeline.addFirst("NettySslEncryptionHandler", sslHandler);
+ // Cannot use zero-copy with HTTPS, so we add in our ChunkedWriteHandler just before the
+ // MessageEncoder
+ pipeline.addLast("chunkedWriter", new ChunkedWriteHandler());
+ }
+
if (channelsLimiter != null) {
pipeline.addLast("limiter", channelsLimiter);
}
Review Comment:
Rate limiter is something which does not exist in spark ... this looks fine to me, but please do take a look at this reviewers !
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
Review Comment:
Unlike in spark, where clients do not have ssl cert, here we allow for ssl client to present their certificate - when available.
This is mainly relevant when master and workers communicate, and not so much between applications and master/worker (not common for cert to be present for spark/flink/mr apps) - providing for master/worker to validate their identity.
See `SslConnectivitySuiteJ.testUntrustedClientCertFails` where this validation failure is tested.
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
+ engine.setEnabledProtocols(enabledProtocols(engine, requestedProtocol));
+ engine.setEnabledCipherSuites(enabledCipherSuites(engine, requestedCiphers));
+ return engine;
+ }
+
+ private SSLEngine createEngine(boolean isClient, ByteBufAllocator allocator) {
+ return jdkSslContext.createSSLEngine();
+ }
+
+ private static final X509Certificate[] EMPTY_CERT_ARRAY = new X509Certificate[0];
+
+ private static TrustManager[] credulousTrustStoreManagers() {
+ return new TrustManager[] {
+ new X509TrustManager() {
+ @Override
+ public void checkClientTrusted(X509Certificate[] x509Certificates, String s)
+ throws CertificateException {}
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
+ throws CertificateException {}
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return EMPTY_CERT_ARRAY;
Review Comment:
Note: This fixes a bug in spark, and I will backport it there.
##########
common/src/test/java/org/apache/celeborn/common/network/RpcIntegrationSuiteJ.java:
##########
@@ -101,6 +107,7 @@ public boolean checkRegistered() {
public static void tearDown() {
server.close();
clientFactory.close();
+ context.close();
testData.cleanup();
}
Review Comment:
The existing test suites, like `RpcIntegrationSuite`, `TransportClientFactorySuite`, etc are modified such that a SSL subclass can test all the cases - but over SSL.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2036227791
## [Codecov](https://app.codecov.io/gh/apache/celeborn/pull/2438?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `72.83237%` with `47 lines` in your changes are missing coverage. Please review.
> Project coverage is 40.12%. Comparing base [(`6153ba4`)](https://app.codecov.io/gh/apache/celeborn/commit/6153ba4c627de4bdeedcb4f1e4ae45f18ad2965b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`d80bacd`)](https://app.codecov.io/gh/apache/celeborn/pull/2438?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/celeborn/pull/2438?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...apache/celeborn/common/network/ssl/SSLFactory.java](https://app.codecov.io/gh/apache/celeborn/pull/2438?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fnetwork%2Fssl%2FSSLFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9zc2wvU1NMRmFjdG9yeS5qYXZh) | 73.28% | [17 Missing and 14 partials :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2438?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ache/celeborn/common/network/TransportContext.java](https://app.codecov.io/gh/apache/celeborn/pull/2438?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fnetwork%2FTransportContext.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9UcmFuc3BvcnRDb250ZXh0LmphdmE=) | 72.23% | [8 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2438?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../common/network/client/TransportClientFactory.java](https://app.codecov.io/gh/apache/celeborn/pull/2438?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fceleborn%2Fcommon%2Fnetwork%2Fclient%2FTransportClientFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9jZWxlYm9ybi9jb21tb24vbmV0d29yay9jbGllbnQvVHJhbnNwb3J0Q2xpZW50RmFjdG9yeS5qYXZh) | 77.78% | [3 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2438?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...apache/celeborn/common/rpc/netty/NettyRpcEnv.scala](https://app.codecov.io/gh/apache/celeborn/pull/2438?src=pr&el=tree&filepath=common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fceleborn%2Fcommon%2Frpc%2Fnetty%2FNettyRpcEnv.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvY2VsZWJvcm4vY29tbW9uL3JwYy9uZXR0eS9OZXR0eVJwY0Vudi5zY2FsYQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/celeborn/pull/2438?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2438 +/- ##
==========================================
+ Coverage 39.24% 40.12% +0.88%
==========================================
Files 217 218 +1
Lines 13432 13600 +168
Branches 1173 1195 +22
==========================================
+ Hits 5270 5455 +185
+ Misses 7863 7831 -32
- Partials 299 314 +15
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celeborn/pull/2438?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1554820276
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ return null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
Review Comment:
Thanks for catching this ! This method is not used anymore - removed it.
OpenSSL is actually planned, but for later (see CELEBORN-1367).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1555206104
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
+ engine.setEnabledProtocols(enabledProtocols(engine, requestedProtocol));
+ engine.setEnabledCipherSuites(enabledCipherSuites(engine, requestedCiphers));
+ return engine;
+ }
+
+ private SSLEngine createEngine(boolean isClient, ByteBufAllocator allocator) {
+ return jdkSslContext.createSSLEngine();
+ }
+
+ private static final X509Certificate[] EMPTY_CERT_ARRAY = new X509Certificate[0];
+
+ private static TrustManager[] credulousTrustStoreManagers() {
+ return new TrustManager[] {
+ new X509TrustManager() {
+ @Override
+ public void checkClientTrusted(X509Certificate[] x509Certificates, String s)
+ throws CertificateException {}
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
+ throws CertificateException {}
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return EMPTY_CERT_ARRAY;
Review Comment:
I assume this surfaced as part of the usage of `wantClientAuth`? What was the bug?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1548249729
##########
common/src/main/java/org/apache/celeborn/common/network/TransportContext.java:
##########
@@ -135,31 +146,53 @@ public TransportServer createServer() {
return createServer(null, 0, Collections.emptyList());
}
+ public boolean sslEncryptionEnabled() {
+ return this.sslFactory != null;
+ }
+
public TransportChannelHandler initializePipeline(
- SocketChannel channel, ChannelInboundHandlerAdapter decoder) {
- return initializePipeline(channel, decoder, msgHandler);
+ SocketChannel channel, ChannelInboundHandlerAdapter decoder, boolean isClient) {
+ return initializePipeline(channel, decoder, msgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
- SocketChannel channel, BaseMessageHandler resolvedMsgHandler) {
- return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler);
+ SocketChannel channel, BaseMessageHandler resolvedMsgHandler, boolean isClient) {
+ return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
SocketChannel channel,
ChannelInboundHandlerAdapter decoder,
- BaseMessageHandler resolvedMsgHandler) {
+ BaseMessageHandler resolvedMsgHandler,
+ boolean isClient) {
try {
ChannelPipeline pipeline = channel.pipeline();
if (nettyLogger.getLoggingHandler() != null) {
pipeline.addLast("loggingHandler", nettyLogger.getLoggingHandler());
}
+ if (sslEncryptionEnabled()) {
+ if (!isClient && !sslFactory.hasKeyManagers()) {
Review Comment:
If I understood your query - for non client connections, we need a private key in order for SSL encryption.
A client connection can also optionally provide a certificate, but it is not required.
In other words, at client side, keys are not required (will use server cert provided as part of the handshake) - but for server, keys are mandatory and is used for encryption/decryption - until a session key is negotiated.
Given this, we cannot pull this into `createSslFactory` as it is common for both client and servers.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Add SSLFactory, and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1547114021
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
Review Comment:
Unlike in spark, where clients do not have ssl cert, here we allow for ssl client to present their certificate - when available.
This is mainly relevant when master and workers communicate, and not so much between applications and master/worker (not common for cert to be present for spark/flink/mr apps) - providing for master/worker to validate each others identity. In future, this can be the basis for SASL EXTERNAL between s2s components !
See `SslConnectivitySuiteJ.testUntrustedClientCertFails` where this validation failure is tested.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Add SSLFactory, and enable TLS support [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2030980401
@mridulm, did you update any document? There are unnecessary changes of document in this pull request. Please check the changes of document.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Add SSLFactory, and enable TLS support [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1547208557
##########
common/src/main/java/org/apache/celeborn/common/network/TransportContext.java:
##########
@@ -135,31 +146,53 @@ public TransportServer createServer() {
return createServer(null, 0, Collections.emptyList());
}
+ public boolean sslEncryptionEnabled() {
+ return this.sslFactory != null;
+ }
+
public TransportChannelHandler initializePipeline(
- SocketChannel channel, ChannelInboundHandlerAdapter decoder) {
- return initializePipeline(channel, decoder, msgHandler);
+ SocketChannel channel, ChannelInboundHandlerAdapter decoder, boolean isClient) {
+ return initializePipeline(channel, decoder, msgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
- SocketChannel channel, BaseMessageHandler resolvedMsgHandler) {
- return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler);
+ SocketChannel channel, BaseMessageHandler resolvedMsgHandler, boolean isClient) {
+ return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
SocketChannel channel,
ChannelInboundHandlerAdapter decoder,
- BaseMessageHandler resolvedMsgHandler) {
+ BaseMessageHandler resolvedMsgHandler,
+ boolean isClient) {
try {
ChannelPipeline pipeline = channel.pipeline();
if (nettyLogger.getLoggingHandler() != null) {
pipeline.addLast("loggingHandler", nettyLogger.getLoggingHandler());
}
+ if (sslEncryptionEnabled()) {
+ if (!isClient && !sslFactory.hasKeyManagers()) {
Review Comment:
Could the check `!sslFactory.hasKeyManagers` move into `createSslFactory`? Meanwhile, does this need to throw exception when not a client connection? What about not add ` SslHandler` for non-client conection?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2038969072
@SteNicholas, please do let me know if I can help clarify better ... hopefully the details above help.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1548249729
##########
common/src/main/java/org/apache/celeborn/common/network/TransportContext.java:
##########
@@ -135,31 +146,53 @@ public TransportServer createServer() {
return createServer(null, 0, Collections.emptyList());
}
+ public boolean sslEncryptionEnabled() {
+ return this.sslFactory != null;
+ }
+
public TransportChannelHandler initializePipeline(
- SocketChannel channel, ChannelInboundHandlerAdapter decoder) {
- return initializePipeline(channel, decoder, msgHandler);
+ SocketChannel channel, ChannelInboundHandlerAdapter decoder, boolean isClient) {
+ return initializePipeline(channel, decoder, msgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
- SocketChannel channel, BaseMessageHandler resolvedMsgHandler) {
- return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler);
+ SocketChannel channel, BaseMessageHandler resolvedMsgHandler, boolean isClient) {
+ return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
SocketChannel channel,
ChannelInboundHandlerAdapter decoder,
- BaseMessageHandler resolvedMsgHandler) {
+ BaseMessageHandler resolvedMsgHandler,
+ boolean isClient) {
try {
ChannelPipeline pipeline = channel.pipeline();
if (nettyLogger.getLoggingHandler() != null) {
pipeline.addLast("loggingHandler", nettyLogger.getLoggingHandler());
}
+ if (sslEncryptionEnabled()) {
+ if (!isClient && !sslFactory.hasKeyManagers()) {
Review Comment:
If I understood your query - for non client connections, we need a private key in order for SSL encryption.
A client connection can also optionally provide a certificate, but it is not required.
In other words, at client side, keys are not required (will use server cert provided as part of the handshake) - but for server, keys are mandatory and is used for encryption/decryption - until a session key is negotiated.
Given this, we cannot pull this into `createSslFactory` as it is common for both client and servers. Please let me know if I am missing something here.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1555379433
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
Review Comment:
I was not sure if there is a lot of value in it for spark - in Celeborn this is mainly useful for s2s auth between master and workers - it is not that common for client apps to have signed certs
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas closed pull request #2438: [CELEBORN-1351] Introduce SSLFactory and enable TLS support
URL: https://github.com/apache/celeborn/pull/2438
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Add SSLFactory, and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1547114021
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
Review Comment:
Unlike in spark, where clients do not have ssl cert, here we allow for ssl client to present their certificate - when available.
This is mainly relevant when master and workers communicate, and not so much between applications and master/worker (not common for cert to be present for spark/flink/mr apps) - providing for master/worker to validate each others identity.
See `SslConnectivitySuiteJ.testUntrustedClientCertFails` where this validation failure is tested.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1554820664
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ return null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
Review Comment:
Updated (dropped the `ex.toString()` as well)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2041765229
Thanks for the reviews @SteNicholas, @otterc !
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Add SSLFactory, and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2030928771
The test failure does not look related to this PR (and the same tests passed [here](https://github.com/apache/celeborn/pull/2416)):
```
- celeborn spark integration test - pushdata timeout will add to pushExcludedWorkers *** FAILED ***
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Add SSLFactory, and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2031035546
Oh no, this is some weird artifact of pulling from my other branch - let me revert those, sorry about that !
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1554820605
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClientFactory.java:
##########
@@ -252,6 +257,31 @@ public void initChannel(SocketChannel ch) {
} else if (cf.cause() != null) {
throw new CelebornIOException(String.format("Failed to connect to %s", address), cf.cause());
}
+ if (context.sslEncryptionEnabled()) {
+ final SslHandler sslHandler = cf.channel().pipeline().get(SslHandler.class);
+ Future<Channel> future =
+ sslHandler
+ .handshakeFuture()
+ .addListener(
+ new GenericFutureListener<Future<Channel>>() {
+ @Override
+ public void operationComplete(final Future<Channel> handshakeFuture) {
+ if (handshakeFuture.isSuccess()) {
+ logger.debug("{} successfully completed TLS handshake to ", address);
+ } else {
+ logger.info(
+ "failed to complete TLS handshake to " + address,
Review Comment:
Done
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1554818383
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
Review Comment:
I dropped it given `SslConnectivitySuiteJ` we here added in Celeborn is more comprehensive and actually tests e2e ssl communication.
Particularly given we only support jsse and not openssl, the interaction between both sets of configs does not exist for Celeborn.
Do you think we should add it for completeness sake ?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1554820276
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ return null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
Review Comment:
Thanks for catching this ! This method is not used anymore - removed it.
OpenSSL is actually planned, but for later (see CELEBORN-1367).
Found another reference to open ssl in the code, and removed that as well.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1554820557
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClientFactory.java:
##########
@@ -252,6 +257,31 @@ public void initChannel(SocketChannel ch) {
} else if (cf.cause() != null) {
throw new CelebornIOException(String.format("Failed to connect to %s", address), cf.cause());
}
+ if (context.sslEncryptionEnabled()) {
+ final SslHandler sslHandler = cf.channel().pipeline().get(SslHandler.class);
+ Future<Channel> future =
+ sslHandler
+ .handshakeFuture()
+ .addListener(
+ new GenericFutureListener<Future<Channel>>() {
+ @Override
+ public void operationComplete(final Future<Channel> handshakeFuture) {
+ if (handshakeFuture.isSuccess()) {
+ logger.debug("{} successfully completed TLS handshake to ", address);
Review Comment:
Done
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1555382881
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
+ engine.setEnabledProtocols(enabledProtocols(engine, requestedProtocol));
+ engine.setEnabledCipherSuites(enabledCipherSuites(engine, requestedCiphers));
+ return engine;
+ }
+
+ private SSLEngine createEngine(boolean isClient, ByteBufAllocator allocator) {
+ return jdkSslContext.createSSLEngine();
+ }
+
+ private static final X509Certificate[] EMPTY_CERT_ARRAY = new X509Certificate[0];
+
+ private static TrustManager[] credulousTrustStoreManagers() {
+ return new TrustManager[] {
+ new X509TrustManager() {
+ @Override
+ public void checkClientTrusted(X509Certificate[] x509Certificates, String s)
+ throws CertificateException {}
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
+ throws CertificateException {}
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return EMPTY_CERT_ARRAY;
Review Comment:
The api expects an empty array and not null to be returned.
As part of testing various combinations, this surfaced, yes: though I dont recall if it was specifically related to want auth - it probably was
We need something similar to ssl connectivity test in Spark as well, which can help drive the matrix of combinations for testing (there are a lot more combinations in Spark)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1555046743
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
Review Comment:
If we have a more comprehensive test suite, then I don't think we need to add it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2032739430
To answer query about which modules are relevant, all modules can be secured using TLS.
At a minimum, given shared secret is negotiated when `LifecycleManager` registers with Master, it is advisible to secure rpc calls - towards that end, we will split the rpc calls within the application (driver/executors) and from client to server's (see CELEBORN-1356), so that users can independently configure those.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1555379433
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
Review Comment:
The api expects an empty array and not null to be returned.
As part of testing various combinations, this surfaced, yes.
We need something similar to ssl connectivity test in Spark as well.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2041577122
I will merge it tomorrow if it is still open.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "otterc (via GitHub)" <gi...@apache.org>.
otterc commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1554800829
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClientFactory.java:
##########
@@ -252,6 +257,31 @@ public void initChannel(SocketChannel ch) {
} else if (cf.cause() != null) {
throw new CelebornIOException(String.format("Failed to connect to %s", address), cf.cause());
}
+ if (context.sslEncryptionEnabled()) {
+ final SslHandler sslHandler = cf.channel().pipeline().get(SslHandler.class);
+ Future<Channel> future =
+ sslHandler
+ .handshakeFuture()
+ .addListener(
+ new GenericFutureListener<Future<Channel>>() {
+ @Override
+ public void operationComplete(final Future<Channel> handshakeFuture) {
+ if (handshakeFuture.isSuccess()) {
+ logger.debug("{} successfully completed TLS handshake to ", address);
+ } else {
+ logger.info(
+ "failed to complete TLS handshake to " + address,
Review Comment:
nit: `logger.info("failed to complete TLS handshake to {}", address, ...`
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
Review Comment:
There is a `SSLFactorySuite` in Spark. Should that be copied over?
##########
common/src/main/java/org/apache/celeborn/common/network/client/TransportClientFactory.java:
##########
@@ -252,6 +257,31 @@ public void initChannel(SocketChannel ch) {
} else if (cf.cause() != null) {
throw new CelebornIOException(String.format("Failed to connect to %s", address), cf.cause());
}
+ if (context.sslEncryptionEnabled()) {
+ final SslHandler sslHandler = cf.channel().pipeline().get(SslHandler.class);
+ Future<Channel> future =
+ sslHandler
+ .handshakeFuture()
+ .addListener(
+ new GenericFutureListener<Future<Channel>>() {
+ @Override
+ public void operationComplete(final Future<Channel> handshakeFuture) {
+ if (handshakeFuture.isSuccess()) {
+ logger.debug("{} successfully completed TLS handshake to ", address);
Review Comment:
super nit: `logger.debug("successfully completed TLS handshake to {}", address)
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ return null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
Review Comment:
Javadoc is saying something different than the implementation. Will the support for `OpenSSL` added later?
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ return null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
Review Comment:
nit: logger.info("Interrupted while destroying trust manager: {}", ex.toString(), ex);
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1554818383
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
Review Comment:
I dropped it given `SslConnectivitySuiteJ` we here added in Celeborn is more comprehensive and actually tests e2e ssl communication (and as part of it, configures ssl in various ways - which is a superset of what is in this suite).
Particularly given we only support jsse and not openssl, the interaction between both sets of configs does not exist for Celeborn.
Do you think we should add it for completeness sake ?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1554820664
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,405 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ return null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
Review Comment:
Updated (dropped the `toString()` for `ex` as well)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1548249729
##########
common/src/main/java/org/apache/celeborn/common/network/TransportContext.java:
##########
@@ -135,31 +146,53 @@ public TransportServer createServer() {
return createServer(null, 0, Collections.emptyList());
}
+ public boolean sslEncryptionEnabled() {
+ return this.sslFactory != null;
+ }
+
public TransportChannelHandler initializePipeline(
- SocketChannel channel, ChannelInboundHandlerAdapter decoder) {
- return initializePipeline(channel, decoder, msgHandler);
+ SocketChannel channel, ChannelInboundHandlerAdapter decoder, boolean isClient) {
+ return initializePipeline(channel, decoder, msgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
- SocketChannel channel, BaseMessageHandler resolvedMsgHandler) {
- return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler);
+ SocketChannel channel, BaseMessageHandler resolvedMsgHandler, boolean isClient) {
+ return initializePipeline(channel, new TransportFrameDecoder(), resolvedMsgHandler, isClient);
}
public TransportChannelHandler initializePipeline(
SocketChannel channel,
ChannelInboundHandlerAdapter decoder,
- BaseMessageHandler resolvedMsgHandler) {
+ BaseMessageHandler resolvedMsgHandler,
+ boolean isClient) {
try {
ChannelPipeline pipeline = channel.pipeline();
if (nettyLogger.getLoggingHandler() != null) {
pipeline.addLast("loggingHandler", nettyLogger.getLoggingHandler());
}
+ if (sslEncryptionEnabled()) {
+ if (!isClient && !sslFactory.hasKeyManagers()) {
Review Comment:
If I understood your query - for non client connections, we need a private key in order for SSL encryption.
A client connection can also optionally provide a certificate, but it is not required.
In other words, at client side, keys are not required (will use server cert provided as part of the handshake) - but for server, keys are mandatory and is used for encryption/decryption - until a session key is negotiated.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1555205907
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
Review Comment:
> Unlike in spark, where clients do not have ssl cert, here we allow for ssl client to present their certificate - when available.
Would be great to do this on Spark too. I had a PR to do this that I never got around to submitting as part of other feature tests, but I wasn't sure if it would have value in isolation beyond the other changes I had in mind (those proved infeasible).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2041764202
LGTM. Merging to main(v0.5.0).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Introduce SSLFactory and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#discussion_r1555382881
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/SSLFactory.java:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.celeborn.common.network.ssl;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.security.GeneralSecurityException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import javax.net.ssl.KeyManager;
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+
+import com.google.common.io.Files;
+import io.netty.buffer.ByteBufAllocator;
+import io.netty.handler.ssl.SslProvider;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.util.JavaUtils;
+
+/**
+ * SSLFactory to initialize and configure use of JSSE for SSL in Celeborn.
+ *
+ * <p>Note: code was initially copied from Apache Spark.
+ */
+public class SSLFactory {
+ private static final Logger logger = LoggerFactory.getLogger(SSLFactory.class);
+
+ /** For a configuration specifying keystore/truststore files */
+ private SSLContext jdkSslContext;
+
+ private KeyManager[] keyManagers;
+ private TrustManager[] trustManagers;
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+
+ private SSLFactory(final Builder b) {
+ this.requestedProtocol = b.requestedProtocol;
+ this.requestedCiphers = b.requestedCiphers;
+ try {
+ initJdkSslContext(b);
+ } catch (Exception e) {
+ throw new RuntimeException("SSLFactory creation failed", e);
+ }
+ }
+
+ private void initJdkSslContext(final Builder b) throws IOException, GeneralSecurityException {
+ this.keyManagers =
+ null != b.keyStore ? keyManagers(b.keyStore, b.keyPassword, b.keyStorePassword) : null;
+ this.trustManagers =
+ trustStoreManagers(
+ b.trustStore, b.trustStorePassword,
+ b.trustStoreReloadingEnabled, b.trustStoreReloadIntervalMs);
+ this.jdkSslContext = createSSLContext(requestedProtocol, keyManagers, trustManagers);
+ }
+
+ public boolean hasKeyManagers() {
+ // nettyServerSslContext requires keys to be specified, this path applies when
+ // applies only when jks is being used without keyManagers
+ return null == jdkSslContext || null != keyManagers;
+ }
+
+ /**
+ * If OpenSSL is requested, this will check if an implementation is available on the local host.
+ * If an implementation is not available it will fall back to the JDK {@link SslProvider}.
+ *
+ * @param b
+ * @return
+ */
+ private SslProvider getSslProvider(Builder b) {
+ return SslProvider.JDK;
+ }
+
+ public void destroy() {
+ if (trustManagers != null) {
+ for (int i = 0; i < trustManagers.length; i++) {
+ if (trustManagers[i] instanceof ReloadingX509TrustManager) {
+ try {
+ ((ReloadingX509TrustManager) trustManagers[i]).destroy();
+ } catch (InterruptedException ex) {
+ logger.info("Interrupted while destroying trust manager: " + ex.toString(), ex);
+ }
+ }
+ }
+ trustManagers = null;
+ }
+
+ keyManagers = null;
+ jdkSslContext = null;
+ requestedProtocol = null;
+ requestedCiphers = null;
+ }
+
+ /** Builder class to construct instances of {@link SSLFactory} with specific options */
+ public static class Builder {
+ private String requestedProtocol;
+ private String[] requestedCiphers;
+ private File keyStore;
+ private String keyStorePassword;
+ private String keyPassword;
+ private File trustStore;
+ private String trustStorePassword;
+ private boolean trustStoreReloadingEnabled;
+ private int trustStoreReloadIntervalMs;
+
+ /**
+ * Sets the requested protocol, i.e., "TLSv1.2", "TLSv1.1", etc
+ *
+ * @param requestedProtocol The requested protocol
+ * @return The builder object
+ */
+ public Builder requestedProtocol(String requestedProtocol) {
+ this.requestedProtocol = requestedProtocol == null ? "TLSv1.3" : requestedProtocol;
+ return this;
+ }
+
+ /**
+ * Sets the requested cipher suites, i.e., "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", etc
+ *
+ * @param requestedCiphers The requested ciphers
+ * @return The builder object
+ */
+ public Builder requestedCiphers(String[] requestedCiphers) {
+ this.requestedCiphers = requestedCiphers;
+ return this;
+ }
+
+ /**
+ * Sets the Keystore and Keystore password
+ *
+ * @param keyStore The key store file to use
+ * @param keyStorePassword The password for the key store
+ * @return The builder object
+ */
+ public Builder keyStore(File keyStore, String keyStorePassword) {
+ this.keyStore = keyStore;
+ this.keyStorePassword = keyStorePassword;
+ return this;
+ }
+
+ /**
+ * Sets the key password
+ *
+ * @param keyPassword The password for the private key in the key store
+ * @return The builder object
+ */
+ public Builder keyPassword(String keyPassword) {
+ this.keyPassword = keyPassword;
+ return this;
+ }
+
+ /**
+ * Sets the trust-store, trust-store password, whether to use a Reloading TrustStore, and the
+ * trust-store reload interval, if enabled
+ *
+ * @param trustStore The trust store file to use
+ * @param trustStorePassword The password for the trust store
+ * @param trustStoreReloadingEnabled Whether trust store reloading is enabled
+ * @param trustStoreReloadIntervalMs The interval at which to reload the trust store file
+ * @return The builder object
+ */
+ public Builder trustStore(
+ File trustStore,
+ String trustStorePassword,
+ boolean trustStoreReloadingEnabled,
+ int trustStoreReloadIntervalMs) {
+ this.trustStore = trustStore;
+ this.trustStorePassword = trustStorePassword;
+ this.trustStoreReloadingEnabled = trustStoreReloadingEnabled;
+ this.trustStoreReloadIntervalMs = trustStoreReloadIntervalMs;
+ return this;
+ }
+
+ /**
+ * Builds our {@link SSLFactory}
+ *
+ * @return The built {@link SSLFactory}
+ */
+ public SSLFactory build() {
+ return new SSLFactory(this);
+ }
+ }
+
+ /**
+ * Returns an initialized {@link SSLContext}
+ *
+ * @param requestedProtocol The requested protocol to use
+ * @param keyManagers The list of key managers to use
+ * @param trustManagers The list of trust managers to use
+ * @return The built {@link SSLContext}
+ * @throws GeneralSecurityException
+ */
+ private static SSLContext createSSLContext(
+ String requestedProtocol, KeyManager[] keyManagers, TrustManager[] trustManagers)
+ throws GeneralSecurityException {
+ SSLContext sslContext = SSLContext.getInstance(requestedProtocol);
+ sslContext.init(keyManagers, trustManagers, null);
+ return sslContext;
+ }
+
+ /**
+ * Creates a new {@link SSLEngine}. Note that currently client auth is not supported
+ *
+ * @param isClient Whether the engine is used in a client context
+ * @param allocator The {@link ByteBufAllocator to use}
+ * @return A valid {@link SSLEngine}.
+ */
+ public SSLEngine createSSLEngine(boolean isClient, ByteBufAllocator allocator) {
+ SSLEngine engine = createEngine(isClient, allocator);
+ engine.setUseClientMode(isClient);
+ engine.setWantClientAuth(true);
+ engine.setEnabledProtocols(enabledProtocols(engine, requestedProtocol));
+ engine.setEnabledCipherSuites(enabledCipherSuites(engine, requestedCiphers));
+ return engine;
+ }
+
+ private SSLEngine createEngine(boolean isClient, ByteBufAllocator allocator) {
+ return jdkSslContext.createSSLEngine();
+ }
+
+ private static final X509Certificate[] EMPTY_CERT_ARRAY = new X509Certificate[0];
+
+ private static TrustManager[] credulousTrustStoreManagers() {
+ return new TrustManager[] {
+ new X509TrustManager() {
+ @Override
+ public void checkClientTrusted(X509Certificate[] x509Certificates, String s)
+ throws CertificateException {}
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
+ throws CertificateException {}
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return EMPTY_CERT_ARRAY;
Review Comment:
The api expects an empty array and not null to be returned.
As part of testing various combinations, this surfaced, yes: though I do t recall if it was specifically related to want auth - it probably was
We need something similar to ssl connectivity test in Spark as well, which can help drive the matrix of combinations for testing (there are a lot more combinations in Spark)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1351] Add SSLFactory, and enable TLS support [celeborn]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #2438:
URL: https://github.com/apache/celeborn/pull/2438#issuecomment-2030930665
+CC @otterc, @FMX, @RexXiong, @SteNicholas, @waitinfuture
--
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.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org