ywcb00 commented on code in PR #2569:
URL: https://github.com/apache/systemds/pull/2569#discussion_r3710942250
##########
src/test/java/org/apache/sysds/test/functions/federated/io/FederatedSSLTest.java:
##########
@@ -18,132 +18,249 @@
*/
package org.apache.sysds.test.functions.federated.io;
-
import java.io.File;
-import java.util.Arrays;
-import java.util.Collection;
+import java.net.InetSocketAddress;
+import java.security.cert.CertificateException;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
+
+import javax.net.ssl.SSLException;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.sysds.common.Types;
+import org.apache.sysds.conf.ConfigurationManager;
+import org.apache.sysds.conf.DMLConfig;
import org.apache.sysds.runtime.controlprogram.caching.MatrixObject;
import org.apache.sysds.runtime.controlprogram.federated.FederatedData;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedRequest;
+import
org.apache.sysds.runtime.controlprogram.federated.FederatedRequest.RequestType;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedResponse;
+import org.apache.sysds.runtime.DMLRuntimeException;
+import org.apache.sysds.runtime.controlprogram.federated.FederatedSSLUtil;
import org.apache.sysds.runtime.meta.MatrixCharacteristics;
import org.apache.sysds.test.AutomatedTestBase;
import org.apache.sysds.test.TestConfiguration;
import org.apache.sysds.test.TestUtils;
import
org.apache.sysds.test.functions.federated.FederatedTestObjectConstructor;
import org.junit.Assert;
-import org.junit.Ignore;
+import org.junit.Before;
import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-@RunWith(value = Parameterized.class)
@net.jcip.annotations.NotThreadSafe
public class FederatedSSLTest extends AutomatedTestBase {
private static final Log LOG =
LogFactory.getLog(FederatedSSLTest.class.getName());
- // This test use the same scripts as the Federated Reader tests, just
with SSL enabled.
+ // These tests use the same scripts as the Federated Reader tests, just
with SSL enabled.
private final static String TEST_DIR = "functions/federated/io/";
private final static String TEST_NAME = "FederatedReaderTest";
private final static String TEST_CLASS_DIR = TEST_DIR +
FederatedSSLTest.class.getSimpleName() + "/";
private final static int blocksize = 1024;
- private final static File TEST_CONF_FILE = new File(SCRIPT_DIR +
TEST_DIR + "SSLConfig.xml");
+ private final static int rows = 10;
+ private final static int cols = 13;
- @Parameterized.Parameter()
- public int rows;
- @Parameterized.Parameter(1)
- public int cols;
- @Parameterized.Parameter(2)
- public boolean rowPartitioned;
- @Parameterized.Parameter(3)
- public int fedCount;
+ private final static String CONF_DIR = SCRIPT_DIR + TEST_DIR +
"config/";
+ // Certificate issued for localhost and signed by the authority the
coordinator trusts
+ private final static File SIGNED_CONF = new File(CONF_DIR,
"SignedSSLConfig.xml");
+ // Coordinator trusting an authority unrelated to the one that signed
the worker certificate
+ private final static File UNTRUSTED_CONF = new File(CONF_DIR,
"UntrustedSSLConfig.xml");
+ // Certificate signed by the trusted authority, but issued for another
host
+ private final static File OTHER_HOST_CONF = new File(CONF_DIR,
"OtherHostSSLConfig.xml");
+ // SSL enabled without configuring any certificate, which is not
supported
+ private final static File NO_CERT_CONF = new File(SCRIPT_DIR + TEST_DIR
+ "SSLConfig.xml");
+
+ private File confFile = SIGNED_CONF;
+ private File workerConfFile = null;
@Override
public void setUp() {
TestUtils.clearAssertionInformation();
addTestConfiguration(TEST_NAME, new
TestConfiguration(TEST_CLASS_DIR, TEST_NAME));
}
- @Parameterized.Parameters
- public static Collection<Object[]> data() {
- // number of rows or cols has to be >= number of federated
locations.
- return Arrays.asList(new Object[][] {{10, 13, true, 2}});
+ @Before
+ public void clearSSLState() {
+ // The SSL context of the coordinator is cached for the JVM, so
it should be cleared in between the tests
+ FederatedSSLUtil.resetClientContext();
+ FederatedData.resetFederatedSites();
Review Comment:
Can we move this code to the `setUp` method to only have one method that is
executed before the tests?
##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedSSLUtil.java:
##########
@@ -40,24 +50,93 @@ private FederatedSSLUtil(){
/** A Singleton constructed SSL context, that only is assigned if ssl
is enabled. */
private static SslContextMan sslInstance = null;
- protected static SslContextMan SslConstructor() {
+ protected synchronized static SslContextMan SslConstructor() {
if(sslInstance == null)
- return new SslContextMan();
- else
- return sslInstance;
+ sslInstance = new SslContextMan();
+ return sslInstance;
+ }
+
+ // Drop the cached client side SSL context, so that the next connection
is built from the current configuration.
+ // Only relevant if the configuration changes while the JVM is running,
as it does in tests.
+ public synchronized static void resetClientContext() {
+ sslInstance = null;
}
protected static SslHandler createSSLHandler(SocketChannel ch,
InetSocketAddress address) {
- return SslConstructor().context.newHandler(ch.alloc(),
address.getAddress().getHostAddress(), address.getPort());
+ final SslContextMan man = SslConstructor();
+ // prefer the configured host name over the resolved address,
since certificates are issued for host names.
+ final String host = (address.getHostString() != null) ?
address.getHostString() : address.getAddress()
+ .getHostAddress();
+ final SslHandler handler = man.context.newHandler(ch.alloc(),
host, address.getPort());
+
+ // the certificate of a worker has to be issued for the host it
is contacted on, otherwise any worker
+ // with a trusted certificate could impersonate any other
worker.
+ final SSLEngine engine = handler.engine();
+ final SSLParameters params = engine.getSSLParameters();
+ params.setEndpointIdentificationAlgorithm("HTTPS");
+ engine.setSSLParameters(params);
+
+ return handler;
+ }
+
+ /**
+ * Construct the SSL context of a federated worker, based on the
certificate and private key configured via
+ * {@link DMLConfig#FEDERATED_SSL_CERT} and {@link
DMLConfig#FEDERATED_SSL_KEY}. Both are required, a worker that
+ * cannot be authenticated by the coordinator is not supported. If the
private key is encrypted, its password is
+ * read from the {@link #SSL_KEY_PASSWORD_ENV} environment variable.
+ *
+ * @return The server side SSL context of the federated worker
+ */
+ public static SslContext createServerContext() {
+ final DMLConfig conf = ConfigurationManager.getDMLConfig();
+ final String certPath =
conf.getTextValue(DMLConfig.FEDERATED_SSL_CERT);
+ final String keyPath =
conf.getTextValue(DMLConfig.FEDERATED_SSL_KEY);
+ final String keyPassword = System.getenv(SSL_KEY_PASSWORD_ENV);
+
+ if(!isSet(certPath) || !isSet(keyPath))
+ throw new DMLRuntimeException("Federated SSL requires a
signed certificate, configure the certificate "
+ + "chain in " + DMLConfig.FEDERATED_SSL_CERT +
" and the matching private key in "
+ + DMLConfig.FEDERATED_SSL_KEY + ".");
+
+ try {
+ LOG.info("Federated worker SSL using certificate: " +
certPath);
+ return SslContextBuilder
+ .forServer(readableFile(certPath,
DMLConfig.FEDERATED_SSL_CERT),
+ readableFile(keyPath,
DMLConfig.FEDERATED_SSL_KEY), isSet(keyPassword) ? keyPassword : null)
+ .build();
+ }
+ catch(SSLException e) {
+ throw new DMLRuntimeException("Static SSL setup failed
for worker side", e);
+ }
+ }
+
+ private static boolean isSet(String value) {
+ return value != null && !value.trim().isEmpty();
}
+ private static File readableFile(String path, String configName) {
+ final File f = new File(path.trim());
+ if(!f.canRead())
+ throw new DMLRuntimeException(
+ "Federated SSL file configured in " +
configName + " is not a readable file: " + path);
+ return f;
+ }
private static class SslContextMan {
protected final SslContext context;
private SslContextMan() {
+ final DMLConfig conf =
ConfigurationManager.getDMLConfig();
+ final String trustPath =
conf.getTextValue(DMLConfig.FEDERATED_SSL_TRUST);
+
+ if(!isSet(trustPath))
+ throw new DMLRuntimeException("Federated SSL
requires the certificates that are trusted to sign "
+ + "worker certificates, configure them
in " + DMLConfig.FEDERATED_SSL_TRUST + ".");
+
try {
- context =
SslContextBuilder.forClient().trustManager(InsecureTrustManagerFactory.INSTANCE).build();
+ LOG.info("Federated SSL trusting certificates
in: " + trustPath);
Review Comment:
This LOG should be at debug level
##########
src/main/java/org/apache/sysds/runtime/controlprogram/federated/FederatedData.java:
##########
@@ -308,18 +310,43 @@ public synchronized static void createWorkGroup() {
}
private static class DataRequestHandler extends
ChannelInboundHandlerAdapter {
+ // The promise is assigned by the requesting thread, while the
channel events below are handled on the
+ // event loop, and the two orders are not guaranteed: a
rejected SSL handshake already fails the channel
+ // while the requesting thread is still connecting.
+ private final Object _promLock = new Object();
Review Comment:
Is there a specific reason for creating an object for locking the member
variable `_prom`? I think that we can just use the intrinsic lock from the
member variable `_prom` directly, and thus synchronize on `_prom` instead of
`_promLock`.
##########
scripts/tutorials/federated/conf/ssl.xml:
##########
@@ -18,4 +18,13 @@
-->
<root>
<sysds.federated.ssl>true</sysds.federated.ssl>
+
+ <!-- Required with SSL: the workers need a certificate issued for the host
the coordinator contacts them on
+ and signed by an authority the coordinator trusts, fill in the paths
below before using this
+ configuration. An encrypted private key takes its password from the
environment variable
+ SYSTEMDS_FEDERATED_SSL_KEY_PASSWORD.
+
<sysds.federated.ssl.cert>/path/to/worker-cert.pem</sysds.federated.ssl.cert>
+ <sysds.federated.ssl.key>/path/to/worker-key.pem</sysds.federated.ssl.key>
+ <sysds.federated.ssl.trust>/path/to/ca-cert.pem</sysds.federated.ssl.trust>
Review Comment:
Since these configurations are required when ssl is enabled, I would include
the configurations in this xml file. I see the point that we do not have any
certificates for the corresponding tutorial yet, leaving us with three options
to resolve this issue:
1. Generate respective certificates for the tutorial and include them in the
repository.
2. Include the configurations with the current placeholder value
(path/to/...) and ensure that, when executing the tutorial with ssl, it is
clear from the error messages that these certificates are missing.
3. Remove the 'ssl' option from `scripts/tutorials/federated/parameters.sh`.
I think the first option would be the best, and the second option fits the
scope of this PR best.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
