You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by ch...@apache.org on 2020/09/08 19:54:37 UTC
[qpid-dispatch] branch master updated: DISPATCH-1762: Connector ssl
config errors must prohibit connections
This is an automated email from the ASF dual-hosted git repository.
chug pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/qpid-dispatch.git
The following commit(s) were added to refs/heads/master by this push:
new c55eb09 DISPATCH-1762: Connector ssl config errors must prohibit connections
c55eb09 is described below
commit c55eb09ab636e48262275439d71906d9fce1f86b
Author: Chuck Rolke <ch...@apache.org>
AuthorDate: Tue Sep 8 15:37:35 2020 -0400
DISPATCH-1762: Connector ssl config errors must prohibit connections
This patch aborts all connector connection attempts if there are
any configuration errors reported by Proton. Previously certain
combinations of hostname verification and certificate validity allowed
connections with unexpected ssl protection levels.
This patch uses clearer language in the error log messages to explain
the specific configuration errors and to identify the connections
that were denied.
* Each configuration error logs the reason indentifying it as a
configuration error and not as an in-band, openssl-detected problem.
* Configuration error logs include server connection id number and the
configured host and port.
* Connections closed due to internal config errors are abruptly closed.
A separate log message notes the connection id and the abort reason.
Thanks to Robbie Gemmell for analysis and code review.
This closes #843
---
src/server.c | 87 ++++++++-----
tests/system_tests_ssl.py | 311 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 367 insertions(+), 31 deletions(-)
diff --git a/src/server.c b/src/server.c
index 0eee2b5..0771be7 100644
--- a/src/server.c
+++ b/src/server.c
@@ -98,7 +98,7 @@ char *COMPONENT_SEPARATOR = ";";
static const int BACKLOG = 50; /* Listening backlog */
-static void setup_ssl_sasl_and_open(qd_connection_t *ctx);
+static bool setup_ssl_sasl_and_open(qd_connection_t *ctx); // true if ssl, sasl, and open succeeded
static qd_failover_item_t *qd_connector_get_conn_info(qd_connector_t *ct);
/**
@@ -755,7 +755,13 @@ static void on_connection_bound(qd_server_t *server, pn_event_t *e) {
ctx->connection_id, name, ctx->rhost_port);
} else if (ctx->connector) { /* Establishing an outgoing connection */
config = &ctx->connector->config;
- setup_ssl_sasl_and_open(ctx);
+ if (!setup_ssl_sasl_and_open(ctx)) {
+ qd_log(ctx->server->log_source, QD_LOG_ERROR, "[C%"PRIu64"] Connection aborted due to internal setup error",
+ ctx->connection_id);
+ pn_transport_close_tail(tport);
+ pn_transport_close_head(tport);
+ return;
+ }
} else { /* No connector and no listener */
connect_fail(ctx, QD_AMQP_COND_INTERNAL_ERROR, "unknown Connection");
@@ -1170,7 +1176,7 @@ static void try_open_lh(qd_connector_t *ct)
pn_proactor_connect(ct->server->proactor, ctx->pn_conn, host_port);
}
-static void setup_ssl_sasl_and_open(qd_connection_t *ctx)
+static bool setup_ssl_sasl_and_open(qd_connection_t *ctx)
{
qd_connector_t *ct = ctx->connector;
const qd_server_config_t *config = &ct->config;
@@ -1183,31 +1189,34 @@ static void setup_ssl_sasl_and_open(qd_connection_t *ctx)
pn_ssl_domain_t *domain = pn_ssl_domain(PN_SSL_MODE_CLIENT);
if (!domain) {
- qd_error(QD_ERROR_RUNTIME, "SSL domain failed for connection to %s:%s",
- ct->config.host, ct->config.port);
- return;
+ qd_error(QD_ERROR_RUNTIME, "SSL domain allocation failed for connection [C%"PRIu64"] to %s:%s",
+ ctx->connection_id, config->host, config->port);
+ return false;
}
+ bool failed = false;
+
// set our trusted database for checking the peer's cert:
if (config->ssl_trusted_certificate_db) {
if (pn_ssl_domain_set_trusted_ca_db(domain, config->ssl_trusted_certificate_db)) {
qd_log(ct->server->log_source, QD_LOG_ERROR,
- "SSL CA configuration failed for %s:%s",
- ct->config.host, ct->config.port);
+ "SSL CA configuration failed for connection [C%"PRIu64"] to %s:%s",
+ ctx->connection_id, config->host, config->port);
+ failed = true;
}
}
- // should we force the peer to provide a cert?
- if (config->ssl_require_peer_authentication) {
- const char *trusted = (config->ssl_trusted_certificates)
- ? config->ssl_trusted_certificates
- : config->ssl_trusted_certificate_db;
- if (pn_ssl_domain_set_peer_authentication(domain,
- PN_SSL_VERIFY_PEER,
- trusted)) {
- qd_log(ct->server->log_source, QD_LOG_ERROR,
- "SSL peer auth configuration failed for %s:%s",
- config->host, config->port);
- }
+
+ // peer must provide a cert
+ const char *trusted = (config->ssl_trusted_certificates)
+ ? config->ssl_trusted_certificates
+ : config->ssl_trusted_certificate_db;
+ if (pn_ssl_domain_set_peer_authentication(domain,
+ PN_SSL_VERIFY_PEER,
+ trusted)) {
+ qd_log(ct->server->log_source, QD_LOG_ERROR,
+ "SSL peer auth configuration failed for connection [C%"PRIu64"] to %s:%s",
+ ctx->connection_id, config->host, config->port);
+ failed = true;
}
// configure our certificate if the peer requests one:
@@ -1217,39 +1226,54 @@ static void setup_ssl_sasl_and_open(qd_connection_t *ctx)
config->ssl_private_key_file,
config->ssl_password)) {
qd_log(ct->server->log_source, QD_LOG_ERROR,
- "SSL local configuration failed for %s:%s",
- config->host, config->port);
+ "SSL local certificate configuration failed for connection [C%"PRIu64"] to %s:%s",
+ ctx->connection_id, config->host, config->port);
+ failed = true;
}
}
if (config->ssl_ciphers) {
if (pn_ssl_domain_set_ciphers(domain, config->ssl_ciphers)) {
qd_log(ct->server->log_source, QD_LOG_ERROR,
- "SSL cipher configuration failed for %s:%s",
- config->host, config->port);
+ "SSL cipher configuration failed for connection [C%"PRIu64"] to %s:%s",
+ ctx->connection_id, config->host, config->port);
+ failed = true;
}
}
if (config->ssl_protocols) {
if (pn_ssl_domain_set_protocols(domain, config->ssl_protocols)) {
qd_log(ct->server->log_source, QD_LOG_ERROR,
- "Permitted TLS protocols configuration failed %s:%s",
- config->host, config->port);
+ "Permitted TLS protocols configuration failed for connection [C%"PRIu64"] to %s:%s",
+ ctx->connection_id, config->host, config->port);
+ failed = true;
}
}
//If ssl is enabled and verify_host_name is true, instruct proton to verify peer name
if (config->verify_host_name) {
if (pn_ssl_domain_set_peer_authentication(domain, PN_SSL_VERIFY_PEER_NAME, NULL)) {
- qd_log(ct->server->log_source, QD_LOG_ERROR,
- "SSL peer host name verification failed for %s:%s",
- config->host, config->port);
+ qd_log(ct->server->log_source, QD_LOG_ERROR,
+ "SSL peer host name verification configuration failed for connection [C%"PRIu64"] to %s:%s",
+ ctx->connection_id, config->host, config->port);
+ failed = true;
}
}
- ctx->ssl = pn_ssl(tport);
- pn_ssl_init(ctx->ssl, domain, 0);
+ if (!failed) {
+ ctx->ssl = pn_ssl(tport);
+ if (pn_ssl_init(ctx->ssl, domain, 0) != 0) {
+ qd_log(ct->server->log_source, QD_LOG_ERROR,
+ "SSL domain internal initialization failed for connection [C%"PRIu64"] to %s:%s",
+ ctx->connection_id, config->host, config->port);
+ failed = true;
+ }
+ }
pn_ssl_domain_free(domain);
+ if (failed) {
+ return false;
+ }
+
}
//
@@ -1263,6 +1287,7 @@ static void setup_ssl_sasl_and_open(qd_connection_t *ctx)
sys_mutex_unlock(ct->server->lock);
pn_connection_open(ctx->pn_conn);
+ return true;
}
static void try_open_cb(void *context) {
diff --git a/tests/system_tests_ssl.py b/tests/system_tests_ssl.py
index b150955..1e0cd0c 100644
--- a/tests/system_tests_ssl.py
+++ b/tests/system_tests_ssl.py
@@ -24,6 +24,7 @@ import os
import ssl
import sys
import re
+import time
from subprocess import Popen, PIPE
from qpid_dispatch.management.client import Node
from system_test import TestCase, main_module, Qdrouterd, DIR, SkipIfNeeded
@@ -780,5 +781,315 @@ class RouterTestSslInterRouter(RouterTestSslBase):
self.assertEqual(len(router_nodes), expected_nodes)
+class RouterTestSslInterRouterWithInvalidPathToCA(RouterTestSslBase):
+ """
+ DISPATCH-1762
+ Starts 2 routers:
+ Router A two listeners serve a normal, good certificate
+ Router B two connectors configured with an invalid CA file path in its profile
+ - one sets verifyHostname true, the other false.
+ Test proves:
+ Router B must not connect to A with mis-configured CA file path regardless of
+ verifyHostname setting.
+ """
+ # Listener ports for each TLS protocol definition
+ PORT_NO_SSL = 0
+ PORT_TLS_ALL = 0
+
+ @classmethod
+ def setUpClass(cls):
+ """
+ Prepares 2 routers to form a network.
+ """
+ super(RouterTestSslInterRouterWithInvalidPathToCA, cls).setUpClass()
+
+ if not SASL.extended():
+ return
+
+ os.environ["ENV_SASL_PASSWORD"] = "password"
+
+ # Generate authentication DB
+ super(RouterTestSslInterRouterWithInvalidPathToCA, cls).create_sasl_files()
+
+ # Router expected to be connected
+ cls.connected_tls_sasl_routers = []
+
+ # Generated router list
+ cls.routers = []
+
+ # Saving listener ports for each TLS definition
+ cls.PORT_NO_SSL = cls.tester.get_port()
+ cls.PORT_TLS_ALL_1 = cls.tester.get_port()
+ cls.PORT_TLS_ALL_2 = cls.tester.get_port()
+
+ # Configured connector host
+ cls.CONNECTOR_HOST = "localhost"
+
+ config_a = Qdrouterd.Config([
+ ('router', {'id': 'QDR.A',
+ 'mode': 'interior',
+ 'saslConfigName': 'tests-mech-PLAIN',
+ 'saslConfigDir': os.getcwd()}),
+ # No auth and no SSL for management access
+ ('listener', {'host': '0.0.0.0', 'role': 'normal', 'port': cls.PORT_NO_SSL}),
+ # All TLS versions and normal, good sslProfile config
+ ('listener', {'host': '0.0.0.0', 'role': 'inter-router', 'port': cls.PORT_TLS_ALL_1,
+ 'saslMechanisms': 'PLAIN',
+ 'requireEncryption': 'yes', 'requireSsl': 'yes',
+ 'sslProfile': 'ssl-profile-tls-all'}),
+ # All TLS versions and normal, good sslProfile config
+ ('listener', {'host': '0.0.0.0', 'role': 'inter-router', 'port': cls.PORT_TLS_ALL_2,
+ 'saslMechanisms': 'PLAIN',
+ 'requireEncryption': 'yes', 'requireSsl': 'yes',
+ 'sslProfile': 'ssl-profile-tls-all'}),
+ # SSL Profile for all TLS versions (protocols element not defined)
+ ('sslProfile', {'name': 'ssl-profile-tls-all',
+ 'caCertFile': cls.ssl_file('ca-certificate.pem'),
+ 'certFile': cls.ssl_file('server-certificate.pem'),
+ 'privateKeyFile': cls.ssl_file('server-private-key.pem'),
+ 'ciphers': 'ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:' \
+ 'DH+AES:RSA+AESGCM:RSA+AES:!aNULL:!MD5:!DSS',
+ 'password': 'server-password'})
+ ])
+
+ # Router B has a connector to listener that allows all protocols but will not verify hostname.
+ # The sslProfile has a bad caCertFile name and this router should not connect.
+ config_b = Qdrouterd.Config([
+ ('router', {'id': 'QDR.B',
+ 'mode': 'interior'}),
+ # Connector to All TLS versions allowed listener
+ ('connector', {'name': 'connector1',
+ 'host': cls.CONNECTOR_HOST, 'role': 'inter-router',
+ 'port': cls.PORT_TLS_ALL_1,
+ 'verifyHostname': 'no', 'saslMechanisms': 'PLAIN',
+ 'saslUsername': 'test@domain.com', 'saslPassword': 'pass:password',
+ 'sslProfile': 'ssl-profile-tls-all'}),
+ # Connector to All TLS versions allowed listener
+ ('connector', {'name': 'connector2',
+ 'host': cls.CONNECTOR_HOST, 'role': 'inter-router',
+ 'port': cls.PORT_TLS_ALL_2,
+ 'verifyHostname': 'yes', 'saslMechanisms': 'PLAIN',
+ 'saslUsername': 'test@domain.com', 'saslPassword': 'pass:password',
+ 'sslProfile': 'ssl-profile-tls-all'}),
+ # SSL Profile with an invalid caCertFile file path. The correct file path here would allow this
+ # router to connect. The object is to trigger a specific failure in the ssl
+ # setup chain of calls to pn_ssl_domain_* functions.
+ ('sslProfile', {'name': 'ssl-profile-tls-all',
+ 'caCertFile': cls.ssl_file('ca-certificate-INVALID-FILENAME.pem'),
+ 'ciphers': 'ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:' \
+ 'DH+AES:RSA+AESGCM:RSA+AES:!aNULL:!MD5:!DSS'})
+ ])
+
+ cls.routers.append(cls.tester.qdrouterd("A", config_a, wait=False))
+ cls.routers.append(cls.tester.qdrouterd("B", config_b, wait=False))
+
+ # Wait until A is running
+ cls.routers[0].wait_ports()
+
+ # Can't wait until B is connected because it's not supposed to connect.
+
+ def get_router_nodes(self):
+ """
+ Retrieves connected router nodes from QDR.A
+ :return: list of connected router id's
+ """
+ if not SASL.extended():
+ self.skipTest("Cyrus library not available. skipping test")
+
+ url = Url("amqp://0.0.0.0:%d/$management" % self.PORT_NO_SSL)
+ node = Node.connect(url)
+ response = node.query(type="org.apache.qpid.dispatch.router.node", attribute_names=["id"])
+ router_nodes = []
+ for resp in response.get_dicts():
+ router_nodes.append(resp['id'])
+ node.close()
+ return router_nodes
+
+ def test_invalid_ca_path(self):
+ """
+ Prove sslProfile with invalid path to CA prevents the router from joining the network
+ """
+ if not SASL.extended():
+ self.skipTest("Cyrus library not available. skipping test")
+
+ # Poll for a while until the connector error shows up in router B's log
+ pattern = " SERVER (error) SSL CA configuration failed"
+ host_port_1 = self.CONNECTOR_HOST + ":" + str(self.PORT_TLS_ALL_1)
+ host_port_2 = self.CONNECTOR_HOST + ":" + str(self.PORT_TLS_ALL_2)
+ sleep_time = 0.1 # seconds
+ poll_duration = 60.0 # seconds
+ verified = False
+ for tries in range(int(poll_duration / sleep_time)):
+ logfile = os.path.join(self.routers[1].outdir, self.routers[1].logfile)
+ if os.path.exists(logfile):
+ with open(logfile, 'r') as router_log:
+ log_lines = router_log.read().split("\n")
+ e1_lines = [s for s in log_lines if pattern in s and host_port_1 in s]
+ e2_lines = [s for s in log_lines if pattern in s and host_port_2 in s]
+ verified = len(e1_lines) > 0 and len(e2_lines) > 0
+ if verified:
+ break;
+ time.sleep(sleep_time)
+ self.assertTrue(verified, "Log line containing '%s' not seen for both connectors in QDR.B log" % pattern)
+
+ verified = False
+ pattern1 = "Connection to %s failed:" % host_port_1
+ pattern2 = "Connection to %s failed:" % host_port_2
+ for tries in range(int(poll_duration / sleep_time)):
+ logfile = os.path.join(self.routers[1].outdir, self.routers[1].logfile)
+ if os.path.exists(logfile):
+ with open(logfile, 'r') as router_log:
+ log_lines = router_log.read().split("\n")
+ e1_lines = [s for s in log_lines if pattern1 in s]
+ e2_lines = [s for s in log_lines if pattern2 in s]
+ verified = len(e1_lines) > 0 and len(e2_lines) > 0
+ if verified:
+ break;
+ time.sleep(sleep_time)
+ self.assertTrue(verified, "Log line containing '%s' or '%s' not seen in QDR.B log" % (pattern1, pattern2))
+
+ # Show that router A does not have router B in its network
+ router_nodes = self.get_router_nodes()
+ self.assertTrue(router_nodes)
+ node = "QDR.B"
+ self.assertNotIn(node, router_nodes, msg=("%s should not be connected" % node))
+
+
+class RouterTestSslInterRouterWithoutHostnameVerificationAndMismatchedCA(RouterTestSslBase):
+ """
+ DISPATCH-1762
+ Starts 2 routers:
+ Router A listener serves a normal, good certificate.
+ Router B connector is configured with a CA cert that did not sign the server cert, and verifyHostname is false.
+ Test proves:
+ Router B must not connect to A.
+ """
+ # Listener ports for each TLS protocol definition
+ PORT_NO_SSL = 0
+ PORT_TLS_ALL = 0
+
+ @classmethod
+ def setUpClass(cls):
+ """
+ Prepares 2 routers to form a network.
+ """
+ super(RouterTestSslInterRouterWithoutHostnameVerificationAndMismatchedCA, cls).setUpClass()
+
+ if not SASL.extended():
+ return
+
+ os.environ["ENV_SASL_PASSWORD"] = "password"
+
+ # Generate authentication DB
+ super(RouterTestSslInterRouterWithoutHostnameVerificationAndMismatchedCA, cls).create_sasl_files()
+
+ # Router expected to be connected
+ cls.connected_tls_sasl_routers = []
+
+ # Generated router list
+ cls.routers = []
+
+ # Saving listener ports for each TLS definition
+ cls.PORT_NO_SSL = cls.tester.get_port()
+ cls.PORT_TLS_ALL = cls.tester.get_port()
+
+ config_a = Qdrouterd.Config([
+ ('router', {'id': 'QDR.A',
+ 'mode': 'interior',
+ 'saslConfigName': 'tests-mech-PLAIN',
+ 'saslConfigDir': os.getcwd()}),
+ # No auth and no SSL for management access
+ ('listener', {'host': '0.0.0.0', 'role': 'normal', 'port': cls.PORT_NO_SSL}),
+ # All TLS versions and normal, good sslProfile config
+ ('listener', {'host': '0.0.0.0', 'role': 'inter-router', 'port': cls.PORT_TLS_ALL,
+ 'saslMechanisms': 'PLAIN',
+ 'requireEncryption': 'yes', 'requireSsl': 'yes',
+ 'sslProfile': 'ssl-profile-tls-all'}),
+ # SSL Profile for all TLS versions (protocols element not defined)
+ ('sslProfile', {'name': 'ssl-profile-tls-all',
+ 'caCertFile': cls.ssl_file('ca-certificate.pem'),
+ 'certFile': cls.ssl_file('server-certificate.pem'),
+ 'privateKeyFile': cls.ssl_file('server-private-key.pem'),
+ 'ciphers': 'ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:' \
+ 'DH+AES:RSA+AESGCM:RSA+AES:!aNULL:!MD5:!DSS',
+ 'password': 'server-password'})
+ ])
+
+ # Router B has a connector to listener that allows all protocols but will not verify hostname.
+ # The sslProfile has a caCertFile that does not sign the server cert, so this router should not connect.
+ config_b = Qdrouterd.Config([
+ ('router', {'id': 'QDR.B',
+ 'mode': 'interior'}),
+ # Connector to All TLS versions allowed listener
+ ('connector', {'host': 'localhost', 'role': 'inter-router', 'port': cls.PORT_TLS_ALL,
+ 'verifyHostname': 'no', 'saslMechanisms': 'PLAIN',
+ 'saslUsername': 'test@domain.com', 'saslPassword': 'pass:password',
+ 'sslProfile': 'ssl-profile-tls-all'}),
+ # SSL Profile with caCertFile to cert that does not sign the server cert. The correct path here would allow this
+ # router to connect. The object is to trigger a certificate verification failure while hostname verification is off.
+ ('sslProfile', {'name': 'ssl-profile-tls-all',
+ 'caCertFile': cls.ssl_file('bad-ca-certificate.pem'),
+ 'ciphers': 'ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:' \
+ 'DH+AES:RSA+AESGCM:RSA+AES:!aNULL:!MD5:!DSS'})
+ ])
+
+ cls.routers.append(cls.tester.qdrouterd("A", config_a, wait=False))
+ cls.routers.append(cls.tester.qdrouterd("B", config_b, wait=False))
+
+ # Wait until A is running
+ cls.routers[0].wait_ports()
+
+ # Can't wait until B is connected because it's not supposed to connect.
+
+ def get_router_nodes(self):
+ """
+ Retrieves connected router nodes from QDR.A
+ :return: list of connected router id's
+ """
+ if not SASL.extended():
+ self.skipTest("Cyrus library not available. skipping test")
+
+ url = Url("amqp://0.0.0.0:%d/$management" % self.PORT_NO_SSL)
+ node = Node.connect(url)
+ response = node.query(type="org.apache.qpid.dispatch.router.node", attribute_names=["id"])
+ router_nodes = []
+ for resp in response.get_dicts():
+ router_nodes.append(resp['id'])
+ node.close()
+ return router_nodes
+
+ def test_mismatched_ca_and_no_hostname_verification(self):
+ """
+ Prove that improperly configured ssl-enabled connector prevents the router
+ from joining the network
+ """
+ if not SASL.extended():
+ self.skipTest("Cyrus library not available. skipping test")
+
+ # Poll for a while until the connector error shows up in router B's log
+ pattern = "Connection to localhost:%s failed:" % self.PORT_TLS_ALL
+ sleep_time = 0.1 # seconds
+ poll_duration = 60.0 # seconds
+ verified = False
+ for tries in range(int(poll_duration / sleep_time)):
+ logfile = os.path.join(self.routers[1].outdir, self.routers[1].logfile)
+ if os.path.exists(logfile):
+ with open(logfile, 'r') as router_log:
+ log_lines = router_log.read().split("\n")
+ e_lines = [s for s in log_lines if pattern in s]
+ verified = len(e_lines) > 0
+ if verified:
+ break;
+ time.sleep(sleep_time)
+ self.assertTrue(verified, "Log line containing '%s' not seen in QDR.B log" % pattern)
+
+ # Show that router A does not have router B in its network
+ router_nodes = self.get_router_nodes()
+ self.assertTrue(router_nodes)
+ node = "QDR.B"
+ self.assertNotIn(node, router_nodes, msg=("%s should not be connected" % node))
+
+
+
if __name__ == '__main__':
unittest.main(main_module())
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org