You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/09/02 14:14:10 UTC
[GitHub] [qpid-dispatch] ChugR opened a new pull request #843: DISPATCH-1762: Connector ssl config errors must prohibit connections
ChugR opened a new pull request #843:
URL: https://github.com/apache/qpid-dispatch/pull/843
Errors seen passing ssl protocol settings to Proton were logged but
the connector was allowed to connect anyway.
The initial complaint in DISPATCH-1762 was about how a bad ca cert
file name allowed the connection to proceed without using any ca cert
file. A self test is added to check that this particular case is fixed.
This patch prevents all connector connection attempts if there are
any configuration errors reported by Proton.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] asfgit closed pull request #843: DISPATCH-1762: Connector ssl config errors must prohibit connections
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #843:
URL: https://github.com/apache/qpid-dispatch/pull/843
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #843: DISPATCH-1762: Connector ssl config errors must prohibit connections
Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #843:
URL: https://github.com/apache/qpid-dispatch/pull/843#discussion_r482469927
##########
File path: tests/system_tests_ssl.py
##########
@@ -780,5 +781,148 @@ def test_connected_tls_sasl_routers(self):
self.assertEqual(len(router_nodes), expected_nodes)
+class RouterTestSslInterRouterVerifyHostname(RouterTestSslBase):
+ """
+ DISPATCH-1762
+ Starts 2 routers:
+ Router A listener serves a normal, good certificate
+ Router B connector is configured with a bad CA in its profile but verifyHostname is false.
+ Test proves:
+ Router B must not connect to A.
+
+ DISPATCH-1762 fixes several issues related to failed ssl setup
+ allowing a connector to connect anyway.
+ This test checks only one of the failures.
+ """
+ # 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(RouterTestSslInterRouterVerifyHostname, cls).setUpClass()
+
+ if not SASL.extended():
+ return
+
+ os.environ["ENV_SASL_PASSWORD"] = "password"
+
+ # Generate authentication DB
+ super(RouterTestSslInterRouterVerifyHostname, 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 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', {'host': '0.0.0.0', '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 bad caCertFile name. The correct name 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-BAD.pem'),
+ 'certFile': cls.ssl_file('client-certificate.pem'),
+ 'privateKeyFile': cls.ssl_file('client-private-key.pem'),
+ 'ciphers': 'ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:' \
+ 'DH+AES:RSA+AESGCM:RSA+AES:!aNULL:!MD5:!DSS',
+ 'password': 'client-password'})
+ ])
+
+ 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
+
+ @SkipIfNeeded(RouterTestSslBase.DISABLE_SSL_TESTING or not SASL.extended(),
+ "Cyrus library not available. skipping test")
+ def test_connected_misconfigured_tls_sasl_routers(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 = " SERVER (error) SSL CA configuration failed"
+ sleep_time = 0.1 # seconds
+ poll_duration = 10.0 # seconds
+ verified = False
+ for tries in range(int(poll_duration / sleep_time)):
+ with open('../setUpClass/B.log', '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.assertTrue(node not in router_nodes, "%s should not be connected" % node)
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #843: DISPATCH-1762: Connector ssl config errors must prohibit connections
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #843:
URL: https://github.com/apache/qpid-dispatch/pull/843#discussion_r482393487
##########
File path: tests/system_tests_ssl.py
##########
@@ -780,5 +781,148 @@ def test_connected_tls_sasl_routers(self):
self.assertEqual(len(router_nodes), expected_nodes)
+class RouterTestSslInterRouterVerifyHostname(RouterTestSslBase):
+ """
+ DISPATCH-1762
+ Starts 2 routers:
+ Router A listener serves a normal, good certificate
+ Router B connector is configured with a bad CA in its profile but verifyHostname is false.
+ Test proves:
+ Router B must not connect to A.
+
+ DISPATCH-1762 fixes several issues related to failed ssl setup
+ allowing a connector to connect anyway.
+ This test checks only one of the failures.
+ """
+ # 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(RouterTestSslInterRouterVerifyHostname, cls).setUpClass()
+
+ if not SASL.extended():
+ return
+
+ os.environ["ENV_SASL_PASSWORD"] = "password"
+
+ # Generate authentication DB
+ super(RouterTestSslInterRouterVerifyHostname, 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 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', {'host': '0.0.0.0', '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 bad caCertFile name. The correct name 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-BAD.pem'),
+ 'certFile': cls.ssl_file('client-certificate.pem'),
+ 'privateKeyFile': cls.ssl_file('client-private-key.pem'),
+ 'ciphers': 'ECDH+AESGCM:DH+AESGCM:ECDH+AES256:DH+AES256:ECDH+AES128:' \
+ 'DH+AES:RSA+AESGCM:RSA+AES:!aNULL:!MD5:!DSS',
+ 'password': 'client-password'})
+ ])
+
+ 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
+
+ @SkipIfNeeded(RouterTestSslBase.DISABLE_SSL_TESTING or not SASL.extended(),
+ "Cyrus library not available. skipping test")
+ def test_connected_misconfigured_tls_sasl_routers(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 = " SERVER (error) SSL CA configuration failed"
+ sleep_time = 0.1 # seconds
+ poll_duration = 10.0 # seconds
+ verified = False
+ for tries in range(int(poll_duration / sleep_time)):
+ with open('../setUpClass/B.log', '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.assertTrue(node not in router_nodes, "%s should not be connected" % node)
Review comment:
Use assertNotIn instead of assertTrue
https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertNotIn
https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertNotIn
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #843: DISPATCH-1762: Connector ssl config errors must prohibit connections
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #843:
URL: https://github.com/apache/qpid-dispatch/pull/843#discussion_r482440099
##########
File path: src/server.c
##########
@@ -1219,6 +1222,8 @@ static void setup_ssl_sasl_and_open(qd_connection_t *ctx)
qd_log(ct->server->log_source, QD_LOG_ERROR,
"SSL local configuration failed for %s:%s",
config->host, config->port);
+ pn_ssl_domain_free(domain);
+ return;
Review comment:
could we populate a boolean inside these if statements and check it finally and then return ? That would keep the code behaving as it was earlier.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ganeshmurthy commented on pull request #843: DISPATCH-1762: Connector ssl config errors must prohibit connections
Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on pull request #843:
URL: https://github.com/apache/qpid-dispatch/pull/843#issuecomment-689050077
Chuck, this looks good. Please free to squash and commit to master. Thank you
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #843: DISPATCH-1762: Connector ssl config errors must prohibit connections
Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #843:
URL: https://github.com/apache/qpid-dispatch/pull/843#discussion_r482470394
##########
File path: src/server.c
##########
@@ -1219,6 +1222,8 @@ static void setup_ssl_sasl_and_open(qd_connection_t *ctx)
qd_log(ct->server->log_source, QD_LOG_ERROR,
"SSL local configuration failed for %s:%s",
config->host, config->port);
+ pn_ssl_domain_free(domain);
+ return;
Review comment:
yes. fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org