gemmellr commented on a change in pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282#discussion_r664444156
##########
File path: tests/system_tests_qdmanage.py
##########
@@ -612,6 +622,64 @@ def test_check_memory_usage(self):
# is added
self.assertTrue(mem is None)
+ def test_ssl_connection(self):
+ """
+ Verify qdmanage can securely connect via SSL:
+ """
+ ssl_address = "amqps://localhost:%s" % self.secure_port
+ ssl_user_address = "amqps://localhost:%s" % self.secure_user_port
+ query = 'QUERY --type org.apache.qpid.dispatch.router'
+
+ # this should fail: no trustfile
+ try:
+ self.run_qdmanage(query, address=ssl_address)
+ self.assertTrue(False, "expected qdmanage to fail")
Review comment:
I'd say its more than tiny, makes a huge difference to readability, much
easier to skim and understand a straight-up "fail" than considering the
assertions.
(Aside, comment applies to the many instances of this)
##########
File path: tests/system_tests_qdstat.py
##########
@@ -895,6 +921,29 @@ def test_ssl_trustfile_cert_to_unsecured(self):
def test_ssl_bad_trustfile_to_unsecured(self):
self.ssl_test_bad('unsecured_s', ['bad_trustfile'])
+ def test_ssl_peer_name_verify_disabled(self):
+ """
+ Verify the --ssl-disable-peer-name-verify option
+ """
+ params = [x for x in self.get_ssl_args()['trustfile']]
+ try:
+ self.run_qdstat(address="amqps://127.0.0.1:%s" %
self.strict_port,
+ args=['--general'] + params)
+ self.assertTrue(False, "Expected connection attempt to fail!")
+ except AssertionError:
+ # Expect the connection to fail, since the certificate has
+ # 'localhost' as the peer name and we used '127.0.0.1' instead.
+ pass
Review comment:
This was my main comment from looking it over. I expect its because
run_qdstat performs asserts and throws on failure, per ssl_test_bad, but as you
say catching that assertion will break the fallback failure when it succeeds
connecting unexpectedly, since it obviously does too.
##########
File path: tests/system_tests_qdstat.py
##########
@@ -895,6 +921,29 @@ def test_ssl_trustfile_cert_to_unsecured(self):
def test_ssl_bad_trustfile_to_unsecured(self):
self.ssl_test_bad('unsecured_s', ['bad_trustfile'])
+ def test_ssl_peer_name_verify_disabled(self):
+ """
+ Verify the --ssl-disable-peer-name-verify option
+ """
+ params = [x for x in self.get_ssl_args()['trustfile']]
+ try:
+ self.run_qdstat(address="amqps://127.0.0.1:%s" %
self.strict_port,
+ args=['--general'] + params)
+ self.assertTrue(False, "Expected connection attempt to fail!")
+ except AssertionError:
+ # Expect the connection to fail, since the certificate has
+ # 'localhost' as the peer name and we used '127.0.0.1' instead.
+ pass
Review comment:
This was my main comment from looking it over. I expect its because
run_qdstat performs asserts and throws on failure, per ssl_test_bad, but as you
say catching that assertion in this way will break the fallback failure when it
succeeds connecting unexpectedly, since it obviously does too.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
