absurdfarce commented on code in PR #1293:
URL:
https://github.com/apache/cassandra-python-driver/pull/1293#discussion_r3574595421
##########
tests/integration/long/test_ssl.py:
##########
@@ -314,10 +305,6 @@ def test_cannot_connect_with_bad_client_auth(self):
'ssl_version': ssl_version,
'keyfile': DRIVER_KEYFILE}
- if not USES_PYOPENSSL:
- # I don't set the bad certfile for pyopenssl because it hangs
- ssl_options['certfile'] = DRIVER_CERTFILE_BAD
-
cluster = TestCluster(
Review Comment:
So, I agree that ssl_options['certfile'] should be set in the general case.
We were only avoiding setting it when using pyOpenSSL and we're no longer doing
that... so it should just be part of the default case now.
What I _don't_ understand is why we define ssl_options above and then fail
to use it as an arg to the TestCluster constructor. Instead we... create
exactly the same object inline. Let's fix that while we're here; I'm thinking
something like:
```python
ssl_options = {'ca_certs': CLIENT_CA_CERTS,
'ssl_version': ssl_version,
'keyfile': DRIVER_KEYFILE,
'certfile': DRIVER_CERTFILE_BAD}
cluster = TestCluster(ssl_options=ssl_options)
```
##########
tests/integration/long/test_ssl.py:
##########
Review Comment:
Unrelated to removing the event loops but this logic makes no sense anymore.
ssl.match_hostname() was deprecated in Python 3.7 and removed in Python 3.12
(see [PYTHON-1331](https://datastax-oss.atlassian.net/browse/PYTHON-1331)).
Changes for PYTHON-1331 removed any usage of ssl.match_hostname() generally
(see the [corresponding
PR](https://github.com/apache/cassandra-python-driver/pull/1191) for more on
that point) so this check seems entirely out-of-date.
##########
tests/integration/long/test_ssl.py:
##########
@@ -22,8 +22,6 @@
from cassandra import ConsistencyLevel
from cassandra.query import SimpleStatement
-from OpenSSL import SSL, crypto
-
from tests.integration import (
get_cluster, remove_cluster, use_single_node, start_cluster_wait_for_up,
EVENT_LOOP_MANAGER, TestCluster
)
Review Comment:
Agreed, we should be able to safely remove this now.
##########
tests/integration/long/test_ssl.py:
##########
@@ -50,16 +48,9 @@
DRIVER_CERTFILE =
os.path.abspath("tests/integration/long/ssl/client.crt_signed")
DRIVER_CERTFILE_BAD =
os.path.abspath("tests/integration/long/ssl/client_bad.key")
-USES_PYOPENSSL = "twisted" in EVENT_LOOP_MANAGER or "eventlet" in
EVENT_LOOP_MANAGER
-if "twisted" in EVENT_LOOP_MANAGER:
- import OpenSSL
- ssl_version = OpenSSL.SSL.TLSv1_2_METHOD
- verify_certs = {'cert_reqs': SSL.VERIFY_PEER,
- 'check_hostname': True}
-else:
- ssl_version = ssl.PROTOCOL_TLS
- verify_certs = {'cert_reqs': ssl.CERT_REQUIRED,
- 'check_hostname': True}
+ssl_version = ssl.PROTOCOL_TLS
+verify_certs = {'cert_reqs': ssl.CERT_REQUIRED,
+ 'check_hostname': True}
Review Comment:
Yup, agree with this one as well.
--
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]
