Add python 3.12 and 3.13 support #707

blink1073 · 2023-10-04T16:50:14Z

Fixes #706

blink1073 · 2023-10-04T16:52:19Z

The traceback on my fork is:

kmip/tests/integration/conftest.py:49: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
kmip/pie/client.py:173: in open
    self.proxy.open()
kmip/services/kmip_client.py:285: in open
    six.reraise(*last_error)
.tox/integration/lib/python3.9/site-packages/six.py:719: in reraise
    raise value
kmip/services/kmip_client.py:274: in open
    self.socket.connect((self.host, self.port))
/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/ssl.py:1376: in connect
    self._real_connect(addr, False)
/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/ssl.py:1367: in _real_connect
    self.do_handshake()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <ssl.SSLSocket [closed] fd=-1, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0>
block = False

    @_sslcopydoc
    def do_handshake(self, block=False):
        self._check_connected()
        timeout = self.gettimeout()
        try:
            if timeout == 0.0 and block:
                self.settimeout(None)
>           self._sslobj.do_handshake()
E           ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self-signed certificate (_ssl.c:1129)

/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/ssl.py:1343: SSLCertVerificationError
_ ERROR at setup of TestProxyKmipClientIntegration.test_x509_certificate_register_get_destroy _

request = <SubRequest 'simple' for <TestCaseFunction test_asymmetric_key_pair_create_get_destroy>>

    @pytest.fixture(scope="class")
    def simple(request):
        config = request.config.getoption("--config")
    
        client = pclient.ProxyKmipClient(config=config)
>       client.open()

kmip/tests/integration/conftest.py:49: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
kmip/pie/client.py:173: in open
    self.proxy.open()
kmip/services/kmip_client.py:285: in open
    six.reraise(*last_error)
.tox/integration/lib/python3.9/site-packages/six.py:719: in reraise
    raise value
kmip/services/kmip_client.py:274: in open
    self.socket.connect((self.host, self.port))
/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/ssl.py:1376: in connect
    self._real_connect(addr, False)
/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/ssl.py:1367: in _real_connect
    self.do_handshake()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <ssl.SSLSocket [closed] fd=-1, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0>
block = False

    @_sslcopydoc
    def do_handshake(self, block=False):
        self._check_connected()
        timeout = self.gettimeout()
        try:
            if timeout == 0.0 and block:
                self.settimeout(None)
>           self._sslobj.do_handshake()
E           ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self-signed certificate (_ssl.c:1129)

/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/ssl.py:1343: SSLCertVerificationError

kmip/services/kmip_client.py

+        context.check_hostname = False
+        if self.certfile:
+            context.load_cert_chain(self.certfile, self.keyfile)
+        self.socket = context.wrap_socket(


kmip/services/server/server.py

+            keyfile = self.config.settings.get('key_path')
+            context.load_cert_chain(certfile, keyfile=keyfile)
+
+        self._socket = context.wrap_socket(


s-t-e-v-e-n-k · 2024-03-28T03:37:54Z

The problem with your mocking is that you can't mock ssl.SSLContext.wrap_socket because the code constructs an SSLContext object and then calls wrap_socket on that, so you're not mocking the right thing.

I've edited the test changes for your patch (and not your code changes, which looks good) thusly:

@@ -210,9 +210,9 @@ class TestKmipServer(testtools.TestCase)
         # Test that in ideal cases no errors are generated and the right
         # log messages are.
         with mock.patch('socket.socket') as socket_mock:
-            with mock.patch('ssl.wrap_socket') as ssl_mock:
+            with mock.patch('ssl.SSLContext') as ssl_mock:
                 socket_mock.return_value = a_mock
-                ssl_mock.return_value = b_mock
+                ssl_mock.return_value.wrap_socket.return_value = b_mock

                 manager_mock.assert_not_called()
                 monitor_mock.assert_not_called()

And I've confirmed the tests pass with Python 3.9-3.12.

This reverts commit 4618b1d.

blink1073 · 2024-03-29T16:44:40Z

Thanks @s-t-e-v-e-n-k, new build in progress on my fork.

blink1073 · 2024-03-29T16:47:55Z

Still no joy: ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self-signed certificate (_ssl.c:1133)

avikivity · 2024-06-02T19:11:33Z

@blink1073 were you able to make this work?

blink1073 · 2024-06-03T11:31:42Z

No, I haven't been able to figure out the testing portion.

petere · 2024-07-24T21:52:02Z

kmip/services/kmip_client.py

@@ -285,13 +285,14 @@ def open(self):
            six.reraise(*last_error)

    def _create_socket(self, sock):
-        self.socket = ssl.wrap_socket(
+        purpose = ssl.Purpose.SERVER_AUTH
+        context = ssl.create_default_context(purpose=purpose, capath=self.ca_certs)


This should be cafile=... instead of capath=..., because the underlying configuration parameter points to a file.

petere · 2024-07-24T21:53:03Z

kmip/services/server/server.py

-        self._socket = ssl.wrap_socket(
+        purpose = ssl.Purpose.CLIENT_AUTH
+        capath = self.config.settings.get('ca_path')
+        context = ssl.create_default_context(purpose=purpose, capath=capath)


This should be cafile=capath instead of capath=capath, because the underlying configuration parameter points to a file. (The naming is unfortunately inconsistent between the Python ssl module and pykmip.)

petere · 2024-07-24T22:03:01Z

kmip/services/kmip_client.py

This no longer pays attention to the configuration settings self.cert_reqs and self.ssl_versions. That's probably okay in practice, since the defaults are good, but these settings are documented, so they should work.

One option might be to not use ssl.create_default_context() and instead use ssl.SSLContext directly and apply these settings. Like

context = ssl.SSLContext(self.ssl_version) context.verify_mode = self.cert_reqs if self.ca_certs: context.load_verify_locations(self.ca_certs)

This is just about as much code as before but it reproduces the previous behavior more precisely.

petere · 2024-07-24T22:06:50Z

kmip/services/server/server.py

Similar to the client, we should make sure that this change does not lose any previously working configuration settings. I see for example that ssl_version=self.auth_suite.protocol and ciphers=self.auth_suite.ciphers don't get used anymore.

petere · 2024-07-24T22:13:07Z

kmip/services/server/server.py

-        self._socket = ssl.wrap_socket(
+        purpose = ssl.Purpose.CLIENT_AUTH
+        capath = self.config.settings.get('ca_path')
+        context = ssl.create_default_context(purpose=purpose, capath=capath)


You must set

context.verify_mode=ssl.CERT_REQUIRED

after creating context. This matches what the previous code did. Without this, the server won't ask the client to send a certificate, and without a client certificate this will later fail at higher levels (such as

PyKMIP/kmip/services/server/session.py

Line 144 in 4d3b5a5

certificate = auth.get_certificate_from_connection(

).

blink1073 · 2024-07-25T00:07:12Z

Thanks @petere! I believe I've addressed your feedback.

petere · 2024-07-26T09:56:51Z

kmip/services/kmip_client.py

+        context.verify_mode = self.cert_reqs
+        if self.ca_certs:
+            context.load_verify_locations(self.ca_certs)
+        context.check_hostname = False


This line doesn't appear to be needed. Probably left over from a previous version that used create_default_context().

petere · 2024-07-26T10:00:53Z

kmip/services/kmip_client.py

+        if self.ca_certs:
+            context.load_verify_locations(self.ca_certs)
+        context.check_hostname = False
+        if self.certfile:


I'm testing combinations of "forgetting" to specify certfile or keyfile in the configuration file. With this code, if you specify certfile but not keyfile, you'll get an exception from load_cert_chain. If you specify keyfile but not certfile, this code is skipped and you'll get whatever the SSL stack ends up deciding. The old module-level wrap_socket had an additional check

if keyfile and not certfile: raise ValueError("certfile must be specified")

to prevent this. Maybe this would be good to have here as well.

petere · 2024-07-26T10:03:10Z

kmip/services/server/server.py

@@ -287,17 +287,24 @@ def interrupt_handler(trigger, frame):
        for cipher in auth_suite_ciphers:
            self._logger.debug(cipher)

-        self._socket = ssl.wrap_socket(
+        cafile = self.config.settings.get('ca_path')
+        context = ssl.SSLContext(self.ssl_version)


This should be

context = ssl.SSLContext(self.auth_suite.protocol)

(wrongly copy-pasted from client code?)

petere · 2024-07-26T10:04:46Z

kmip/services/server/server.py

+        cafile = self.config.settings.get('ca_path')
+        context = ssl.SSLContext(self.ssl_version)
+        context.verify_mode = ssl.CERT_REQUIRED
+        if auth_suite_ciphers:


This should be self.auth_suite.ciphers. I'm not sure exactly what the relationship between that and auth_suite_ciphers is, but this one crashes.

petere · 2024-07-26T10:08:23Z

kmip/services/server/server.py

+            context.load_verify_locations(cafile)
+        certfile = self.config.settings.get('certificate_path')
+
+        if certfile:


As with the client, it's worth checking here about how different combinations of not specifying certificate or key behave. For example, the old module-level wrap_socket() had a check

if server_side and not certfile: raise ValueError("certfile must be specified for server-side " "operations")

so omitting the certificate would have been a hard error.

I think what I had in mind was like

if certfile: keyfile = self.config.settings.get('key_path') context.load_cert_chain(certfile, keyfile=keyfile) + else: + raise ValueError("certfile must be specified for server-side operations")

I instead let the context raise an appropriate error. This has the benefit of letting us mock the context behavior in the tests.

petere · 2024-07-29T06:56:24Z

kmip/services/server/server.py

+            keyfile = self.config.settings.get('key_path')
+            context.load_cert_chain(certfile, keyfile=keyfile)
+        else:
+            raise ValueError("certfile must be specified for server-side operations")


What we don't have here is the equivalent of the client-side check

if self.keyfile and not self.certfile: raise ValueError("certfile must be specified")

but it also turns out that the higher levels already check that both certificate and key are specified:

kmip.core.exceptions.ConfigurationError: Setting 'key_path' is missing from the configuration file. kmip.core.exceptions.ConfigurationError: Setting 'certificate_path' is missing from the configuration file.

So it's probably not necessary.

Also, it looks like this causes the test suite to fail in TestKmipServer.test_start:

ValueError: certfile must be specified for server-side operations

So either this code or the test setup would need adjustments.

Removed from the server

from OpenKMIP#707

blink1073 · 2024-11-14T21:40:44Z

@petere is there anything else you'd like me to try?

petere · 2024-11-27T15:51:19Z

Other than the single comment I just added (which I just got my looking at my local copy, but it's been a while since I played with this), I am content with this version.

How we get from that to getting this merged and released, I don't know.

…rap-socket

blink1073 · 2024-12-02T12:22:58Z

This is now ready for review. cc @arp102.

blink1073 · 2024-12-02T12:23:12Z

The workflows are all passing on my fork: https://github.com/blink1073/PyKMIP/actions/runs/12119149288?pr=1

blink1073 added 10 commits October 4, 2023 09:59

Add python 3.12 support

433ab3e

fix usage

f16f36d

fix usage

c90654c

cleanup

61c5528

fix cert handling

06f3a1f

add capath

22fc83c

try loading default certs

519cbdc

cleanup

ec91215

fixups

a8e44f3

cleanup

eea62bd

github-advanced-security bot found potential problems Oct 4, 2023

View reviewed changes

Use all original options

4618b1d

blink1073 added 2 commits March 29, 2024 11:42

Revert "Use all original options"

c3a5dbc

This reverts commit 4618b1d.

Apply fix

5a02eb7

petere suggested changes Jul 24, 2024

View reviewed changes

address review

2897e40

blink1073 requested review from petere July 25, 2024 00:07

petere suggested changes Jul 26, 2024

View reviewed changes

blink1073 added 3 commits July 26, 2024 08:19

address review

cc035f4

fix client

d206639

lint

777c70d

oittaa mentioned this pull request Jul 28, 2024

ssl.PROTOCOL_TLS, ssl.PROTOCOL_TLSv1_2, and so on are deprecated #719

Open

petere approved these changes Jul 29, 2024

View reviewed changes

petere added a commit to petere/PyKMIP that referenced this pull request Jul 29, 2024

Fix Python 3.12 support

01c3a9f

from OpenKMIP#707

remove extra check

3da6f8d

blink1073 requested a review from petere July 29, 2024 18:37

blink1073 added 10 commits November 30, 2024 13:40

address review

80faaf6

Merge branch 'master' of github.com:OpenKMIP/PyKMIP into remove-ssl-w…

452693a

…rap-socket

fix test

4b781ff

address docs error

529e123

leave validation to the context

da34f31

mock load_cert_chain

1b46974

fix wrap

8352de5

fix stop test

042378b

fix stop test

65c6993

add python 3.13 support

5b736ad

blink1073 changed the title ~~Add python 3.12 support~~ Add python 3.12 and 3.13 support Dec 2, 2024

blink1073 marked this pull request as ready for review December 2, 2024 12:21

blink1073 mentioned this pull request Dec 3, 2024

DRIVERS-2743 Add support for Python 3.12 and Python 3.13 mongodb-labs/drivers-evergreen-tools#555

Merged

Add python 3.12 and 3.13 support #707

Are you sure you want to change the base?

Add python 3.12 and 3.13 support #707

Conversation

blink1073 commented Oct 4, 2023 • edited Loading

blink1073 commented Oct 4, 2023

s-t-e-v-e-n-k commented Mar 28, 2024

blink1073 commented Mar 29, 2024

blink1073 commented Mar 29, 2024

avikivity commented Jun 2, 2024

blink1073 commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blink1073 commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blink1073 commented Nov 14, 2024

petere commented Nov 27, 2024

blink1073 commented Dec 2, 2024

blink1073 commented Dec 2, 2024

blink1073 commented Oct 4, 2023 •

edited

Loading