[
https://issues.apache.org/jira/browse/DISPATCH-2190?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17375439#comment-17375439
]
ASF GitHub Bot commented on DISPATCH-2190:
------------------------------------------
jiridanek commented on a change in pull request #1282:
URL: https://github.com/apache/qpid-dispatch/pull/1282#discussion_r663343758
##########
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")
+ except Exception as exc:
Review comment:
There is a `with: block in unittest lib for constructing this style of
tests,
https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertRaises
I am not sure rewriting it to that form, but maybe for future tests...
I'd like to see catching a more specific exception class than just
Exception; I'll try to see in the code if that's practical. (The assert
regarding exception test is sufficient protection against catching unrelated
exception here, I just want to see if it could be even more specific early on.)
##########
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:
```suggestion
self.fail("expected qdmanage to fail")
```
tiny improvement regarding unittest API,
https://docs.python.org/3/library/unittest.html#unittest.TestCase.fail
##########
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:
Don't catch `AssertionError`? Catching AssertionError makes the test
pass when the connection attempt does not fail.
##########
File path: tests/system_tests_qdstat.py
##########
@@ -760,7 +774,19 @@ def get_ssl_args(self):
return args
def ssl_test(self, url_name, arg_names):
- """Run simple SSL connection test with supplied parameters.
+ """
Review comment:
The summary sentence of the docstring should be right after `""""`, on
the same line https://www.python.org/dev/peps/pep-0008/#documentation-strings
and linked https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings
There are some linters for this in flake8 but they are disabled for Dispatch.
##########
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")
+ except Exception as exc:
Review comment:
So, the more specific exception won't work all that well for running a
subprocess... never mind that.
##########
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")
+ except Exception as exc:
+ self.assertIn("certificate verify failed", str(exc),
+ "unexpected exception: %s" % str(exc))
+
+ # this should pass:
+ self.run_qdmanage(query + " --ssl-trustfile " +
+ self.ssl_file('ca-certificate.pem'),
+ address=ssl_address)
Review comment:
```suggestion
with self.subTest("this should pass"):
self.run_qdmanage(query + " --ssl-trustfile " +
self.ssl_file('ca-certificate.pem'),
address=ssl_address)
```
It is possible to add a bit more structure in the test like this. Turning
comments into subtest names,
https://docs.python.org/3/library/unittest.html#unittest.TestCase.subTest
--
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]
> Need a test to verify "ssl-disable-peer-name-verify" qdstat/qdmanage option
> ---------------------------------------------------------------------------
>
> Key: DISPATCH-2190
> URL: https://issues.apache.org/jira/browse/DISPATCH-2190
> Project: Qpid Dispatch
> Issue Type: Test
> Components: Tools
> Affects Versions: 1.16.0
> Reporter: Ken Giusti
> Assignee: Ken Giusti
> Priority: Major
> Fix For: 1.17.0
>
>
> Test should:
> # configure SSL/TLS on router listener
> # run qdstat/qdmanage with '127.0.0.1' as server host address
> # confirm qdstat/qdmanage cannot connect
> # re-run qdstat/qdmanage with 127.0.0.1 as server host address AND specify
> --ssl-disable-peer-name-verify option
> # confirm qdstat/qdmanage can connect
> # Profit!!!
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
